Refactor: useBlockTools hook #58979
Refactor: useBlockTools hook #58979
Conversation
jeryj
requested review from
draganescu,
talldan,
noisysocks,
tellthemachines,
adamziel,
kevin940726 and
ellatrix
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -5 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 2f9b9c6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7904280994
|
jeryj
added
[Type] Bug
packages/block-editor/src/components/block-toolbar/use-has-block-toolbar.js
Outdated
Show resolved
Hide resolved
| * | ||
| * @return {boolean} Whether the block toolbar component will be rendered. | ||
| */ | ||
| export function useHasBlockToolbar() { |
There was a problem hiding this comment.
| export function useHasBlockToolbar() { | |
| export function useShowBlockToolbar() { |
There was a problem hiding this comment.
This isn't a check to know if it will show or not, but rather if it can be shown. Like, does the current selection have a block toolbar it can render? useShowBlockToolbar makes me think it will do something to display the toolbar.
packages/block-editor/src/components/block-tools/use-show-block-tools.js
Outdated
Show resolved
Hide resolved
| @@ -77,7 +79,12 @@ export default function HeaderEditMode() { | |||
| }, [] ); | |||
|
|
|||
| const isLargeViewport = useViewportMatch( 'medium' ); | |||
| const isTopToolbar = ! isZoomOutMode && hasFixedToolbar && isLargeViewport; | |||
| const { showFixedToolbar } = useShowBlockTools(); | |||
| const hasTopToolbar = | |||
There was a problem hiding this comment.
same comment as above, could this determination be abstracted in tiny hook?
There was a problem hiding this comment.
I refactored it further by adding ! isZoomOutMode to the showFixedToolbar check and removing hasFixedToolbar here, as that setting was already checked in showFixedToolbar. Now the hasTopToolbar check is just isLargeViewport && showFixedToolbar.
I don't think it's worth extracting that to its own hook as we can't easily share it between the edit-site and edit-post packages. Especially now that it's just an isLargeViewport check. I didn't want to put the showTopToolbar vs showFixedToolbar within the block-editor package, as it isn't supposed to have knowledge about how the block toolbar is being used outside of it.
|
I think the label you want is Backport to WP beta/RC? |
draganescu
added
Backport to WP 6.7 Beta/RC
jeryj
added
[Package] Block editor
|
Thanks @MaggieCabrera! I went ahead and removed the label because it is a larger change the bug it fixes is minor. |
| if ( | ||
| ! isToolbarEnabled || | ||
| ( ! isDefaultEditingMode && ! hasAnyBlockControls ) | ||
| ) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This is the same check that block-toolbar.js used to determine if it would early return or continue to render.
| const selectedBlockClientIds = getSelectedBlockClientIds(); | ||
| const selectedBlockClientId = selectedBlockClientIds[ 0 ]; |
There was a problem hiding this comment.
Suggestion: We can probably use getBlockSelectionStart here if we want only the first block clientId.
There was a problem hiding this comment.
I do think that would be a good change because getBlockSelectionStart() returns a state value and selectedBlockClientIds() does quite a bit more work. However, if I:
- Add three paragraphs
- click and drag from the third paragraph up to the first
getBlockSelectionStart()will return the third paragraph ID (where selection started) andgetSelectedBlockClientIds()[0]will return the first paragraph ID.
So, as it would be a functional change, I think we should do it in a follow-up.
| showEmptyBlockSideInserter: _showEmptyBlockSideInserter, | ||
| showBreadcrumb: | ||
| ! _showEmptyBlockSideInserter && maybeShowBreadcrumb, | ||
| showBlockToolbarPopover: | ||
| hasBlockToolbar && | ||
| ! getSettings().hasFixedToolbar && | ||
| ! _showEmptyBlockSideInserter && | ||
| hasSelectedBlock && | ||
| ! isEmptyDefaultBlock && | ||
| ! maybeShowBreadcrumb, | ||
| showFixedToolbar: | ||
| editorMode !== 'zoom-out' && | ||
| hasBlockToolbar && | ||
| getSettings().hasFixedToolbar, |
There was a problem hiding this comment.
This hook subscribes to four values. Three of these are only used by BlockTools and the remaining showFixedToolbar by only editor packages.
I'm unsure if moving the selection of values used only by BlockTools to this hook is worth it. cc @ellatrix
There was a problem hiding this comment.
I had concerns about that too. However, the information we need for showFixedToolbar is all right here, and this hook is already running for <BlockTools />. It doesn't need to do any additional work to set this value. I think it will end up being more performant and less code, so it's worth the tradeoff.
There was a problem hiding this comment.
This is probably fine. We can always refactor if CodeVitals catches any perf regression.
There was a problem hiding this comment.
It's fine, it's not worth trying to optimise performance here because there's only one block toolbar visible at any point in time.
There was a problem hiding this comment.
Or maybe not, looking at the comments below. Not sure if this is the cause of the regression though.
| const isEmptyDefaultBlock = | ||
| hasSelectedBlock && isUnmodifiedDefaultBlock( block ); |
There was a problem hiding this comment.
The isUnmodifiedDefaultBlock expects the block object. Even though it currently only uses attribute and name properties, I think this is more future proof.
Other changes are just minor cleanups.
jeryj
force-pushed
the
refactor/show-block-tools-hook
branch
from
82a80cc to
161454f
Compare
|
Hi there, it looks like this PR introduced a meaningful performance regression for the "block select" metric. Can we look into it? |
|
@youknowriad Sure! Thanks for flagging it. |
|
@youknowriad I'm looking at https://www.codevitals.run/project/gutenberg and I see the increased Block Select time, but I'm having a hard time tracking down what this metric is really measuring. I've been looking through the e2e performance tests and Readme. In the performance test for this branch, the one that stands out as an increase is Could you point me in the right direction for debugging this more? |
|
Yes, it's this one gutenberg/test/performance/specs/post-editor.spec.js Lines 249 to 297 in f3676dc |
|
@youknowriad @ellatrix The performance regression is because in order to know if the block toolbar will render or not, we need to rely on the This was seemingly good enough when it was only within the So, we could explore:
|
|
Why do we need to check fills? Don't we always need to render the switcher and options buttons? |
|
@ellatrix There are a few very specific instances:
#53410 explains it well. I'm not sure if those things are doing it wrong and should be fixed in those locations, or if they have valid use cases that we need to support here. |
|
Not sure I get this. To me the should always be the block icon visible? For this PR specifically, can't you check first if there is anything else to render in the toolbar before making this expensive check? If there's something to render, there's no need to do this check. |
|
The featured image replace one specifically is when the editor is in contentOnly mode, and the toolbar mostly only renders things in default editor mode. I'm not saying this is a good idea, but that's how it's set up. I'd love to get rid of the code that checks all the block tools slots for fills. It seems like a heavy solution to render one button in one mode for one block that affects the performance of everything else. |
|
Could you still do other checks first to fix the performance problem? |
|
Any follow-ups here, let's not let it slip |
|
@youknowriad - I'm feeling stuck on how to address Previously, #58979 (comment) and #53410 explain |
|
Is it possible to keep the call to |
Unfortunately not :/ The purpose of this PR was to make it possible to know if the block toolbar would render, and leaving |
|
If we address the content-locked image block toolbar offering a "replace" button, then we could go back to checking for content-locked to not render the toolbar again. This would allow us to remove the |
|
@jeryj, both links are pointing to the same pull request. |
|
To be honest, the bug fixed by this PR feels rather small while performance regression are experienced by everyone. I'm not saying we should not fix it but maybe it help prioritize things. |
Updated. Thanks for catching, @Mamaduka!
Fair. I'm putting in a PR that only checks for the "other" slot, as that's the only one used. It reduces the selecting block focus by 3-4ms. It's not a perfect code solution, but it does reduce the impact. |
|
Great thanks for the follow-ups :) |
Closes #57288 alternative to #57291 and #58947.
There are times where a block is selected but no tools are available. If we don't have any tools, we don't want to show a toolbar. This combines the logic for showing the block toolbar that was sprinkled throughout the codebase:
What?
Unify logic for which block tools will be showing so there is a source of truth for what will be showing (block toolbar popover, fixed block toolbar, block inserter, block breadcrumb).
Why?
Fixes #57288.
How?
useHasBlockToolbarhook that has the logic for if the block-toolbar will render or not.useBlockToolshook that privately exports which block tools will be showing (block toolbar popover, fixed block toolbar, block inserter, block breadcrumb).is-collapsedclass to the central toolbar areaTesting Instructions
Toolbar should show/hide in the same way as before.
Testing Instructions for Keyboard
is-collapsedclass