From f7229379f3fb197e246f3bb36da40d515e3f35c8 Mon Sep 17 00:00:00 2001 From: Kerry Liu Date: Wed, 29 Sep 2021 15:52:12 -0700 Subject: [PATCH 1/6] List View: experiment with only rendering a fixed number of items --- bin/plugin/commands/performance.js | 18 +- .../src/components/list-view/block.js | 2 + .../src/components/list-view/branch.js | 181 +++++++++++++----- .../src/components/list-view/index.js | 39 +++- .../src/hooks/use-fixed-window-list/index.js | 159 +++++++++++++++ packages/compose/src/index.js | 1 + .../e2e-tests/config/performance-reporter.js | 16 ++ .../specs/performance/post-editor.test.js | 29 ++- .../specs/performance/site-editor.test.js | 11 +- 9 files changed, 397 insertions(+), 59 deletions(-) create mode 100644 packages/compose/src/hooks/use-fixed-window-list/index.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/block-editor/src/components/list-view/block.js b/packages/block-editor/src/components/list-view/block.js index 97be704702f6e..c4987d59ee83f 100644 --- a/packages/block-editor/src/components/list-view/block.js +++ b/packages/block-editor/src/components/list-view/block.js @@ -39,6 +39,7 @@ export default function ListViewBlock( { showBlockMovers, path, isExpanded, + style, } ) { const cellRef = useRef( null ); const [ isHovered, setIsHovered ] = useState( false ); @@ -184,6 +185,7 @@ export default function ListViewBlock( { className="block-editor-list-view-block__contents-cell" colSpan={ colSpan } ref={ cellRef } + style={ style } > { ( { ref, tabIndex, onFocus } ) => (
diff --git a/packages/block-editor/src/components/list-view/branch.js b/packages/block-editor/src/components/list-view/branch.js index 222fbad712b59..3d262910f55be 100644 --- a/packages/block-editor/src/components/list-view/branch.js +++ b/packages/block-editor/src/components/list-view/branch.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import { map, compact } from 'lodash'; +import { compact } from 'lodash'; /** * WordPress dependencies @@ -14,6 +14,38 @@ import { Fragment } from '@wordpress/element'; import ListViewBlock from './block'; import { useListViewContext } from './context'; +function countBlocks( block, expandedState, draggedClientIds ) { + const isDragged = draggedClientIds?.includes( block.clientId ); + if ( isDragged ) { + return 0; + } + const isExpanded = expandedState[ block.clientId ] ?? true; + if ( isExpanded ) { + return ( + 1 + + block.innerBlocks.reduce( + countReducer( expandedState, draggedClientIds ), + 0 + ) + ); + } + return 1; +} +const countReducer = ( expandedState, draggedClientIds ) => ( + count, + block +) => { + const isDragged = draggedClientIds?.includes( block.clientId ); + if ( isDragged ) { + return count; + } + const isExpanded = expandedState[ block.clientId ] ?? true; + if ( isExpanded && block.innerBlocks.length > 0 ) { + return count + countBlocks( block, expandedState, draggedClientIds ); + } + return count + 1; +}; + export default function ListViewBranch( props ) { const { blocks, @@ -22,62 +54,109 @@ export default function ListViewBranch( props ) { showNestedBlocks, level = 1, path = '', + listPosition = 0, + fixedListWindow, } = props; - const { expandedState, draggedClientIds } = useListViewContext(); + const { + expandedState, + draggedClientIds, + __experimentalPersistentListViewFeatures, + } = useListViewContext(); const filteredBlocks = compact( blocks ); const blockCount = filteredBlocks.length; + let nextPosition = listPosition; + + const listItems = []; + for ( let index = 0; index < filteredBlocks.length; index++ ) { + const block = filteredBlocks[ index ]; + const { clientId, innerBlocks } = block; + + if ( index > 0 ) { + nextPosition += countBlocks( + filteredBlocks[ index - 1 ], + expandedState, + draggedClientIds + ); + } + + const usesWindowing = __experimentalPersistentListViewFeatures; + const { + start, + end, + itemInView, + startPadding, + endPadding, + } = fixedListWindow; + + const blockInView = ! usesWindowing || itemInView( nextPosition ); + + const isDragging = draggedClientIds?.length > 0; + if ( + usesWindowing && + ! isDragging && + ! blockInView && + nextPosition > start + ) { + // found the end of the window, don't bother processing the rest of the items + break; + } + const style = usesWindowing + ? { + paddingTop: start === nextPosition ? startPadding : 0, + paddingBottom: end === nextPosition ? endPadding : 0, + } + : undefined; + + const position = index + 1; + const updatedPath = + path.length > 0 ? `${ path }_${ position }` : `${ position }`; + const hasNestedBlocks = + showNestedBlocks && !! innerBlocks && !! innerBlocks.length; + + const isExpanded = hasNestedBlocks + ? expandedState[ clientId ] ?? true + : undefined; + + // Make updates to the selected or dragged blocks synchronous, + // but asynchronous for any other block. + const isDragged = !! draggedClientIds?.includes( clientId ); - return ( - <> - { map( filteredBlocks, ( block, index ) => { - const { clientId, innerBlocks } = block; - const position = index + 1; - // This string value is 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 isExpanded = hasNestedBlocks - ? expandedState[ clientId ] ?? true - : undefined; - - const isDragged = !! draggedClientIds?.includes( clientId ); - - return ( - - - { hasNestedBlocks && isExpanded && ! isDragged && ( - - ) } - - ); - } ) } - - ); + listItems.push( + + { ( isDragged || blockInView ) && ( + + ) } + { hasNestedBlocks && isExpanded && ! isDragged && ( + + ) } + + ); + } + return <>{ listItems }; } ListViewBranch.defaultProps = { diff --git a/packages/block-editor/src/components/list-view/index.js b/packages/block-editor/src/components/list-view/index.js index 8ce9f492b44ce..7f8379a53a3ac 100644 --- a/packages/block-editor/src/components/list-view/index.js +++ b/packages/block-editor/src/components/list-view/index.js @@ -1,10 +1,12 @@ /** * WordPress dependencies */ - -import { useMergeRefs } from '@wordpress/compose'; +import { + useMergeRefs, + __experimentalUseFixedWindowList as useFixedWindowList, +} from '@wordpress/compose'; import { __experimentalTreeGrid as TreeGrid } from '@wordpress/components'; -import { AsyncModeProvider, useDispatch } from '@wordpress/data'; +import { AsyncModeProvider, useDispatch, useSelect } from '@wordpress/data'; import { useCallback, useEffect, @@ -67,6 +69,21 @@ function ListView( ) { const { clientIdsTree, draggedClientIds } = useListViewClientIds( blocks ); const { selectBlock } = useDispatch( blockEditorStore ); + const { visibleBlockCount } = useSelect( + ( select ) => { + const { getGlobalBlockCount, getClientIdsOfDescendants } = select( + blockEditorStore + ); + const draggedBlockCount = + draggedClientIds?.length > 0 + ? getClientIdsOfDescendants( draggedClientIds ).length + 1 + : 0; + return { + visibleBlockCount: getGlobalBlockCount() - draggedBlockCount, + }; + }, + [ draggedClientIds ] + ); const selectEditorBlock = useCallback( ( clientId ) => { selectBlock( clientId ); @@ -85,6 +102,16 @@ function ListView( isMounted.current = true; }, [] ); + const [ fixedListWindow ] = useFixedWindowList( + elementRef, + 36, + visibleBlockCount, + { + windowOverscan: 1, + useWindowing: __experimentalPersistentListViewFeatures, + } + ); + const expand = useCallback( ( clientId ) => { if ( ! clientId ) { @@ -151,6 +178,11 @@ function ListView( ref={ treeGridRef } onCollapseRow={ collapseRow } onExpandRow={ expandRow } + aria-rowcount={ + __experimentalPersistentListViewFeatures + ? visibleBlockCount + : undefined + } > diff --git a/packages/compose/src/hooks/use-fixed-window-list/index.js b/packages/compose/src/hooks/use-fixed-window-list/index.js new file mode 100644 index 0000000000000..d47b1cbf109f5 --- /dev/null +++ b/packages/compose/src/hooks/use-fixed-window-list/index.js @@ -0,0 +1,159 @@ +/** + * External dependencies + */ +import { throttle } from 'lodash'; + +/** + * WordPress dependencies + */ +import { useState, useLayoutEffect } from '@wordpress/element'; +import { getScrollContainer } from '@wordpress/dom'; +import { PAGEUP, PAGEDOWN, HOME, END } from '@wordpress/keycodes'; + +const DEFAULT_INIT_WINDOW_SIZE = 30; + +/** + * @typedef {Object} WPFixedWindowList + * + * @property {number} visibleItems Items visible in the current viewport + * @property {number} start Start index of the window + * @property {number} end End index of the window + * @property {(index:number)=>boolean} itemInView Returns true if item is in the window + * @property {number} startPadding Padding in px to add before the start item + * @property {number} endPadding Padding in px to add after the end item + */ + +/** + * @typedef {Object} WPFixedWindowListOptions + * + * @property {number} [windowOverscan] Renders windowOverscan number of items before and after the calculated visible window. + * @property {boolean} [useWindowing] When false avoids calculating the window size + * @property {number} [initWindowSize] Initial window size to use on first render before we can calculate the window size. + */ + +/** + * + * @param {import('react').RefObject} elementRef Used to find the closest scroll container that contains element. + * @param { number } itemHeight Fixed item height in pixels + * @param { number } totalItems Total items in list + * @param { WPFixedWindowListOptions } [options] Options object + * @return {[ WPFixedWindowList, setFixedListWindow:(nextWindow:WPFixedWindowList)=>void]} Array with the fixed window list and setter + */ +export default function useFixedWindowList( + elementRef, + itemHeight, + totalItems, + options +) { + const initWindowSize = options?.initWindowSize ?? DEFAULT_INIT_WINDOW_SIZE; + const useWindowing = options?.useWindowing ?? true; + + const [ fixedListWindow, setFixedListWindow ] = useState( { + visibleItems: initWindowSize, + start: 0, + end: initWindowSize, + itemInView: ( /** @type {number} */ index ) => { + return index >= 0 && index <= initWindowSize; + }, + startPadding: 0, + endPadding: 0, + } ); + + useLayoutEffect( () => { + if ( ! useWindowing ) { + return; + } + const scrollContainer = getScrollContainer( elementRef.current ); + const measureWindow = () => { + if ( ! scrollContainer ) { + return; + } + const visibleItems = Math.ceil( + scrollContainer.clientHeight / itemHeight + ); + const windowOverscan = options?.windowOverscan ?? visibleItems; + const start = Math.max( + 0, + Math.floor( scrollContainer.scrollTop / itemHeight ) - + windowOverscan + ); + const end = Math.min( + totalItems - 1, + start + visibleItems + windowOverscan + ); + setFixedListWindow( { + visibleItems, + start, + end, + itemInView: ( index ) => { + return start <= index && index <= end; + }, + startPadding: itemHeight * start, + endPadding: + totalItems > end + ? itemHeight * ( totalItems - end - 1 ) + : 0, + } ); + }; + const handleKeyDown = ( /** @type {KeyboardEvent} */ event ) => { + switch ( event.keyCode ) { + case HOME: { + return scrollContainer?.scrollTo( { top: 0 } ); + } + case END: { + return scrollContainer?.scrollTo( { + top: totalItems * itemHeight, + } ); + } + case PAGEUP: { + return scrollContainer?.scrollTo( { + top: + scrollContainer.scrollTop - + fixedListWindow.visibleItems * itemHeight, + } ); + } + case PAGEDOWN: { + return scrollContainer?.scrollTo( { + top: + scrollContainer.scrollTop + + fixedListWindow.visibleItems * itemHeight, + } ); + } + } + }; + + measureWindow(); + const throttleMeasureList = throttle( () => { + measureWindow(); + }, 16 ); + scrollContainer?.addEventListener( 'scroll', throttleMeasureList ); + scrollContainer?.ownerDocument?.defaultView?.addEventListener( + 'resize', + throttleMeasureList + ); + scrollContainer?.ownerDocument?.defaultView?.addEventListener( + 'resize', + throttleMeasureList + ); + scrollContainer?.ownerDocument?.defaultView?.addEventListener( + 'keydown', + handleKeyDown + ); + return () => { + scrollContainer?.removeEventListener( + 'scroll', + throttleMeasureList + ); + scrollContainer?.ownerDocument?.defaultView?.removeEventListener( + 'resize', + throttleMeasureList + ); + scrollContainer?.ownerDocument?.defaultView?.removeEventListener( + 'keydown', + handleKeyDown + ); + }; + }, [ totalItems, itemHeight, elementRef ] ); + + return [ fixedListWindow, setFixedListWindow ]; +} diff --git a/packages/compose/src/index.js b/packages/compose/src/index.js index 8f7a1022cf7b1..2ce3a2ab33f6f 100644 --- a/packages/compose/src/index.js +++ b/packages/compose/src/index.js @@ -37,3 +37,4 @@ export { default as useMergeRefs } from './hooks/use-merge-refs'; export { default as useRefEffect } from './hooks/use-ref-effect'; export { default as __experimentalUseDropZone } from './hooks/use-drop-zone'; export { default as useFocusableIframe } from './hooks/use-focusable-iframe'; +export { default as __experimentalUseFixedWindowList } from './hooks/use-fixed-window-list'; 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 9941fc81dfba94540b056c5233000ea1d3ccc0ab Mon Sep 17 00:00:00 2001 From: Kerry Liu Date: Fri, 22 Oct 2021 11:54:38 -0700 Subject: [PATCH 2/6] List view: avoid re-rendering so often on scroll --- .../src/components/list-view/branch.js | 7 ++-- .../src/components/list-view/index.js | 1 - .../src/hooks/use-fixed-window-list/index.js | 34 ++++++++++++------- 3 files changed, 27 insertions(+), 15 deletions(-) diff --git a/packages/block-editor/src/components/list-view/branch.js b/packages/block-editor/src/components/list-view/branch.js index 3d262910f55be..fab0ebebdc595 100644 --- a/packages/block-editor/src/components/list-view/branch.js +++ b/packages/block-editor/src/components/list-view/branch.js @@ -6,7 +6,7 @@ import { compact } from 'lodash'; /** * WordPress dependencies */ -import { Fragment } from '@wordpress/element'; +import { Fragment, memo } from '@wordpress/element'; /** * Internal dependencies @@ -46,7 +46,7 @@ const countReducer = ( expandedState, draggedClientIds ) => ( return count + 1; }; -export default function ListViewBranch( props ) { +function ListViewBranch( props ) { const { blocks, selectBlock, @@ -82,6 +82,7 @@ export default function ListViewBranch( props ) { } const usesWindowing = __experimentalPersistentListViewFeatures; + const { start, end, @@ -162,3 +163,5 @@ export default function ListViewBranch( props ) { ListViewBranch.defaultProps = { selectBlock: () => {}, }; + +export default memo( ListViewBranch ); diff --git a/packages/block-editor/src/components/list-view/index.js b/packages/block-editor/src/components/list-view/index.js index 7f8379a53a3ac..7b56d73949181 100644 --- a/packages/block-editor/src/components/list-view/index.js +++ b/packages/block-editor/src/components/list-view/index.js @@ -107,7 +107,6 @@ function ListView( 36, visibleBlockCount, { - windowOverscan: 1, useWindowing: __experimentalPersistentListViewFeatures, } ); diff --git a/packages/compose/src/hooks/use-fixed-window-list/index.js b/packages/compose/src/hooks/use-fixed-window-list/index.js index d47b1cbf109f5..556052f0909c7 100644 --- a/packages/compose/src/hooks/use-fixed-window-list/index.js +++ b/packages/compose/src/hooks/use-fixed-window-list/index.js @@ -81,18 +81,28 @@ export default function useFixedWindowList( totalItems - 1, start + visibleItems + windowOverscan ); - setFixedListWindow( { - visibleItems, - start, - end, - itemInView: ( index ) => { - return start <= index && index <= end; - }, - startPadding: itemHeight * start, - endPadding: - totalItems > end - ? itemHeight * ( totalItems - end - 1 ) - : 0, + setFixedListWindow( ( lastWindow ) => { + const nextWindow = { + visibleItems, + start, + end, + itemInView: ( /** @type {number} */ index ) => { + return start <= index && index <= end; + }, + startPadding: itemHeight * start, + endPadding: + totalItems > end + ? itemHeight * ( totalItems - end - 1 ) + : 0, + }; + if ( + lastWindow.start !== nextWindow.start || + lastWindow.end !== nextWindow.end || + lastWindow.visibleItems !== nextWindow.visibleItems + ) { + return nextWindow; + } + return lastWindow; } ); }; const handleKeyDown = ( /** @type {KeyboardEvent} */ event ) => { From abcab5d657c5b1dd14289b52d7cb1be7bb8393fd Mon Sep 17 00:00:00 2001 From: Kerry Liu Date: Fri, 22 Oct 2021 13:40:48 -0700 Subject: [PATCH 3/6] List View: use tr placeholders vs padding --- .../src/components/list-view/block.js | 2 - .../src/components/list-view/branch.js | 164 ++++++++---------- .../src/components/list-view/index.js | 5 - .../src/components/list-view/style.scss | 6 + .../src/hooks/use-fixed-window-list/index.js | 18 +- 5 files changed, 84 insertions(+), 111 deletions(-) diff --git a/packages/block-editor/src/components/list-view/block.js b/packages/block-editor/src/components/list-view/block.js index c4987d59ee83f..97be704702f6e 100644 --- a/packages/block-editor/src/components/list-view/block.js +++ b/packages/block-editor/src/components/list-view/block.js @@ -39,7 +39,6 @@ export default function ListViewBlock( { showBlockMovers, path, isExpanded, - style, } ) { const cellRef = useRef( null ); const [ isHovered, setIsHovered ] = useState( false ); @@ -185,7 +184,6 @@ export default function ListViewBlock( { className="block-editor-list-view-block__contents-cell" colSpan={ colSpan } ref={ cellRef } - style={ style } > { ( { ref, tabIndex, onFocus } ) => (
diff --git a/packages/block-editor/src/components/list-view/branch.js b/packages/block-editor/src/components/list-view/branch.js index fab0ebebdc595..20f4c815c583e 100644 --- a/packages/block-editor/src/components/list-view/branch.js +++ b/packages/block-editor/src/components/list-view/branch.js @@ -68,96 +68,80 @@ function ListViewBranch( props ) { const blockCount = filteredBlocks.length; let nextPosition = listPosition; - const listItems = []; - for ( let index = 0; index < filteredBlocks.length; index++ ) { - const block = filteredBlocks[ index ]; - const { clientId, innerBlocks } = block; - - if ( index > 0 ) { - nextPosition += countBlocks( - filteredBlocks[ index - 1 ], - expandedState, - draggedClientIds - ); - } - - const usesWindowing = __experimentalPersistentListViewFeatures; - - const { - start, - end, - itemInView, - startPadding, - endPadding, - } = fixedListWindow; - - const blockInView = ! usesWindowing || itemInView( nextPosition ); - - const isDragging = draggedClientIds?.length > 0; - if ( - usesWindowing && - ! isDragging && - ! blockInView && - nextPosition > start - ) { - // found the end of the window, don't bother processing the rest of the items - break; - } - const style = usesWindowing - ? { - paddingTop: start === nextPosition ? startPadding : 0, - paddingBottom: end === nextPosition ? endPadding : 0, - } - : undefined; - - const position = index + 1; - const updatedPath = - path.length > 0 ? `${ path }_${ position }` : `${ position }`; - const hasNestedBlocks = - showNestedBlocks && !! innerBlocks && !! innerBlocks.length; - - const isExpanded = hasNestedBlocks - ? expandedState[ clientId ] ?? true - : undefined; - - // Make updates to the selected or dragged blocks synchronous, - // but asynchronous for any other block. - const isDragged = !! draggedClientIds?.includes( clientId ); - - listItems.push( - - { ( isDragged || blockInView ) && ( - - ) } - { hasNestedBlocks && isExpanded && ! isDragged && ( - - ) } - - ); - } - return <>{ listItems }; + return ( + <> + { filteredBlocks.map( ( block, index ) => { + const { clientId, innerBlocks } = block; + + if ( index > 0 ) { + nextPosition += countBlocks( + filteredBlocks[ index - 1 ], + expandedState, + draggedClientIds + ); + } + + const usesWindowing = __experimentalPersistentListViewFeatures; + + const { itemInView } = fixedListWindow; + + const blockInView = + ! usesWindowing || itemInView( nextPosition ); + + const position = index + 1; + const updatedPath = + path.length > 0 + ? `${ path }_${ position }` + : `${ position }`; + const hasNestedBlocks = + showNestedBlocks && !! innerBlocks && !! innerBlocks.length; + + const isExpanded = hasNestedBlocks + ? expandedState[ clientId ] ?? true + : undefined; + + const isDragged = !! draggedClientIds?.includes( clientId ); + + const showBlock = isDragged || blockInView; + return ( + + { showBlock && ( + + ) } + { ! showBlock && ( + + + + ) } + { hasNestedBlocks && isExpanded && ! isDragged && ( + + ) } + + ); + } ) } + + ); } ListViewBranch.defaultProps = { diff --git a/packages/block-editor/src/components/list-view/index.js b/packages/block-editor/src/components/list-view/index.js index 7b56d73949181..83cb1be42bd90 100644 --- a/packages/block-editor/src/components/list-view/index.js +++ b/packages/block-editor/src/components/list-view/index.js @@ -177,11 +177,6 @@ function ListView( ref={ treeGridRef } onCollapseRow={ collapseRow } onExpandRow={ expandRow } - aria-rowcount={ - __experimentalPersistentListViewFeatures - ? visibleBlockCount - : undefined - } > boolean} itemInView Returns true if item is in the window - * @property {number} startPadding Padding in px to add before the start item - * @property {number} endPadding Padding in px to add after the end item */ /** @@ -55,8 +53,6 @@ export default function useFixedWindowList( itemInView: ( /** @type {number} */ index ) => { return index >= 0 && index <= initWindowSize; }, - startPadding: 0, - endPadding: 0, } ); useLayoutEffect( () => { @@ -72,14 +68,13 @@ export default function useFixedWindowList( scrollContainer.clientHeight / itemHeight ); const windowOverscan = options?.windowOverscan ?? visibleItems; - const start = Math.max( - 0, - Math.floor( scrollContainer.scrollTop / itemHeight ) - - windowOverscan + const firstViewableIndex = Math.floor( + scrollContainer.scrollTop / itemHeight ); + const start = Math.max( 0, firstViewableIndex - windowOverscan ); const end = Math.min( totalItems - 1, - start + visibleItems + windowOverscan + firstViewableIndex + visibleItems + windowOverscan ); setFixedListWindow( ( lastWindow ) => { const nextWindow = { @@ -89,11 +84,6 @@ export default function useFixedWindowList( itemInView: ( /** @type {number} */ index ) => { return start <= index && index <= end; }, - startPadding: itemHeight * start, - endPadding: - totalItems > end - ? itemHeight * ( totalItems - end - 1 ) - : 0, }; if ( lastWindow.start !== nextWindow.start || From 5e2eaca34901336157605652b4c36e47409f6f6b Mon Sep 17 00:00:00 2001 From: Kerry Liu Date: Mon, 25 Oct 2021 09:53:39 -0700 Subject: [PATCH 4/6] Block Editor: add changelog notes for list view performance improvement --- packages/block-editor/CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/block-editor/CHANGELOG.md b/packages/block-editor/CHANGELOG.md index 8b280692ac135..cd9efdb147261 100644 --- a/packages/block-editor/CHANGELOG.md +++ b/packages/block-editor/CHANGELOG.md @@ -4,7 +4,8 @@ ### Performance -- Avoid re-rendering all List View items on block focus [#35706](https://github.com/WordPress/gutenberg/pull/35706). These changes speed up block focus time in large posts by 80% when List View is open. +- Avoid re-rendering all List View items on block focus [#35706](https://github.com/WordPress/gutenberg/pull/35706). When List View is open Block focus time is 4 times faster in large posts. +- Render fixed number of items in List View [#35706](https://github.com/WordPress/gutenberg/pull/35230). Opening List View is 13 times faster in large posts. ### Breaking change From e640095270452677840966a7822fb8b0c82264e9 Mon Sep 17 00:00:00 2001 From: Kerry Liu Date: Mon, 25 Oct 2021 10:01:46 -0700 Subject: [PATCH 5/6] Remove changes forcing list view open during typing and focus performance tests --- .../e2e-tests/specs/performance/post-editor.test.js | 6 ++---- .../e2e-tests/specs/performance/site-editor.test.js | 10 ++-------- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/packages/e2e-tests/specs/performance/post-editor.test.js b/packages/e2e-tests/specs/performance/post-editor.test.js index fa1893923a29f..014bb28f79e70 100644 --- a/packages/e2e-tests/specs/performance/post-editor.test.js +++ b/packages/e2e-tests/specs/performance/post-editor.test.js @@ -11,6 +11,7 @@ import { sum } from 'lodash'; import { createNewPost, saveDraft, + insertBlock, openGlobalBlockInserter, closeGlobalBlockInserter, openListView, @@ -120,9 +121,7 @@ describe( 'Post Editor Performance', () => { it( 'Typing', async () => { // Measuring typing performance - await openListView(); - await page.click( '.edit-post-visual-editor__post-title-wrapper' ); - await page.keyboard.press( 'Enter' ); + await insertBlock( 'Paragraph' ); let i = 20; await page.tracing.start( { path: traceFile, @@ -161,7 +160,6 @@ 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; diff --git a/packages/e2e-tests/specs/performance/site-editor.test.js b/packages/e2e-tests/specs/performance/site-editor.test.js index 0b7196fb805dc..6e3f801d5905a 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, - openListView, + insertBlock, } from '@wordpress/e2e-test-utils'; /** @@ -118,13 +118,7 @@ describe( 'Site Editor Performance', () => { await canvas().click( '[data-type="core/post-content"] [data-type="core/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' ); + await insertBlock( 'Paragraph' ); i = 200; const traceFile = __dirname + '/trace.json'; await page.tracing.start( { From c00958567c89893d9c98268f6e6f02cd6bc390c6 Mon Sep 17 00:00:00 2001 From: Kerry Liu Date: Wed, 27 Oct 2021 09:24:26 -0700 Subject: [PATCH 6/6] List View: Add jsdoc for countBlocks and comment about 36px item height --- .../block-editor/src/components/list-view/branch.js | 12 ++++++++++++ .../block-editor/src/components/list-view/index.js | 3 +++ .../block-editor/src/components/list-view/style.scss | 3 +++ 3 files changed, 18 insertions(+) diff --git a/packages/block-editor/src/components/list-view/branch.js b/packages/block-editor/src/components/list-view/branch.js index 20f4c815c583e..fd81a2ad98f7d 100644 --- a/packages/block-editor/src/components/list-view/branch.js +++ b/packages/block-editor/src/components/list-view/branch.js @@ -14,6 +14,18 @@ import { Fragment, memo } from '@wordpress/element'; import ListViewBlock from './block'; import { useListViewContext } from './context'; +/** + * Given a block, returns the total number of blocks in that subtree. This is used to help determine + * the list position of a block. + * + * When a block is collapsed, we do not count their children as part of that total. In the current drag + * implementation dragged blocks and their children are not counted. + * + * @param {Object} block block tree + * @param {Object} expandedState state that notes which branches are collapsed + * @param {Array} draggedClientIds a list of dragged client ids + * @return {number} block count + */ function countBlocks( block, expandedState, draggedClientIds ) { const isDragged = draggedClientIds?.includes( block.clientId ); if ( isDragged ) { diff --git a/packages/block-editor/src/components/list-view/index.js b/packages/block-editor/src/components/list-view/index.js index 83cb1be42bd90..3f07ff3316f10 100644 --- a/packages/block-editor/src/components/list-view/index.js +++ b/packages/block-editor/src/components/list-view/index.js @@ -102,6 +102,9 @@ function ListView( isMounted.current = true; }, [] ); + // List View renders a fixed number of items and relies on each having a fixed item height of 36px. + // If this value changes, we should also change the itemHeight value set in useFixedWindowList. + // See: https://github.com/WordPress/gutenberg/pull/35230 for additional context. const [ fixedListWindow ] = useFixedWindowList( elementRef, 36, diff --git a/packages/block-editor/src/components/list-view/style.scss b/packages/block-editor/src/components/list-view/style.scss index 0e5d1b5737030..9b1cb8b88229a 100644 --- a/packages/block-editor/src/components/list-view/style.scss +++ b/packages/block-editor/src/components/list-view/style.scss @@ -62,6 +62,9 @@ display: none; } + // List View renders a fixed number of items and relies on each item having a fixed height of 36px. + // If this value changes, we should also change the itemHeight value set in useFixedWindowList. + // See: https://github.com/WordPress/gutenberg/pull/35230 for additional context. .block-editor-list-view-block-contents { display: flex; align-items: center;