Fix Block Rename UI perf regression #55250
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
getdave
added
the
[Type] Regression
getdave
requested review from
draganescu,
youknowriad,
scruffian and
WunderBart
|
Waiting on Perf test results... |
|
|
Size Change: +3 B (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
|
@scruffian can you compare the output you shared with that you would get from running the same tests against |
|
Makes sense and the perf test confirms it. |
|
On trunk: |
|
We need to keep an eye on https://www.codevitals.run/project/gutenberg to see if the |
|
I also created this PR which proposes updating the docs on the |
Circling back here to confirm this PR fixed the perf regression. |
|
@getdave Today I independently discovered this issue when profiling for #54819, and I think there should be an even better fix. For a large post with approx 1000 blocks, we are currently wrapping every block with the But what if we didn't use the Alternatively, the rename UI doesn't need to be a plugin. It could be hardcoded directly into the Block Settings Menu, like, for example, |
I think ultimately, we need to better architecture all the built-in hooks we have (style, anchor, classname, rename, layout...). Most (if not all) don't need to use hooks and can be adde to the corresponding components, but if we ever do so, let's make sure we add them in an organized way :) |
I think what you suggest above is the most pragmatic option for now. This could be actioned in tandem with the follow up in #55194 to make metadata a default global attribute. |
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?
Fixes performance regression with selecting blocks identified in #54426 (comment).
Why?
Performance is good.
How?
Ensures that block renaming UI is only rendered for blocks which are seletected. Failure to do so leads to expensive slots being rendered for all blocks.
Perf tests caught this because it renders 1000 blocks 👏 👏 👏
Testing Instructions
Rename(also in list view via same option).focusminFocusandmaxFocusare all ~100ms (as per commit prior to the one which caused the regression)Also you can run the performance tests locally and compare the output on this branch vs
trunk. The command is:You should notice that the
focusmetric goes down significantly betweentrunk(regression) and this PR (proposed fix).Testing Instructions for Keyboard
Screenshots or screencast