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.

Sign up for GitHub

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

Jump to bottom

Move the visible blocks state to the block editor store #41104

Merged
merged 4 commits into from
May 19, 2022
Merged

Move the visible blocks state to the block editor store #41104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e29d676
Only show the inserter for blocks that are actually visible on the sc…
youknowriad May 17, 2022
d3b9ebe
try improve performance
youknowriad May 17, 2022
e9dfb4d
Fix automattic change undo
youknowriad May 17, 2022
e9125e7
Simplify block visibility updates
youknowriad May 18, 2022
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
21 changes: 21 additions & 0 deletions docs/reference-guides/data/data-core-block-editor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -976,6 +976,19 @@ _Returns_

- `boolean`: Is Valid.

### isBlockVisible

Tells if the block is visible on the canvas or not.

_Parameters_

- _state_ `Object`: Global application state.
- _clientId_ `Object`: Client Id of the block.

_Returns_

- `boolean`: True if the block is visible.

### isBlockWithinSelection

Returns true if the block corresponding to the specified client ID is
Expand Down Expand Up @@ -1456,6 +1469,14 @@ _Parameters_

- _hasBlockMovingClientId_ `string|null`: Enable/Disable block moving mode.

### setBlockVisibility

Action that sets whether a block has controlled inner blocks.

_Parameters_

- _updates_ `Record<string,boolean>`: The block's clientId.

### setHasControlledInnerBlocks

Action that sets whether a block has controlled inner blocks.
Expand Down
88 changes: 44 additions & 44 deletions packages/block-editor/src/components/block-list/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { AsyncModeProvider, useSelect } from '@wordpress/data';
import { AsyncModeProvider, useSelect, useDispatch } from '@wordpress/data';
import { useViewportMatch, useMergeRefs } from '@wordpress/compose';
import { createContext, useState, useMemo } from '@wordpress/element';

Expand Down Expand Up @@ -48,6 +48,23 @@ function Root( { className, ...settings } ) {
},
[]
);
const { setBlockVisibility } = useDispatch( blockEditorStore );
const intersectionObserver = useMemo( () => {
const { IntersectionObserver: Observer } = window;

if ( ! Observer ) {
return;
}

return new Observer( ( entries ) => {
const updates = {};
for ( const entry of entries ) {
const clientId = entry.target.getAttribute( 'data-block' );
updates[ clientId ] = entry.isIntersecting;
}
setBlockVisibility( updates );
} );
}, [] );
const innerBlocksProps = useInnerBlocksProps(
{
ref: useMergeRefs( [
Expand All @@ -65,7 +82,9 @@ function Root( { className, ...settings } ) {
);
return (
<elementContext.Provider value={ element }>
<div { ...innerBlocksProps } />
<IntersectionObserver.Provider value={ intersectionObserver }>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved the "provider" to "root" because there's no need to have a different intersection observer per "inner blocks" container. One for the whole editor is sufficient.

<div { ...innerBlocksProps } />
</IntersectionObserver.Provider>
</elementContext.Provider>
);
}
Expand All @@ -90,59 +109,40 @@ function Items( {
__experimentalAppenderTagName,
__experimentalLayout: layout = defaultLayout,
} ) {
const [ intersectingBlocks, setIntersectingBlocks ] = useState( new Set() );
const intersectionObserver = useMemo( () => {
const { IntersectionObserver: Observer } = window;

if ( ! Observer ) {
return;
}

return new Observer( ( entries ) => {
setIntersectingBlocks( ( oldIntersectingBlocks ) => {
const newIntersectingBlocks = new Set( oldIntersectingBlocks );
for ( const entry of entries ) {
const clientId = entry.target.getAttribute( 'data-block' );
const action = entry.isIntersecting ? 'add' : 'delete';
newIntersectingBlocks[ action ]( clientId );
}
return newIntersectingBlocks;
} );
} );
}, [ setIntersectingBlocks ] );
const { order, selectedBlocks } = useSelect(
const { order, selectedBlocks, visibleBlocks } = useSelect(
( select ) => {
const { getBlockOrder, getSelectedBlockClientIds } = select(
blockEditorStore
);
const {
getBlockOrder,
getSelectedBlockClientIds,
__unstableGetVisibleBlocks,
} = select( blockEditorStore );
return {
order: getBlockOrder( rootClientId ),
selectedBlocks: getSelectedBlockClientIds(),
visibleBlocks: __unstableGetVisibleBlocks(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There could be a possible optimization opportunity here: visibleBlocks cares only about blocks that are top-level, because that's where async mode is being set. Nested blocks' visibility is redundant. But __unstableGetVisibleBlocks will return all visible blocks, causing wasted rerenders. Happens commonly when I, e.g., scroll away only a part of a complex site header structure. The top-level block remains visible, but some deeply nested blocks become invisible:

Screenshot 2022-09-12 at 13 16 06

Not sure how impactful this really is -- just noting down something I observed when working on #42525 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm misunderstanding, I think we do care about the nested blocks here imagine a big group block containing a high number of paragraphs... The paragraphs need to be considered as "hidden" even if they're nested and part of the group is visible because otherwise, it will freeze the editor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I think it doesn't work. Would need to verify carefully to be 100% sure, but only the top-level BlockListItems component (in @wordpress/block-editor) ever renders <AsyncModeProvider>, switching top-level blocks between sync and async as they become visible or invisible.

If there is a group block with many paragraphs, then these paragraph blocks have correct isBlockVisible state in the store, but the sync/async mode switching doesn't happen, because they're nested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AsyncModeProvider is rendered around each block

gutenberg/packages/block-editor/src/components/block-list/index.js

Line 130 in c1cf175

<AsyncModeProvider

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out I misunderstood how nested blocks are rendered! I thought that the BlockListItems component with all the async providers is rendered only at the root, but it's used by every block that supports nested blocks. That ensures that the sync/async switching works at all levels.

Still, __unstableGetVisibleBlocks could be optimized a bit? Instead of returning all visible blocks, it could take a rootClientId argument and return only immediate children of a given parent block. Because now, whenever there's a visibility change at any level, all BlockListItems instances are rerendered. And there are 24 instances when I edit the main template in Twenty Twenty-Two. With __unstableGetVisibleBlocks( rootClientId ), only the one affected BlockListItems instance would rerender.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all BlockListItems instances are rerendered. And there are 24 instances when I edit the main template in Twenty Twenty-Two. With __unstableGetVisibleBlocks( rootClientId ), only the one affected BlockListItems instance would rerender.

Yes, that sounds like something we could try. That said, It's not always obvious what's more performant: a more complex selector or a loop that does not much (BlockListBlock components are memoized).

};
},
[ rootClientId ]
);

return (
<LayoutProvider value={ layout }>
<IntersectionObserver.Provider value={ intersectionObserver }>
{ order.map( ( clientId ) => (
<AsyncModeProvider
key={ clientId }
value={
// Only provide data asynchronously if the block is
// not visible and not selected.
! intersectingBlocks.has( clientId ) &&
! selectedBlocks.includes( clientId )
}
>
<BlockListBlock
rootClientId={ rootClientId }
clientId={ clientId }
/>
</AsyncModeProvider>
) ) }
</IntersectionObserver.Provider>
{ order.map( ( clientId ) => (
<AsyncModeProvider
key={ clientId }
value={
// Only provide data asynchronously if the block is
// not visible and not selected.
! visibleBlocks.has( clientId ) &&
! selectedBlocks.includes( clientId )
}
>
<BlockListBlock
rootClientId={ rootClientId }
clientId={ clientId }
/>
</AsyncModeProvider>
) ) }
{ order.length < 1 && placeholder }
<BlockListAppender
tagName={ __experimentalAppenderTagName }
Expand Down
19 changes: 12 additions & 7 deletions packages/block-editor/src/components/block-popover/inbetween.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,23 @@ function BlockPopoverInbetween( {
__unstableContentRef,
...props
} ) {
const { orientation, rootClientId } = useSelect(
const { orientation, rootClientId, isVisible } = useSelect(
( select ) => {
const { getBlockListSettings, getBlockRootClientId } = select(
blockEditorStore
);
const {
getBlockListSettings,
getBlockRootClientId,
isBlockVisible,
} = select( blockEditorStore );

const _rootClientId = getBlockRootClientId( previousClientId );
return {
orientation:
getBlockListSettings( _rootClientId )?.orientation ||
'vertical',
rootClientId: _rootClientId,
isVisible:
isBlockVisible( previousClientId ) &&
isBlockVisible( nextClientId ),
};
},
[ previousClientId ]
Expand All @@ -48,7 +53,7 @@ function BlockPopoverInbetween( {
const nextElement = useBlockElement( nextClientId );
const isVertical = orientation === 'vertical';
const style = useMemo( () => {
if ( ! previousElement && ! nextElement ) {
if ( ( ! previousElement && ! nextElement ) || ! isVisible ) {
return {};
}

Expand Down Expand Up @@ -87,7 +92,7 @@ function BlockPopoverInbetween( {
}, [ previousElement, nextElement, isVertical ] );

const getAnchorRect = useCallback( () => {
if ( ! previousElement && ! nextElement ) {
if ( ( ! previousElement && ! nextElement ) || ! isVisible ) {
return {};
}

Expand Down Expand Up @@ -141,7 +146,7 @@ function BlockPopoverInbetween( {

const popoverScrollRef = usePopoverScroll( __unstableContentRef );

if ( ! previousElement || ! nextElement ) {
if ( ! previousElement || ! nextElement || ! isVisible ) {
return null;
}

Expand Down
12 changes: 12 additions & 0 deletions packages/block-editor/src/store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1603,3 +1603,15 @@ export function setHasControlledInnerBlocks(
clientId,
};
}

/**
* Action that sets whether a block has controlled inner blocks.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

-Action that sets whether a block has controlled inner blocks.
+ Action that sets whether a block is visible on the canvas.

*
* @param {Record<string,boolean>} updates The block's clientId.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a mismatch between the English copy, which assumes a single block — thus suggesting an argument list like ( clientId, isVisible ) — and the type of updates — which suggests batching.

From the rest of the PR, I would guess that the batching is preferred, and so the fix would be to update the docstring:

Action that sets whether given blocks are visible on the canvas.

and the param description:

For each block's clientId, its new visibility setting.

*/
export function setBlockVisibility( updates ) {
return {
type: 'SET_BLOCK_VISIBILITY',
updates,
};
}
15 changes: 14 additions & 1 deletion packages/block-editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,6 +606,7 @@ const withBlockReset = ( reducer ) => ( state, action ) => {
order: mapBlockOrder( action.blocks ),
parents: mapBlockParents( action.blocks ),
controlledInnerBlocks: {},
visibility: {},
};

const subTree = buildBlockTree( newState, action.blocks );
Expand Down Expand Up @@ -1139,6 +1140,17 @@ export const blocks = flow(
}
return state;
},

visibility( state = {}, action ) {
if ( action.type === 'SET_BLOCK_VISIBILITY' ) {
return {
...state,
...action.updates,
};
}

return state;
},
} );

/**
Expand Down Expand Up @@ -1678,7 +1690,8 @@ export function automaticChangeStatus( state, action ) {

return;
// Undoing an automatic change should still be possible after mouse
// move.
// move or after visibility change.
case 'SET_BLOCK_VISIBILITY':
case 'START_TYPING':
case 'STOP_TYPING':
return state;
Expand Down
28 changes: 28 additions & 0 deletions packages/block-editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2651,3 +2651,31 @@ export function wasBlockJustInserted( state, clientId, source ) {
lastBlockInserted.source === source
);
}

/**
* Tells if the block is visible on the canvas or not.
*
* @param {Object} state Global application state.
* @param {Object} clientId Client Id of the block.
* @return {boolean} True if the block is visible.
*/
export function isBlockVisible( state, clientId ) {
return state.blocks.visibility?.[ clientId ] ?? true;
}

/**
* Returns the list of all hidden blocks.
*
* @param {Object} state Global application state.
* @return {[string]} List of hidden blocks.
*/
export const __unstableGetVisibleBlocks = createSelector(
( state ) => {
return new Set(
Object.keys( state.blocks.visibility ).filter(
( key ) => state.blocks.visibility[ key ]
)
);
},
( state ) => [ state.blocks.visibility ]
);
5 changes: 5 additions & 0 deletions packages/block-editor/src/store/test/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ describe( 'state', () => {
chicken: '',
},
controlledInnerBlocks: {},
visibility: {},
} );
expect( state.tree.chicken ).not.toBe(
existingState.tree.chicken
Expand Down Expand Up @@ -371,6 +372,7 @@ describe( 'state', () => {
chicken: '',
},
controlledInnerBlocks: {},
visibility: {},
} );
expect( state.tree.chicken ).not.toBe(
existingState.tree.chicken
Expand Down Expand Up @@ -519,6 +521,7 @@ describe( 'state', () => {
[ newChildBlockId3 ]: 'chicken',
},
controlledInnerBlocks: {},
visibility: {},
} );

expect( state.tree[ '' ].innerBlocks[ 0 ] ).toBe(
Expand Down Expand Up @@ -627,6 +630,7 @@ describe( 'state', () => {
[ newChildBlockId ]: 'chicken',
},
controlledInnerBlocks: {},
visibility: {},
} );

// The block object of the parent should be updated.
Expand All @@ -648,6 +652,7 @@ describe( 'state', () => {
isIgnoredChange: false,
tree: {},
controlledInnerBlocks: {},
visibility: {},
} );
} );

Expand Down