Remove showFixedToolbar from useShowBlockTools
#60717
Merged
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
jeryj
requested review from
draganescu,
talldan,
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: -23 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
jeryj
force-pushed
the
perf/move-header-toolbar
branch
from
790ba73
to
e130cfb
Compare
getdave
reviewed
Apr 17, 2024
There was a problem hiding this comment.
Thanks for the PR.
I'm finding myself looking for ways to validate the PR makes the improvement suggested in the description.
What testing methodology did you use to determine the performance has improved? I'd like to use that to verify the changes. Is that possible?
Also there are a lot of changes in this PR? Is it possible to break it down into smaller PRs and merge incrementally towards the desired behaviour? To someone without much context working in this area of the code this feels like a big leap and thus prone to unexpected regressions.
packages/edit-widgets/src/components/header/document-tools/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-toolbar/use-has-block-toolbar.js
Show resolved
Hide resolved
Mamaduka
reviewed
Apr 17, 2024
packages/edit-post/src/components/header/contextual-toolbar/index.js
Outdated
Show resolved
Hide resolved
Mamaduka
reviewed
Apr 17, 2024
Mamaduka
reviewed
Apr 17, 2024
Comment on lines
67
to
68
| const { getSelectedBlockClientIds, getBlockRootClientId } = | ||
| useSelect( blockEditorStore ); |
There was a problem hiding this comment.
The static selector getter doesn't create a store subscription. It's fine to have this separately.
There was a problem hiding this comment.
Can you explain this more? Performance around useSelect is an area I would like to get better at understanding.
There was a problem hiding this comment.
Store subscriptions can add up and affect performance, especially for active stores like core/block-editor. If the selector is only used in event callback or in effect, then it's better to use a static getter that doesn't create subscriptions.
In this case, removing this like and returning selectors from the mapSelect doesn't make a difference, and it's fine to have this getter.
// Doesn't create a sub.
const { getBlockRootClientId } = useSelect( blockEditorStore );
// Does create a sub.
const { getBlockRootClientId } = useSelect( ( select ) => select( blockEditorStore ), [] );P.S. Jarda recently wrote a great article about this subject - https://developer.wordpress.org/news/2024/03/28/how-to-work-effectively-with-the-useselect-hook/.
I ran the performance test of selecting blocks on both this branch and trunk. This branch should have a faster "focus" time.
I'll take a look and see. A lot of this work doesn't have any payoff until all of it is together, so there isn't much point in any one piece of it being merged without the other. I can at least go through and leave comments about each piece of it to make it easier to understand the changes and rationale. |
jeryj
commented
Apr 17, 2024
| }, | ||
| [ hasAnyBlockControls ] |
There was a problem hiding this comment.
Removing this dependency helps this not run as often. hasAnyBlockControls checks all slot fills on a block and is expensive.
Comment on lines
+48
to
+53
| if ( | ||
| ! isToolbarEnabled || | ||
| ( ! isDefaultEditingMode && ! hasAnyBlockControls ) | ||
| ) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This is hard to read, but has been the check for if a block toolbar should render or not for a long time. I don't want to change it in this PR.
| return ( | ||
| <NavigableToolbar | ||
| className="edit-widgets-header-toolbar" | ||
| aria-label={ __( 'Document tools' ) } | ||
| shouldUseKeyboardFocusShortcut={ ! showFixedToolbar } |
There was a problem hiding this comment.
Prevents rerender on widgets editor document tools when block is selected. Shortcut works as intended without this prop.
| @@ -117,7 +113,6 @@ function DocumentTools( { | |||
| className | |||
| ) } | |||
| aria-label={ toolbarAriaLabel } | |||
| shouldUseKeyboardFocusShortcut={ ! showFixedToolbar } | |||
There was a problem hiding this comment.
Prevents rerender on editor document tools when block is selected. Shortcut works as intended without this prop.
Comment on lines
-122
to
-129
| <> | ||
| <div | ||
| className={ classnames( | ||
| 'selected-block-tools-wrapper', | ||
| { | ||
| 'is-collapsed': | ||
| isBlockToolsCollapsed || | ||
| ! hasBlockSelection, |
There was a problem hiding this comment.
Having this element inline meant it was rerendering even when the top toolbar option was off. Moving it into a new component <ContextualToolbar /> means it won't be rerendered unless the top toolbar is on.
Comment on lines
-95
to
-100
| useEffect( () => { | ||
| // If we have a new block selection, show the block tools | ||
| if ( blockSelectionStart ) { | ||
| setIsBlockToolsCollapsed( false ); | ||
| } | ||
| }, [ blockSelectionStart ] ); |
There was a problem hiding this comment.
This useEffect was causing a rerender each time a new block was selected. It's only necessary when top toolbar is on, but the state is necessary when the document bar is showing (when editing a post template), so this has been changed to a function passed to the ContextualToolbar component.
| }; | ||
| }, [] ); | ||
|
|
||
| const isLargeViewport = useViewportMatch( 'medium' ); | ||
| const { showFixedToolbar } = useShowBlockTools(); |
There was a problem hiding this comment.
showFixedToolbar was removed from useShowBlockTools, so we need to know when to show the fixed toolbar a different way. I'll address the performance of this in a follow-up. Playing around with it in #60790
Comment on lines
+47
to
+56
| showEmptyBlockSideInserter: _showEmptyBlockSideInserter, | ||
| showBreadcrumb: | ||
| ! _showEmptyBlockSideInserter && maybeShowBreadcrumb, | ||
| showBlockToolbarPopover: | ||
| ! getSettings().hasFixedToolbar && | ||
| ! _showEmptyBlockSideInserter && | ||
| hasSelectedBlock && | ||
| ! hasMultiSelection() && | ||
| ( editorMode === 'navigation' || editorMode === 'zoom-out' ); | ||
|
|
||
| return { | ||
| showEmptyBlockSideInserter: _showEmptyBlockSideInserter, | ||
| showBreadcrumb: | ||
| ! _showEmptyBlockSideInserter && maybeShowBreadcrumb, | ||
| showBlockToolbarPopover: | ||
| hasBlockToolbar && | ||
| ! getSettings().hasFixedToolbar && | ||
| ! _showEmptyBlockSideInserter && | ||
| hasSelectedBlock && | ||
| ! isEmptyDefaultBlock && | ||
| ! maybeShowBreadcrumb, | ||
| showFixedToolbar: | ||
| editorMode !== 'zoom-out' && | ||
| hasBlockToolbar && | ||
| getSettings().hasFixedToolbar, | ||
| }; | ||
| }, | ||
| [ hasBlockToolbar ] | ||
| ); | ||
| ! isEmptyDefaultBlock && | ||
| ! maybeShowBreadcrumb, | ||
| }; |
There was a problem hiding this comment.
This file's changes are:
- Remove dependency on the slow useHasBlockToolbar
- Remove the showFixedToolbar to prevent rerenders
@jeryj, do you mean it's 10-15% faster on this branch than on the trunk? |
shouldUseKeyboardShortcut was toggling to say if the NavigableToolbar within DocumentTools should use the alt+f10 shortcut based on if a toolbar was available or not. This was causing the Header to rerender everytime, which was performance drag. Removing this means the Header won't rerender as often, and the test coverage for the alt+f10 shortcut still all works as intended, so this prop is not necessary to toggle.
…event unnecessary Header rerenders
…event unnecessary Header rerenders
scruffian
force-pushed
the
perf/move-header-toolbar
branch
from
6fdedf0
to
76532da
Compare
scruffian
reviewed
Apr 19, 2024
| { hasTopToolbar && ( | ||
| <ContextualToolbar | ||
| isCollapsed={ isBlockToolsCollapsed } | ||
| blockSelectionStart={ blockSelectionStart } |
There was a problem hiding this comment.
Since blockSelectionStart isn't used it in this component, should we move it inside ContextualToolbar?
scruffian
approved these changes
Apr 19, 2024
youknowriad
reviewed
Apr 25, 2024
| const { useHasBlockToolbar } = unlock( blockEditorPrivateApis ); | ||
|
|
||
| function ContextualToolbar( { | ||
| blockSelectionStart, |
There was a problem hiding this comment.
This prop is only used here and not in the outer component, it would have been better to move the useSelect call here
There was a problem hiding this comment.
I considered it, but thought combining the useSelects might be better performance-wise.
There was a problem hiding this comment.
It's a tradeoff but I believe unless we have measurable performance impact, it's better to favor collocating the state.
There was a problem hiding this comment.
Also this selector will not be called at all if the component is not rendered but it will always be called if it's done in the upper component.
| return ( | ||
| <> | ||
| <div | ||
| className={ classnames( 'selected-block-tools-wrapper', { |
There was a problem hiding this comment.
I know this predates this PR but this class (and the next one on this file) don't follow our CSS classnames naming guidelines.
|
I'm addressing these in #61104 |
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?
Prevent unnecessary rerenders of post editor header caused by #58979.
Why?
Trying to improve "Selecting blocks"/focus editor performance.
How?
Move the header
<BlockToolbar />and elements that relied onshowFixedToolbarto only run when the fixed toolbar is active. The post editor<Header />was rerendering every time a new block was selected because of this check.Testing Instructions
Post and site editors should work as normal with both popover toolbar and header toolbar.
Run Performance test of selecting blocks on both this branch and trunk. This branch should have a faster "focus" time.
npm run test:performance -- -g "Selecting blocks"The
focusmetric is routinely 10-15% lower on this branch than ontrunk.Using the React Profiler, check the Header does not rerender when:
Testing Instructions for Keyboard
Screenshots or screencast
React Profiler for this branch (no expensive Header rerender)
Screen.Recording.2024-04-17.at.1.28.31.PM.mov
React Profiler for
trunkwith Header rerenderScreen.Recording.2024-04-17.at.1.30.56.PM.mov