Fix group block layout performance #44103
Conversation
talldan
added
[Type] Performance
|
Size Change: +205 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
|
Thanks for looking into this @talldan! I notice that there appears to be a change with this PR applied and the rendering of flow versus constrained layout types in TwentyTwentyTwo theme with existing markup. Visually, the main place I notice the change is in the
I'm not too sure what the expected behaviour is there for legacy markup / existing templates, but just thought I'd mention it first in case it sparks any ideas 🤔 |
It could be that legacy Group block markup that was created with It sounds like the challenge here is ensuring that in legacy markup, the "default" state of the Group block was to be the |
Right. It's confusing that this works in the editor. I'm assuming this is due to the migration here: gutenberg/packages/block-library/src/group/deprecated.js Lines 112 to 127 in cb75861 This is quite confusing because this seems to bypass the setting of the default value of the attribute, basically unsetting it. The group block receives an I assume the issue that you noticed happens because there's no PHP version of this migration. It should be pretty trivial to add something that checks the |
|
It sure is a tricky one!
Similar logic to that migration exists on these lines, so that the layout type of gutenberg/lib/block-supports/layout.php Lines 296 to 299 in 93e29bb Do we need to somehow update the logic there for this case (inferring the flow/default layout type)? |
|
Thanks for working on this @talldan ! I can reproduce the issue @andrewserong noticed with the TT2 single template. The block that is wrongly getting constrained layout set in the front end is the wrapper Group with the |
| $block_gap = gutenberg_get_global_settings( array( 'spacing', 'blockGap' ) ); | ||
| $global_layout_settings = gutenberg_get_global_settings( array( 'layout' ) ); | ||
| $has_block_gap_support = isset( $block_gap ) ? null !== $block_gap : false; | ||
| $default_block_layout = _wp_array_get( $block_type->supports, array( '__experimentalLayout', 'default' ), array() ); | ||
| $used_layout = isset( $block['attrs']['layout'] ) ? $block['attrs']['layout'] : $default_block_layout; | ||
| $used_layout = isset( $attributes['layout'] ) ? $attributes['layout'] : $default_block_layout; |
There was a problem hiding this comment.
I think we need to use $parsed_block['attrs']['layout'] here instead of $attributes['layout'], because $attributes['layout'] contains the default layout as well as the specific block's layout attributes.
There was a problem hiding this comment.
Ok I see the reason for the change now - new Group blocks no longer have the layout attribute explicitly set. The problem with doing that is that now we have no way of distinguishing a new Group with default constrained layout from a legacy Group with the previous default layout.
We need this to work on the front end for all existing markup, so there has to be a way to tell these two types of Group apart. The only way I could find to do that was explicitly setting layout on all new Groups, which landed us in the present pickle 😅
I'm curious - could this bit in the Nav block also have similar consequences? We might need to find a better way of setting attributes on new blocks.
There was a problem hiding this comment.
The problem with doing that is that now we have no way of distinguishing a new Group with default constrained layout from a legacy Group with the previous default layout.
It does work in the editor though, so I'm hoping it's possible to replicate that in the PHP code.
I'm curious - could this bit in the Nav block also have similar consequences? We might need to find a better way of setting attributes on new blocks.
Possibly, I'm not sure of the absolute root cause yet, but it could do with some further investigation.
useBlockSync is the other bit of code that that results in the infinite looping. This bit here:
gutenberg/packages/block-editor/src/components/provider/use-block-sync.js
Lines 141 to 172 in 5cbfe81
When setControlledBlocks() is called on line 162, it replaces all the inner blocks, and the group block's effect runs again. But the group block's effect then causes useBlockSync to run again. And so on. I think @youknowriad knows this code best so might have some insights.
There was a problem hiding this comment.
It only seems to work in the editor when first adding the block. If you save and refresh the page, the block loses the constrained layout 😬
There was a problem hiding this comment.
The only way I could find to do that was explicitly setting layout on all new Groups, which landed us in the present pickle 😅
Just thinking about this a little more... if it winds up being too hard to infer the values from the PHP side, I suppose part of the challenge is that the concepts of a default value when inserted, and what constitutes a default value from serialized markup, are linked. I'm wondering if there could be a way (somehow) for us to explicitly set a default value when a block is inserted, that is different from what determines whether or not a value is serialized? Or to put it differently — to tease apart the two different concepts of default for attributes in block.json. That might require some API changes, though, so could be a bit of a rabbithole 😓🤔
There was a problem hiding this comment.
Do you mean setting default attributes explicitly on blocks, instead of having the "default" state be attribute-less?
There was a problem hiding this comment.
I'm going to try the alternative Riad suggested here, if it works it should solve the problem 😅
There was a problem hiding this comment.
Do you mean setting default attributes explicitly on blocks, instead of having the "default" state be attribute-less?
That's what I was idly wondering about, yeah... but 🤞 Riad's suggestion gives us a good way forward!
priethor
added
Backport to WP 6.7 Beta/RC
|
Closing in favor of #44176, as I don't think there's any way to maintain backwards compatibility with this approach. |
ockham
added
Backport to Gutenberg RC
aaronrobertshaw
removed
the
Backport to Gutenberg RC
What?
Fixes #43760
The group block seemed to be causing an infinite looping to occur when it's used inside a template part. Duplicating the template part would cause the site editor to freeze.
It's hard to ascertain exactly why it happens. The way the group block was updating an attribute in an effect seemed to be causing
useBlockSyncto trigger, which would then also trigger the effect again, ad infinitum.How?
The effect in the group block wasn't needed. It was being used to serialize the group block's default layout attribute.
After reading the comment thread here - https://github.com/WordPress/gutenberg/pull/42763/files#r968097246, I realised this was added because the default layout type attribute couldn't be read on the
$parsed_block. The problem is fixed by using theWP_Blockinstance, which applies default values via a__getmethod when reading the$block->attributesproperty.Testing Instructions
Group block
Template part block