Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

List View: improve block focus time by providing stable function references #35683

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion bin/plugin/commands/performance.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ const config = require( '../config' );
* @property {number[]} inserterOpen Average time to open global inserter.
* @property {number[]} inserterSearch Average time to search the inserter.
* @property {number[]} inserterHover Average time to move mouse between two block item in the inserter.
* @property {number[]} listViewOpen Average time to open listView
*/

/**
Expand All @@ -52,7 +53,7 @@ const config = require( '../config' );
* @property {number=} firstContentfulPaint Represents the time when the browser first renders any text or media.
* @property {number=} firstBlock Represents the time when Puppeteer first sees a block selector in the DOM.
* @property {number=} type Average type time.
* @property {number=} minType Minium type time.
* @property {number=} minType Minimum type time.
* @property {number=} maxType Maximum type time.
* @property {number=} focus Average block selection time.
* @property {number=} minFocus Min block selection time.
Expand All @@ -66,6 +67,9 @@ const config = require( '../config' );
* @property {number=} inserterHover Average time to move mouse between two block item in the inserter.
* @property {number=} minInserterHover Min time to move mouse between two block item in the inserter.
* @property {number=} maxInserterHover Max time to move mouse between two block item in the inserter.
* @property {number=} listViewOpen Average time to open list view.
* @property {number=} minListViewOpen Min time to open list view.
* @property {number=} maxListViewOpen Max time to open list view.
*/

/**
Expand Down Expand Up @@ -136,6 +140,9 @@ function curateResults( results ) {
inserterHover: average( results.inserterHover ),
minInserterHover: Math.min( ...results.inserterHover ),
maxInserterHover: Math.max( ...results.inserterHover ),
listViewOpen: average( results.listViewOpen ),
minListViewOpen: Math.min( ...results.listViewOpen ),
maxListViewOpen: Math.max( ...results.listViewOpen ),
};
}

Expand Down Expand Up @@ -378,6 +385,15 @@ async function runPerformanceTests( branches, options ) {
maxInserterHover: rawResults.map(
( r ) => r[ branch ].maxInserterHover
),
listViewOpen: rawResults.map(
( r ) => r[ branch ].listViewOpen
),
minListViewOpen: rawResults.map(
( r ) => r[ branch ].minListViewOpen
),
maxListViewOpen: rawResults.map(
( r ) => r[ branch ].maxListViewOpen
),
},
median
);
Expand Down
49 changes: 28 additions & 21 deletions packages/block-editor/src/components/block-draggable/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import { getBlockType } from '@wordpress/blocks';
import { Draggable } from '@wordpress/components';
import { useSelect, useDispatch } from '@wordpress/data';
import { useEffect, useRef } from '@wordpress/element';
import { useEffect, useRef, useCallback } from '@wordpress/element';

/**
* Internal dependencies
Expand Down Expand Up @@ -61,6 +61,31 @@ const BlockDraggable = ( {
};
}, [] );

const draggableOnDragStart = useCallback(
( event ) => {
startDraggingBlocks( clientIds );
isDragging.current = true;

startScrolling( event );

if ( onDragStart ) {
onDragStart();
}
},
[ clientIds, startScrolling, onDragStart ]
);

const draggableOnDragEnd = useCallback( () => {
stopDraggingBlocks();
isDragging.current = false;

stopScrolling();

if ( onDragEnd ) {
onDragEnd();
}
}, [ stopDraggingBlocks, isDragging, stopScrolling, onDragEnd ] );

if ( ! isDraggable ) {
return children( { isDraggable: false } );
}
Expand All @@ -76,27 +101,9 @@ const BlockDraggable = ( {
cloneClassname={ cloneClassname }
__experimentalTransferDataType="wp-blocks"
transferData={ transferData }
onDragStart={ ( event ) => {
startDraggingBlocks( clientIds );
isDragging.current = true;

startScrolling( event );

if ( onDragStart ) {
onDragStart();
}
} }
onDragStart={ draggableOnDragStart }
onDragOver={ scrollOnDragOver }
onDragEnd={ () => {
stopDraggingBlocks();
isDragging.current = false;

stopScrolling();

if ( onDragEnd ) {
onDragEnd();
}
} }
onDragEnd={ draggableOnDragEnd }
__experimentalDragComponent={
<BlockDraggableChip count={ clientIds.length } icon={ icon } />
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import classnames from 'classnames';
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';
import { forwardRef } from '@wordpress/element';
import { forwardRef, useMemo } from '@wordpress/element';

/**
* Internal dependencies
Expand Down Expand Up @@ -55,8 +55,12 @@ const ListViewBlockContents = forwardRef(
'is-dropping-before': isBlockMoveTarget,
} );

const clientIds = useMemo( () => [ block.clientId ], [
block.clientId,
] );

return (
<BlockDraggable clientIds={ [ block.clientId ] }>
<BlockDraggable clientIds={ clientIds }>
{ ( { draggable, onDragStart, onDragEnd } ) => (
<ListViewBlockSelectButton
ref={ ref }
Expand Down
41 changes: 31 additions & 10 deletions packages/block-editor/src/components/list-view/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
__experimentalTreeGridItem as TreeGridItem,
} from '@wordpress/components';
import { moreVertical } from '@wordpress/icons';
import { useState, useRef, useEffect } from '@wordpress/element';
import { useState, useRef, useEffect, useCallback } from '@wordpress/element';
import { useDispatch } from '@wordpress/data';

/**
Expand All @@ -33,8 +33,7 @@ export default function ListViewBlock( {
isDragged,
isBranchSelected,
isLastOfSelectedBranch,
onClick,
onToggleExpanded,
selectBlock,
position,
level,
rowCount,
Expand All @@ -59,6 +58,8 @@ export default function ListViewBlock( {
__experimentalFeatures: withExperimentalFeatures,
__experimentalPersistentListViewFeatures: withExperimentalPersistentListViewFeatures,
isTreeGridMounted,
expand,
collapse,
} = useListViewContext();
const listViewBlockSettingsClassName = classnames(
'block-editor-list-view-block__menu-cell',
Expand All @@ -82,14 +83,34 @@ export default function ListViewBlock( {
? toggleBlockHighlight
: () => {};

const onMouseEnter = () => {
const onMouseEnter = useCallback( () => {
setIsHovered( true );
highlightBlock( clientId, true );
};
const onMouseLeave = () => {
}, [ clientId, setIsHovered, highlightBlock ] );
const onMouseLeave = useCallback( () => {
setIsHovered( false );
highlightBlock( clientId, false );
};
}, [ clientId, setIsHovered, highlightBlock ] );

const selectEditorBlock = useCallback(
( event ) => {
event.stopPropagation();
selectBlock( clientId );
},
[ clientId, selectBlock ]
);

const toggleExpanded = useCallback(
( event ) => {
event.stopPropagation();
if ( isExpanded === true ) {
collapse( clientId );
} else if ( isExpanded === false ) {
expand( clientId );
}
},
[ clientId, expand, collapse, isExpanded ]
);

const classes = classnames( {
'is-selected': isSelected,
Expand Down Expand Up @@ -125,8 +146,8 @@ export default function ListViewBlock( {
<div className="block-editor-list-view-block__contents-container">
<ListViewBlockContents
block={ block }
onClick={ onClick }
onToggleExpanded={ onToggleExpanded }
onClick={ selectEditorBlock }
onToggleExpanded={ toggleExpanded }
isSelected={ isSelected }
position={ position }
siblingBlockCount={ siblingBlockCount }
Expand Down Expand Up @@ -183,7 +204,7 @@ export default function ListViewBlock( {
onFocus,
} }
disableOpenOnArrowDown
__experimentalSelectBlock={ onClick }
__experimentalSelectBlock={ selectEditorBlock }
/>
) }
</TreeGridCell>
Expand Down
42 changes: 13 additions & 29 deletions packages/block-editor/src/components/list-view/branch.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,13 @@ export default function ListViewBranch( props ) {
showNestedBlocks,
parentBlockClientId,
level = 1,
terminatedLevels = [],
path = [],
path = '',
isBranchSelected = false,
isLastOfBranch = false,
} = props;

const {
expandedState,
expand,
collapse,
draggedClientIds,
selectedClientIds,
} = useListViewContext();
Expand All @@ -56,11 +53,12 @@ export default function ListViewBranch( props ) {
{ map( filteredBlocks, ( block, index ) => {
const { clientId, innerBlocks } = block;
const position = index + 1;
const isLastRowAtLevel = rowCount === position;
const updatedTerminatedLevels = isLastRowAtLevel
? [ ...terminatedLevels, level ]
: terminatedLevels;
const updatedPath = [ ...path, position ];
// If the string value changes, it's used to trigger an animation change.
// This may be removed if we use a different animation library in the future.
const updatedPath =
path.length > 0
? `${ path }_${ position }`
: `${ position }`;
const hasNestedBlocks =
showNestedBlocks && !! innerBlocks && !! innerBlocks.length;
const hasNestedAppender = itemHasAppender( clientId );
Expand All @@ -84,20 +82,6 @@ export default function ListViewBranch( props ) {
? expandedState[ clientId ] ?? true
: undefined;

const selectBlockWithClientId = ( event ) => {
event.stopPropagation();
selectBlock( clientId );
};

const toggleExpanded = ( event ) => {
event.stopPropagation();
if ( isExpanded === true ) {
collapse( clientId );
} else if ( isExpanded === false ) {
expand( clientId );
}
};

// Make updates to the selected or dragged blocks synchronous,
// but asynchronous for any other block.
const isDragged = !! draggedClientIds?.includes( clientId );
Expand All @@ -106,8 +90,7 @@ export default function ListViewBranch( props ) {
<AsyncModeProvider key={ clientId } value={ ! isSelected }>
<ListViewBlock
block={ block }
onClick={ selectBlockWithClientId }
onToggleExpanded={ toggleExpanded }
selectBlock={ selectBlock }
isDragged={ isDragged }
isSelected={ isSelected }
isBranchSelected={ isSelectedBranch }
Expand All @@ -117,7 +100,6 @@ export default function ListViewBranch( props ) {
rowCount={ rowCount }
siblingBlockCount={ blockCount }
showBlockMovers={ showBlockMovers }
terminatedLevels={ terminatedLevels }
path={ updatedPath }
isExpanded={ isExpanded }
/>
Expand All @@ -132,7 +114,6 @@ export default function ListViewBranch( props ) {
showNestedBlocks={ showNestedBlocks }
parentBlockClientId={ clientId }
level={ level + 1 }
terminatedLevels={ updatedTerminatedLevels }
path={ updatedPath }
/>
) }
Expand All @@ -145,8 +126,11 @@ export default function ListViewBranch( props ) {
position={ rowCount }
rowCount={ appenderPosition }
level={ level }
terminatedLevels={ terminatedLevels }
path={ [ ...path, appenderPosition ] }
path={
path.length > 0
? `${ path }_${ appenderPosition }`
: `${ appenderPosition }`
}
/>
) }
</>
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/list-view/leaf.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export default function ListViewLeaf( {
isSelected,
adjustScrolling: false,
enableAnimation: true,
triggerAnimationOnChange: path.join( '_' ),
triggerAnimationOnChange: path,
} );

return (
Expand Down
7 changes: 5 additions & 2 deletions packages/components/src/tree-grid/roving-tab-index-item.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { useRef, forwardRef } from '@wordpress/element';
import { useRef, forwardRef, useCallback } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -24,7 +24,10 @@ export default forwardRef( function RovingTabIndexItem(
tabIndex = lastFocusedElement === ref.current ? 0 : -1;
}

const onFocus = ( event ) => setLastFocusedElement( event.target );
const onFocus = useCallback(
( event ) => setLastFocusedElement( event.target ),
[ setLastFocusedElement ]
);
const allProps = { ref, tabIndex, onFocus, ...props };

if ( typeof children === 'function' ) {
Expand Down
8 changes: 8 additions & 0 deletions packages/e2e-test-utils/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,10 @@ _Parameters_

Undocumented declaration.

### closeListView

Closes list view

### createEmbeddingMatcher

Creates a function to determine if a request is embedding a certain URL.
Expand Down Expand Up @@ -505,6 +509,10 @@ Clicks on the button in the header which opens Document Settings sidebar when it

Opens the global block inserter.

### openListView

Opens list view

### openPreviewPage

Opens the preview page of an edited post.
Expand Down
1 change: 1 addition & 0 deletions packages/e2e-test-utils/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,5 +90,6 @@ export {
rest as __experimentalRest,
batch as __experimentalBatch,
} from './rest-api';
export { openListView, closeListView } from './list-view';

export * from './mocks';
Loading