-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Refactor: useBlockTools hook #58979
Refactor: useBlockTools hook #58979
Conversation
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
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export function useHasBlockToolbar() { | |
export function useShowBlockToolbar() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above, could this determination be abstracted in tiny hook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: We can probably use getBlockSelectionStart
here if we want only the first block clientId.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up at #59450
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably fine. We can always refactor if CodeVitals catches any perf regression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow up at #59450
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?
useHasBlockToolbar
hook that has the logic for if the block-toolbar will render or not.useBlockTools
hook that privately exports which block tools will be showing (block toolbar popover, fixed block toolbar, block inserter, block breadcrumb).is-collapsed
class to the central toolbar areaTesting Instructions
Toolbar should show/hide in the same way as before.
Testing Instructions for Keyboard
is-collapsed
class