BlockSwitcher: Defer transform calculations until the Dropdown is open #57892
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntsekouras
requested review from
mcsf,
Mamaduka,
youknowriad,
jasmussen and
richtabor
ellatrix
reviewed
Jan 16, 2024
|
Size Change: +146 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
ellatrix
reviewed
Jan 16, 2024
Mamaduka
reviewed
Jan 16, 2024
|
Yes, it would be great if you could move the blocks selector too :) |
|
Makes sense, the performance win will outweigh virtually anything in this case. Can we make it look like this? 
Note that the indentation from the left is due to the dropdown appearing from the button. I would love for us to find a way to nudge the inner padding of the block toolbar so that it can extend downwards, flush with the dark border of the block toolbar itself, like so: 
That is not something for this PR, it's not even trivial, I'm simply noting it here that it's considered. |
I can't move it down because we need to check the validity of blocks to decide if we can even render the component.
I updated not to pass the blocks, but I don't believe this makes any difference since the BlockSwitcher component needs the blocks too and they will trigger a rerender on every keystroke. Do I miss something? |
ntsekouras
commented
Jan 17, 2024
| @@ -57,10 +57,7 @@ test.describe( 'Toolbar roving tabindex', () => { | |||
| // ensures list block toolbar uses roving tabindex | |||
| await editor.insertBlock( { name: 'core/list' } ); | |||
| await page.keyboard.type( 'List' ); | |||
| await ToolbarRovingTabindexUtils.testBlockToolbarKeyboardNavigation( | |||
There was a problem hiding this comment.
It makes no sense to test list as a simple block since it has inner blocks for a while. The only reason this was passing is because List item has no transformation currently. testGroupKeyboardNavigation is handling tests for container blocks.
ntsekouras
force-pushed
the
refactor/block-switcher-to-defer-transform-calculations
branch
from
e4ff6af
to
aab7c63
Compare
|
This is what I had in mind. Example
diff --git packages/block-editor/src/components/block-switcher/index.js packages/block-editor/src/components/block-switcher/index.js
index fd341f09596..4a6644d7866 100644
--- packages/block-editor/src/components/block-switcher/index.js
+++ packages/block-editor/src/components/block-switcher/index.js
@@ -163,59 +163,63 @@ function BlockSwitcherDropdownMenuContents( {
export const BlockSwitcher = ( { clientIds } ) => {
const blockInformation = useBlockDisplayInformation( clientIds?.[ 0 ] );
- const { canRemove, hasBlockStyles, icon, blocks, invalidBlocks } =
- useSelect(
- ( select ) => {
- const {
- getBlockRootClientId,
- getBlocksByClientId,
- canRemoveBlocks,
- } = select( blockEditorStore );
- const { getBlockStyles, getBlockType } = select( blocksStore );
- const _blocks = getBlocksByClientId( clientIds );
- if (
- ! _blocks.length ||
- _blocks.some( ( block ) => ! block )
- ) {
- return { invalidBlocks: true };
- }
- const rootClientId = getBlockRootClientId( clientIds );
- const [ { name: firstBlockName } ] = _blocks;
- const _isSingleBlockSelected = _blocks.length === 1;
- let _icon;
- if ( _isSingleBlockSelected ) {
- _icon = blockInformation?.icon; // Take into account active block variations.
- } else {
- const isSelectionOfSameType =
- new Set( _blocks.map( ( { name } ) => name ) ).size ===
- 1;
- // When selection consists of blocks of multiple types, display an
- // appropriate icon to communicate the non-uniformity.
- _icon = isSelectionOfSameType
- ? getBlockType( firstBlockName )?.icon
- : copy;
- }
- return {
- canRemove: canRemoveBlocks( clientIds, rootClientId ),
- hasBlockStyles:
- _isSingleBlockSelected &&
- !! getBlockStyles( firstBlockName )?.length,
- icon: _icon,
- blocks: _blocks,
- };
- },
- [ clientIds, blockInformation?.icon ]
- );
+ const {
+ canRemove,
+ hasBlockStyles,
+ icon,
+ invalidBlocks,
+ isReusable,
+ isTemplate,
+ } = useSelect(
+ ( select ) => {
+ const {
+ getBlockRootClientId,
+ getBlocksByClientId,
+ canRemoveBlocks,
+ } = select( blockEditorStore );
+ const { getBlockStyles, getBlockType } = select( blocksStore );
+ const _blocks = getBlocksByClientId( clientIds );
+ if ( ! _blocks.length || _blocks.some( ( block ) => ! block ) ) {
+ return { invalidBlocks: true };
+ }
+ const rootClientId = getBlockRootClientId( clientIds );
+ const [ { name: firstBlockName } ] = _blocks;
+ const _isSingleBlockSelected = _blocks.length === 1;
+ let _icon;
+ if ( _isSingleBlockSelected ) {
+ _icon = blockInformation?.icon; // Take into account active block variations.
+ } else {
+ const isSelectionOfSameType =
+ new Set( _blocks.map( ( { name } ) => name ) ).size === 1;
+ // When selection consists of blocks of multiple types, display an
+ // appropriate icon to communicate the non-uniformity.
+ _icon = isSelectionOfSameType
+ ? getBlockType( firstBlockName )?.icon
+ : copy;
+ }
+ return {
+ canRemove: canRemoveBlocks( clientIds, rootClientId ),
+ hasBlockStyles:
+ _isSingleBlockSelected &&
+ !! getBlockStyles( firstBlockName )?.length,
+ icon: _icon,
+ isReusable:
+ _isSingleBlockSelected && isReusableBlock( _blocks[ 0 ] ),
+ isTemplate:
+ _isSingleBlockSelected && isTemplatePart( _blocks[ 0 ] ),
+ };
+ },
+ [ clientIds, blockInformation?.icon ]
+ );
const blockTitle = useBlockDisplayTitle( {
clientId: clientIds?.[ 0 ],
maximumLength: 35,
} );
+
if ( invalidBlocks ) {
return null;
}
- const isSingleBlock = blocks.length === 1;
- const isReusable = isSingleBlock && isReusableBlock( blocks[ 0 ] );
- const isTemplate = isSingleBlock && isTemplatePart( blocks[ 0 ] );
+
const hideDropdown = ! hasBlockStyles && ! canRemove;
if ( hideDropdown ) {
return (
@@ -238,6 +242,8 @@ export const BlockSwitcher = ( { clientIds } ) => {
</ToolbarGroup>
);
}
+
+ const isSingleBlock = clientIds.length === 1;
const blockSwitcherLabel = isSingleBlock
? blockTitle
: __( 'Multiple blocks selected' );
@@ -248,9 +254,9 @@ export const BlockSwitcher = ( { clientIds } ) => {
_n(
'Change type of %d block',
'Change type of %d blocks',
- blocks.length
+ clientIds.length
),
- blocks.length
+ clientIds.length
);
return (
<ToolbarGroup> |
Okay, I got it now. I updated per your suggestion. |
ellatrix
approved these changes
Jan 17, 2024
Mamaduka
approved these changes
Jan 17, 2024
|
Please consider that as reported at #51445 this button should not get a HTML This button is also used to communicate the block type. In fac, its accessible name is the block type. By making the button not focusable, this information is not available to keyboard users and screen reader users. Additionally:
|
|
Sorry for the confusion, @afercia. I initially thought that the dropdown would always be displayed after this PR, which would have removed the need to disabling the button. Your concerns are valid but not necessarily a blocker for this PR. Since it mainly contains performance improvement. |
ntsekouras
deleted the
refactor/block-switcher-to-defer-transform-calculations
branch
|
Btw, created a follow-up for |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR refactors the BlockSwitcher component so as to defer the expensive transform calculations. This should have some small improvements at least in some metrics like
block select, type with top toolbaretc..Why?
Currently when we render this component we calculate all the available transforms(either block or pattern) in order to decide whether to render a Dropdown menu or a simple disabled button. This is happening even if the user never clicks the button and also happens on every keystroke because we need the block information to determine the eligible transforms.
How?
With the refactor, there is a quite rare use case where a block might not have anything to show in the dropdown. In this case we show a message that there is no available transform and in core we can replicate I think only for
List itemblock. I think that's an acceptable trade-off for the performance gain we have.Additionally we still render a disabled button when a single block is selected with no styles that cannot be removed.
Notes
I refactored the tests too as most of them didn't seem to add much value.
Testing Instructions
Listblock and selecting aList itemblock.Screenshots or screencast