From 2478270972a7ad6148ee714778b27585134e12a6 Mon Sep 17 00:00:00 2001 From: Kerry Liu Date: Fri, 15 Oct 2021 09:34:19 -0700 Subject: [PATCH 1/6] List View: add performance test toggling list view, have list view open during typing and focus tests --- bin/plugin/commands/performance.js | 18 +++++++++- packages/e2e-test-utils/README.md | 8 +++++ packages/e2e-test-utils/src/index.js | 1 + packages/e2e-test-utils/src/list-view.js | 33 +++++++++++++++++++ .../e2e-tests/config/performance-reporter.js | 16 +++++++++ .../specs/performance/post-editor.test.js | 29 ++++++++++++++-- .../specs/performance/site-editor.test.js | 11 +++++-- 7 files changed, 111 insertions(+), 5 deletions(-) create mode 100644 packages/e2e-test-utils/src/list-view.js diff --git a/bin/plugin/commands/performance.js b/bin/plugin/commands/performance.js index 6bb98ec4646bb..96f8674098ea7 100644 --- a/bin/plugin/commands/performance.js +++ b/bin/plugin/commands/performance.js @@ -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 */ /** @@ -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. @@ -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. */ /** @@ -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 ), }; } @@ -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 ); diff --git a/packages/e2e-test-utils/README.md b/packages/e2e-test-utils/README.md index aa7d460dac56d..2e29a1b8475bd 100644 --- a/packages/e2e-test-utils/README.md +++ b/packages/e2e-test-utils/README.md @@ -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. @@ -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. diff --git a/packages/e2e-test-utils/src/index.js b/packages/e2e-test-utils/src/index.js index b0613bc3a7d41..6901625577e96 100644 --- a/packages/e2e-test-utils/src/index.js +++ b/packages/e2e-test-utils/src/index.js @@ -90,5 +90,6 @@ export { rest as __experimentalRest, batch as __experimentalBatch, } from './rest-api'; +export { openListView, closeListView } from './list-view'; export * from './mocks'; diff --git a/packages/e2e-test-utils/src/list-view.js b/packages/e2e-test-utils/src/list-view.js new file mode 100644 index 0000000000000..03121c5f84af7 --- /dev/null +++ b/packages/e2e-test-utils/src/list-view.js @@ -0,0 +1,33 @@ +async function toggleListView() { + await page.click( + '.edit-post-header-toolbar__list-view-toggle, .edit-site-header-toolbar__list-view-toggle' + ); +} + +async function isListViewOpen() { + return await page.evaluate( () => { + return !! document.querySelector( + '.edit-post-header-toolbar__list-view-toggle.is-pressed, .edit-site-header-toolbar__list-view-toggle.is-pressed' + ); + } ); +} + +/** + * Opens list view + */ +export async function openListView() { + const isOpen = await isListViewOpen(); + if ( ! isOpen ) { + await toggleListView(); + } +} + +/** + * Closes list view + */ +export async function closeListView() { + const isOpen = await isListViewOpen(); + if ( isOpen ) { + await toggleListView(); + } +} diff --git a/packages/e2e-tests/config/performance-reporter.js b/packages/e2e-tests/config/performance-reporter.js index 6c314917f8ac2..549855bf29713 100644 --- a/packages/e2e-tests/config/performance-reporter.js +++ b/packages/e2e-tests/config/performance-reporter.js @@ -37,6 +37,7 @@ class PerformanceReporter { firstBlock, type, focus, + listViewOpen, inserterOpen, inserterHover, inserterSearch, @@ -90,6 +91,21 @@ Fastest time to select a block: ${ success( ) }` ); } + if ( listViewOpen && listViewOpen.length ) { + // eslint-disable-next-line no-console + console.log( ` +${ title( 'Opening List View Performance:' ) } +Average time to open list view: ${ success( + round( average( listViewOpen ) ) + 'ms' + ) } +Slowest time to open list view: ${ success( + round( Math.max( ...listViewOpen ) ) + 'ms' + ) } +Fastest time to open list view: ${ success( + round( Math.min( ...listViewOpen ) ) + 'ms' + ) }` ); + } + if ( inserterOpen && inserterOpen.length ) { // eslint-disable-next-line no-console console.log( ` diff --git a/packages/e2e-tests/specs/performance/post-editor.test.js b/packages/e2e-tests/specs/performance/post-editor.test.js index f9862f18c9448..fa1893923a29f 100644 --- a/packages/e2e-tests/specs/performance/post-editor.test.js +++ b/packages/e2e-tests/specs/performance/post-editor.test.js @@ -11,9 +11,10 @@ import { sum } from 'lodash'; import { createNewPost, saveDraft, - insertBlock, openGlobalBlockInserter, closeGlobalBlockInserter, + openListView, + closeListView, } from '@wordpress/e2e-test-utils'; /** @@ -41,6 +42,7 @@ describe( 'Post Editor Performance', () => { firstBlock: [], type: [], focus: [], + listViewOpen: [], inserterOpen: [], inserterHover: [], inserterSearch: [], @@ -118,7 +120,9 @@ describe( 'Post Editor Performance', () => { it( 'Typing', async () => { // Measuring typing performance - await insertBlock( 'Paragraph' ); + await openListView(); + await page.click( '.edit-post-visual-editor__post-title-wrapper' ); + await page.keyboard.press( 'Enter' ); let i = 20; await page.tracing.start( { path: traceFile, @@ -157,6 +161,7 @@ describe( 'Post Editor Performance', () => { it( 'Selecting blocks', async () => { // Measuring block selection performance await createNewPost(); + await openListView(); await page.evaluate( () => { const { createBlock } = window.wp.blocks; const { dispatch } = window.wp.data; @@ -184,6 +189,26 @@ describe( 'Post Editor Performance', () => { results.focus = focusEvents; } ); + it( 'Opening persistent list view', async () => { + // Measure time to open inserter + await page.waitForSelector( '.edit-post-layout' ); + for ( let j = 0; j < 10; j++ ) { + await page.tracing.start( { + path: traceFile, + screenshots: false, + categories: [ 'devtools.timeline' ], + } ); + await openListView(); + await page.tracing.stop(); + traceResults = JSON.parse( readFile( traceFile ) ); + const [ mouseClickEvents ] = getClickEventDurations( traceResults ); + for ( let k = 0; k < mouseClickEvents.length; k++ ) { + results.listViewOpen.push( mouseClickEvents[ k ] ); + } + await closeListView(); + } + } ); + it( 'Opening the inserter', async () => { // Measure time to open inserter await page.waitForSelector( '.edit-post-layout' ); diff --git a/packages/e2e-tests/specs/performance/site-editor.test.js b/packages/e2e-tests/specs/performance/site-editor.test.js index 80fa8cecc3c95..0b7196fb805dc 100644 --- a/packages/e2e-tests/specs/performance/site-editor.test.js +++ b/packages/e2e-tests/specs/performance/site-editor.test.js @@ -13,7 +13,7 @@ import { canvas, createNewPost, saveDraft, - insertBlock, + openListView, } from '@wordpress/e2e-test-utils'; /** @@ -55,6 +55,7 @@ describe( 'Site Editor Performance', () => { inserterOpen: [], inserterHover: [], inserterSearch: [], + listViewOpen: [], }; const html = readFile( @@ -117,7 +118,13 @@ describe( 'Site Editor Performance', () => { await canvas().click( '[data-type="core/post-content"] [data-type="core/paragraph"]' ); - await insertBlock( 'Paragraph' ); + await openListView(); + await canvas().click( + '[data-type="core/post-content"] [data-type="core/paragraph"]' + ); + await page.keyboard.press( 'Enter' ); + await page.keyboard.press( 'Enter' ); + await page.keyboard.press( 'ArrowUp' ); i = 200; const traceFile = __dirname + '/trace.json'; await page.tracing.start( { From 413a1cb8bf513a317b4a21e33a2afe4dfc6122d9 Mon Sep 17 00:00:00 2001 From: Kerry Liu Date: Fri, 15 Oct 2021 09:47:31 -0700 Subject: [PATCH 2/6] remove onClick in branch --- .../src/components/list-view/block.js | 24 ++++++++++++------- .../src/components/list-view/branch.js | 7 +----- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/packages/block-editor/src/components/list-view/block.js b/packages/block-editor/src/components/list-view/block.js index f9b7bc8bbc973..dbea2ee9bdab6 100644 --- a/packages/block-editor/src/components/list-view/block.js +++ b/packages/block-editor/src/components/list-view/block.js @@ -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'; /** @@ -33,7 +33,7 @@ export default function ListViewBlock( { isDragged, isBranchSelected, isLastOfSelectedBranch, - onClick, + selectBlock, onToggleExpanded, position, level, @@ -82,14 +82,22 @@ 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 classes = classnames( { 'is-selected': isSelected, @@ -125,7 +133,7 @@ export default function ListViewBlock( {
) } diff --git a/packages/block-editor/src/components/list-view/branch.js b/packages/block-editor/src/components/list-view/branch.js index ccc0010b96a05..0fb3d14bee971 100644 --- a/packages/block-editor/src/components/list-view/branch.js +++ b/packages/block-editor/src/components/list-view/branch.js @@ -84,11 +84,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 ) { @@ -106,7 +101,7 @@ export default function ListViewBranch( props ) { Date: Fri, 15 Oct 2021 09:49:29 -0700 Subject: [PATCH 3/6] removed unused terminated levels --- packages/block-editor/src/components/list-view/branch.js | 8 -------- 1 file changed, 8 deletions(-) diff --git a/packages/block-editor/src/components/list-view/branch.js b/packages/block-editor/src/components/list-view/branch.js index 0fb3d14bee971..e0ca78a0ad5b7 100644 --- a/packages/block-editor/src/components/list-view/branch.js +++ b/packages/block-editor/src/components/list-view/branch.js @@ -25,7 +25,6 @@ export default function ListViewBranch( props ) { showNestedBlocks, parentBlockClientId, level = 1, - terminatedLevels = [], path = [], isBranchSelected = false, isLastOfBranch = false, @@ -56,10 +55,6 @@ 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 ]; const hasNestedBlocks = showNestedBlocks && !! innerBlocks && !! innerBlocks.length; @@ -112,7 +107,6 @@ export default function ListViewBranch( props ) { rowCount={ rowCount } siblingBlockCount={ blockCount } showBlockMovers={ showBlockMovers } - terminatedLevels={ terminatedLevels } path={ updatedPath } isExpanded={ isExpanded } /> @@ -127,7 +121,6 @@ export default function ListViewBranch( props ) { showNestedBlocks={ showNestedBlocks } parentBlockClientId={ clientId } level={ level + 1 } - terminatedLevels={ updatedTerminatedLevels } path={ updatedPath } /> ) } @@ -140,7 +133,6 @@ export default function ListViewBranch( props ) { position={ rowCount } rowCount={ appenderPosition } level={ level } - terminatedLevels={ terminatedLevels } path={ [ ...path, appenderPosition ] } /> ) } From 3f9de590f9be0ed695a465dd98217c91794f5fd0 Mon Sep 17 00:00:00 2001 From: Kerry Liu Date: Fri, 15 Oct 2021 09:54:18 -0700 Subject: [PATCH 4/6] use callback for toggle expand/collapse --- .../src/components/list-view/block.js | 17 +++++++++++++++-- .../src/components/list-view/branch.js | 12 ------------ 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/packages/block-editor/src/components/list-view/block.js b/packages/block-editor/src/components/list-view/block.js index dbea2ee9bdab6..ed494c61205e6 100644 --- a/packages/block-editor/src/components/list-view/block.js +++ b/packages/block-editor/src/components/list-view/block.js @@ -34,7 +34,6 @@ export default function ListViewBlock( { isBranchSelected, isLastOfSelectedBranch, selectBlock, - onToggleExpanded, position, level, rowCount, @@ -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', @@ -99,6 +100,18 @@ export default function ListViewBlock( { [ 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, 'is-branch-selected': @@ -134,7 +147,7 @@ export default function ListViewBlock( { { - 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 ); @@ -97,7 +86,6 @@ export default function ListViewBranch( props ) { Date: Fri, 15 Oct 2021 10:05:53 -0700 Subject: [PATCH 5/6] update path to be a string to avoid ref changes --- .../src/components/list-view/branch.js | 15 ++++++++++++--- .../block-editor/src/components/list-view/leaf.js | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/block-editor/src/components/list-view/branch.js b/packages/block-editor/src/components/list-view/branch.js index 7662fd1dbc303..8f6d71b3683bc 100644 --- a/packages/block-editor/src/components/list-view/branch.js +++ b/packages/block-editor/src/components/list-view/branch.js @@ -25,7 +25,7 @@ export default function ListViewBranch( props ) { showNestedBlocks, parentBlockClientId, level = 1, - path = [], + path = '', isBranchSelected = false, isLastOfBranch = false, } = props; @@ -53,7 +53,12 @@ export default function ListViewBranch( props ) { { map( filteredBlocks, ( block, index ) => { const { clientId, innerBlocks } = block; const position = index + 1; - 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 ); @@ -121,7 +126,11 @@ export default function ListViewBranch( props ) { position={ rowCount } rowCount={ appenderPosition } level={ level } - path={ [ ...path, appenderPosition ] } + path={ + path.length > 0 + ? `${ path }_${ appenderPosition }` + : `${ appenderPosition }` + } /> ) } diff --git a/packages/block-editor/src/components/list-view/leaf.js b/packages/block-editor/src/components/list-view/leaf.js index 8098d44647fc9..41bf4bc34cc66 100644 --- a/packages/block-editor/src/components/list-view/leaf.js +++ b/packages/block-editor/src/components/list-view/leaf.js @@ -30,7 +30,7 @@ export default function ListViewLeaf( { isSelected, adjustScrolling: false, enableAnimation: true, - triggerAnimationOnChange: path.join( '_' ), + triggerAnimationOnChange: path, } ); return ( From 1a09fcd2f415c2b0bfd89e1f62687f755e6456c1 Mon Sep 17 00:00:00 2001 From: Kerry Liu Date: Fri, 15 Oct 2021 10:45:54 -0700 Subject: [PATCH 6/6] ListViewBlockSelectButton: provide stable references for onDragStart, onDragEnd, onFocus --- .../src/components/block-draggable/index.js | 49 +++++++++++-------- .../components/list-view/block-contents.js | 8 ++- .../src/tree-grid/roving-tab-index-item.js | 7 ++- 3 files changed, 39 insertions(+), 25 deletions(-) diff --git a/packages/block-editor/src/components/block-draggable/index.js b/packages/block-editor/src/components/block-draggable/index.js index b1e539654f853..77cd7514eb62f 100644 --- a/packages/block-editor/src/components/block-draggable/index.js +++ b/packages/block-editor/src/components/block-draggable/index.js @@ -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 @@ -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 } ); } @@ -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={ } diff --git a/packages/block-editor/src/components/list-view/block-contents.js b/packages/block-editor/src/components/list-view/block-contents.js index 61e221b77f106..09ca5cb64d319 100644 --- a/packages/block-editor/src/components/list-view/block-contents.js +++ b/packages/block-editor/src/components/list-view/block-contents.js @@ -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 @@ -55,8 +55,12 @@ const ListViewBlockContents = forwardRef( 'is-dropping-before': isBlockMoveTarget, } ); + const clientIds = useMemo( () => [ block.clientId ], [ + block.clientId, + ] ); + return ( - + { ( { draggable, onDragStart, onDragEnd } ) => ( setLastFocusedElement( event.target ); + const onFocus = useCallback( + ( event ) => setLastFocusedElement( event.target ), + [ setLastFocusedElement ] + ); const allProps = { ref, tabIndex, onFocus, ...props }; if ( typeof children === 'function' ) {