Block Editor: Optimize layout style renderer subscription #55762
Conversation
Mamaduka
added
[Type] Performance
Mamaduka
requested review from
jsnajdr,
tellthemachines and
andrewserong
and removed request for
ellatrix
|
@andrewserong, @tellthemachines, I tried to be thorough when testing the changes. Do you have patterns you use for testing layout changes? Those would be helpful here. |
|
Size Change: +40 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d53b542. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6719197768
|
There was a problem hiding this comment.
This is OK and possible to merge, but I noticed quite a few non-blocking pre-existing issues. Some are just code style nits, others are more impactful.
As a followup, would it make sense to merge the withLayoutStyles and withChildLayoutStyles HOCs? Is there some common code that they both need to execute, and that could be executed just once instead of twice in the merged component?
| }; | ||
| }, [ selector, css, setStyleOverride, deleteStyleOverride ] ); | ||
|
|
||
| return <BlockListBlock { ...props } className={ className } />; |
There was a problem hiding this comment.
There's one thing about the child layout code that is buggy, or at least inefficient: if disableLayoutStyles is true, and if the block has a child layout (i.e., there are valid flexSize or selfStretch values), the corresponding CSS will be created and added to style overrides. But the element won't get the wp-container-content-n CSS class, and therefore the generated CSS won't be applied to any element, it's redundant.
The withLayoutStyles HOC is more efficient: it's doesn't even generate the CSS if layout styles are disabled. It would be nice if withChildLayoutStyles behaved the same way.
Cc @tellthemachines to verify that the argument above is correct and that there's no hidden gotcha.
There was a problem hiding this comment.
Oh that sounds like a bug, yes. The child layout CSS shouldn't be generated at all if disableLayoutStyles is true.
|
Thanks for the great feedback, @jsnajdr! I've addressed most of it, besides the #55762 (comment). I'll wait for additional details from @tellthemachines. |
Mamaduka
force-pushed
the
try/optimize-layout-styles-renderers
branch
from
f98d8ce
to
9b88482
Compare
There was a problem hiding this comment.
Changes LGTM and nothing seems broken when testing different blocks with layout ✅
As a followup, would it make sense to merge the withLayoutStyles and withChildLayoutStyles HOCs?
I don't think so because their logic has very little in common. Pretty much the only common part is the use of setStyleOverride in an effect at the end of each of the functions.
|
Thanks for testing, @tellthemachines 🙇♂️ |
* Block Editor: Optimize layout style renderer subscription * Rename components * Swap destructuring with optional chaining * Use assignment * Address selector feedback * Feedback
|
Looks like this change introduced a weird bug in the ChildLayoutControls where the effect that's only supposed to run once on mount is actually running again after a change is made. From my testing it only happens after the first change, if the block has no child layout set. It's only noticeable in the UI when trying to change the child layout to Fixed, because the input doesn't appear below. I'm not sure what in this commit could have caused this, but it's definitely here that the bug was born 😅 Edit: here's a screenshot of the weird intermediate state with "Fixed" selected and no input: 
|
|
@tellthemachines, can you share the reproduction steps? I'll look into this today. Thank you 🙇 |
|
@Mamaduka this is reproducible in 17.1.x and in trunk.
You can also compare against an install of WP 6.4 without Gutenberg as that works as expected. |
|
This is a regression due to the
Oddly, the toggle option group selected value stays the same, while the rest of the component behaves "as expected." It's probably worth investigating separately. I don't have a perfect solution besides reverting the changes, but I'll keep looking. @jsnajdr, we might want to re-evaluate similar perf optimizations and apply same fix. |
|
I'm not sure if I'll be able to look at this today, but what has been described so far sounds like some React props and data changes are not propagated correctly. A typical React bug. For example, the effect in |
|
Right, assuming that the component will only mount once isn't guaranteed in React, and we should look into a fix for the I think we should also fix the |
The effect should only run on mount, because otherwise it will be impossible to set the value to "Fixed" 😅 the problem being that, the way the controls are designed, we need to first set the toggle to "Fixed" in order to display the input where we can then add a fixed value. I guess this wouldn't be a problem if the controls were designed differently, and given there has been talk of that for a while (e.g. in #46128) this effect was seen as an acceptable hack to make things work in the meantime. |
|
Thanks for looking into this folks! #56660 partially fixes this issue (the input now displays correctly) but with the toggle not resetting from "Fixed" to "Fit" when there is no value assigned remains. Looking into that now. Update: the toggle not resetting seems to be caused by the value here always being undefined (the actual intended value of the control is being set as Second update: the above issue happens due to the effect in Third update: #56660 is now merged and I've created #56667 for the toggle issue. |
Yeah, now that I got to have a detailed look at this issue, I understand that better. 🙂
One thing that's bad about the effect is that it pollutes the undo stack with changes of the Ideally, the |
We could perhaps use |
|
I think this PR might have caused a regression in 6.5 (#60569) — it looks like this PR changed the logic so that if |
What?
This is a follow-up to #55754 (review).
Similar to #55449.
PR moves most of the logic from
withLayoutStylesandwithChildLayoutStylesinto separate components, which are only rendered when a block supports layout.Tested using @jsnajdr's debug code (c029303), the store subscriptions are reduced by 4000 on large text post - 1000 blocks, i.e., four subscription per-block.
Why?
See #54819 (comment).
Testing Instructions
Testing Instructions for Keyboard
Same.