Skip to content
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.

Sign up for GitHub

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

Fix group block layout performance #44103

Closed
wants to merge 3 commits into from
Closed

Fix group block layout performance #44103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89ad608
Remove effect hook
talldan Sep 13, 2022
654514f
Use the WP_Block instance for accessing block attributes
talldan Sep 13, 2022
946e555
Update alot of snapshots
talldan Sep 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 19 additions & 15 deletions lib/block-supports/layout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,23 +262,27 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support
/**
* Renders the layout config to the block wrapper.
*
* @param string $block_content Rendered block content.
* @param array $block Block object.
* @return string Filtered block content.
* @param string $block_content Rendered block content.
* @param array $parsed_block The parsed block.
* @param WP_Block $block Instance of WP_Block for the block being rendered.
* @return string Filtered block content.
*/
function gutenberg_render_layout_support_flag( $block_content, $block ) {
$block_type = WP_Block_Type_Registry::get_instance()->get_registered( $block['blockName'] );
function gutenberg_render_layout_support_flag( $block_content, $parsed_block, $block ) {
$block_type = WP_Block_Type_Registry::get_instance()->get_registered( $block->name );
$support_layout = block_has_support( $block_type, array( '__experimentalLayout' ), false );

if ( ! $support_layout ) {
return $block_content;
}

// Use the WP_Block instance to access block attributes, since that class
// provides default attribute values, while the $parsed_block doesn't.
$attributes = $block->attributes;
$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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

useEffect( () => {
if ( pendingChanges.current.outgoing.includes( controlledBlocks ) ) {
// Skip block reset if the value matches expected outbound sync
// triggered by this component by a preceding change detection.
// Only skip if the value matches expectation, since a reset should
// still occur if the value is modified (not equal by reference),
// to allow that the consumer may apply modifications to reflect
// back on the editor.
if (
pendingChanges.current.outgoing[
pendingChanges.current.outgoing.length - 1
] === controlledBlocks
) {
pendingChanges.current.outgoing = [];
}
} else if ( getBlocks( clientId ) !== controlledBlocks ) {
// Reset changing value in all other cases than the sync described
// above. Since this can be reached in an update following an out-
// bound sync, unset the outbound value to avoid considering it in
// subsequent renders.
pendingChanges.current.outgoing = [];
setControlledBlocks();
if ( controlledSelection ) {
resetSelection(
controlledSelection.selectionStart,
controlledSelection.selectionEnd,
controlledSelection.initialPosition
);
}
}
}, [ controlledBlocks, clientId ] );

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😬

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😓🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean setting default attributes explicitly on blocks, instead of having the "default" state be attribute-less?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to try the alternative Riad suggested here, if it works it should solve the problem 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


if ( isset( $used_layout['inherit'] ) && $used_layout['inherit'] ) {
if ( ! $global_layout_settings ) {
Expand All @@ -288,7 +292,7 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {

$class_names = array();
$layout_definitions = _wp_array_get( $global_layout_settings, array( 'definitions' ), array() );
$block_classname = wp_get_block_default_classname( $block['blockName'] );
$block_classname = wp_get_block_default_classname( $block->name );
$container_class = wp_unique_id( 'wp-container-' );
$layout_classname = '';

Expand All @@ -309,15 +313,15 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
// removed in the 5.9 release (https://github.com/WordPress/gutenberg/issues/38719). It is
// not intended to provide an extended set of classes to match all block layout attributes
// here.
if ( ! empty( $block['attrs']['layout']['orientation'] ) ) {
$class_names[] = 'is-' . sanitize_title( $block['attrs']['layout']['orientation'] );
if ( ! empty( $attributes['layout']['orientation'] ) ) {
$class_names[] = 'is-' . sanitize_title( $attributes['layout']['orientation'] );
}

if ( ! empty( $block['attrs']['layout']['justifyContent'] ) ) {
$class_names[] = 'is-content-justification-' . sanitize_title( $block['attrs']['layout']['justifyContent'] );
if ( ! empty( $attributes['layout']['justifyContent'] ) ) {
$class_names[] = 'is-content-justification-' . sanitize_title( $attributes['layout']['justifyContent'] );
}

if ( ! empty( $block['attrs']['layout']['flexWrap'] ) && 'nowrap' === $block['attrs']['layout']['flexWrap'] ) {
if ( ! empty( $attributes['layout']['flexWrap'] ) && 'nowrap' === $attributes['layout']['flexWrap'] ) {
$class_names[] = 'is-nowrap';
}

Expand All @@ -336,7 +340,7 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
// Attribute-based Layout classnames are output in all cases.
if ( ! current_theme_supports( 'disable-layout-styles' ) ) {

$gap_value = _wp_array_get( $block, array( 'attrs', 'style', 'spacing', 'blockGap' ) );
$gap_value = _wp_array_get( $attributes, array( 'style', 'spacing', 'blockGap' ) );
// Skip if gap value contains unsupported characters.
// Regex for CSS value borrowed from `safecss_filter_attr`, and used here
// because we only want to match against the value, not the CSS attribute.
Expand All @@ -349,7 +353,7 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
}

$fallback_gap_value = _wp_array_get( $block_type->supports, array( 'spacing', 'blockGap', '__experimentalDefault' ), '0.5em' );
$block_spacing = _wp_array_get( $block, array( 'attrs', 'style', 'spacing' ), null );
$block_spacing = _wp_array_get( $attributes, array( 'style', 'spacing' ), null );

// If a block's block.json skips serialization for spacing or spacing.blockGap,
// don't apply the user-defined value to the styles.
Expand Down Expand Up @@ -384,7 +388,7 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
if ( function_exists( 'wp_render_layout_support_flag' ) ) {
remove_filter( 'render_block', 'wp_render_layout_support_flag' );
}
add_filter( 'render_block', 'gutenberg_render_layout_support_flag', 10, 2 );
add_filter( 'render_block', 'gutenberg_render_layout_support_flag', 10, 3 );

/**
* For themes without theme.json file, make sure
Expand Down
13 changes: 1 addition & 12 deletions packages/block-library/src/group/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
/**
* WordPress dependencies
*/
import { useSelect, useDispatch } from '@wordpress/data';
import { useEffect } from '@wordpress/element';
import { useSelect } from '@wordpress/data';
import {
InnerBlocks,
useBlockProps,
Expand Down Expand Up @@ -70,16 +69,6 @@ function GroupEdit( { attributes, setAttributes, clientId } ) {
}
);

const { __unstableMarkNextChangeAsNotPersistent } =
useDispatch( blockEditorStore );
const { type: layoutType = null } = layout;
useEffect( () => {
if ( layoutType ) {
__unstableMarkNextChangeAsNotPersistent();
setAttributes( { layout: { ...layout, type: layoutType } } );
}
}, [ layoutType ] );

return (
<>
<InspectorControls __experimentalGroup="advanced">
Expand Down
4 changes: 2 additions & 2 deletions packages/e2e-tests/specs/editor/plugins/__snapshots__/cpt-locking.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ exports[`cpt locking template_lock all should not error when deleting the cotent
`;

exports[`cpt locking template_lock all unlocked group should allow blocks to be moved 1`] = `
"<!-- wp:group {\\"templateLock\\":false,\\"layout\\":{\\"type\\":\\"constrained\\"}} -->
"<!-- wp:group {\\"templateLock\\":false} -->
<div class=\\"wp-block-group\\"><!-- wp:paragraph {\\"placeholder\\":\\"Add a description\\"} -->
<p>p1</p>
<!-- /wp:paragraph -->
Expand All @@ -55,7 +55,7 @@ exports[`cpt locking template_lock all unlocked group should allow blocks to be
`;

exports[`cpt locking template_lock all unlocked group should allow blocks to be removed 1`] = `
"<!-- wp:group {\\"templateLock\\":false,\\"layout\\":{\\"type\\":\\"constrained\\"}} -->
"<!-- wp:group {\\"templateLock\\":false} -->
<div class=\\"wp-block-group\\"><!-- wp:quote -->
<blockquote class=\\"wp-block-quote\\"><!-- wp:paragraph -->
<p></p>
Expand Down
10 changes: 5 additions & 5 deletions packages/e2e-tests/specs/editor/various/__snapshots__/block-grouping.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Block Grouping Group creation creates a group from multiple blocks of different types via block transforms 1`] = `
"<!-- wp:group {\\"layout\\":{\\"type\\":\\"constrained\\"}} -->
"<!-- wp:group -->
<div class=\\"wp-block-group\\"><!-- wp:heading -->
<h2>Group Heading</h2>
<!-- /wp:heading -->
Expand All @@ -17,7 +17,7 @@ exports[`Block Grouping Group creation creates a group from multiple blocks of d
`;

exports[`Block Grouping Group creation creates a group from multiple blocks of the same type via block transforms 1`] = `
"<!-- wp:group {\\"layout\\":{\\"type\\":\\"constrained\\"}} -->
"<!-- wp:group -->
<div class=\\"wp-block-group\\"><!-- wp:paragraph -->
<p>First Paragraph</p>
<!-- /wp:paragraph -->
Expand All @@ -33,7 +33,7 @@ exports[`Block Grouping Group creation creates a group from multiple blocks of t
`;

exports[`Block Grouping Group creation creates a group from multiple blocks of the same type via options toolbar 1`] = `
"<!-- wp:group {\\"layout\\":{\\"type\\":\\"constrained\\"}} -->
"<!-- wp:group -->
<div class=\\"wp-block-group\\"><!-- wp:paragraph -->
<p>First Paragraph</p>
<!-- /wp:paragraph -->
Expand All @@ -49,7 +49,7 @@ exports[`Block Grouping Group creation creates a group from multiple blocks of t
`;

exports[`Block Grouping Group creation groups and ungroups multiple blocks of different types via options toolbar 1`] = `
"<!-- wp:group {\\"layout\\":{\\"type\\":\\"constrained\\"}} -->
"<!-- wp:group -->
<div class=\\"wp-block-group\\"><!-- wp:heading -->
<h2>Group Heading</h2>
<!-- /wp:heading -->
Expand Down Expand Up @@ -79,7 +79,7 @@ exports[`Block Grouping Group creation groups and ungroups multiple blocks of di
`;

exports[`Block Grouping Preserving selected blocks attributes preserves width alignment settings of selected blocks 1`] = `
"<!-- wp:group {\\"layout\\":{\\"type\\":\\"constrained\\"},\\"align\\":\\"full\\"} -->
"<!-- wp:group {\\"align\\":\\"full\\"} -->
<div class=\\"wp-block-group alignfull\\"><!-- wp:heading -->
<h2>Group Heading</h2>
<!-- /wp:heading -->
Expand Down
2 changes: 1 addition & 1 deletion ...ages/e2e-tests/specs/editor/various/__snapshots__/block-hierarchy-navigation.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ exports[`Navigating the block hierarchy should navigate using the list view side
`;

exports[`Navigating the block hierarchy should select the wrapper div for a group 1`] = `
"<!-- wp:group {\\"layout\\":{\\"type\\":\\"constrained\\"}} -->
"<!-- wp:group -->
<div class=\\"wp-block-group\\"><!-- wp:paragraph -->
<p>just a paragraph</p>
<!-- /wp:paragraph -->
Expand Down
2 changes: 1 addition & 1 deletion packages/e2e-tests/specs/editor/various/__snapshots__/inserting-blocks.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ lines preserved[/myshortcode]
`;

exports[`Inserting blocks inserts a block in proper place after having clicked \`Browse All\` from block appender 1`] = `
"<!-- wp:group {\\"layout\\":{\\"type\\":\\"constrained\\"}} -->
"<!-- wp:group -->
<div class=\\"wp-block-group\\"><!-- wp:paragraph -->
<p>Paragraph inside group</p>
<!-- /wp:paragraph --></div>
Expand Down
2 changes: 1 addition & 1 deletion ...e2e-tests/specs/editor/various/__snapshots__/keep-styles-on-block-transforms.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ exports[`Keep styles on block transforms Should keep the font size during a tran
`;

exports[`Keep styles on block transforms Should not include styles in the group block when grouping a block 1`] = `
"<!-- wp:group {\\"layout\\":{\\"type\\":\\"constrained\\"}} -->
"<!-- wp:group -->
<div class=\\"wp-block-group\\"><!-- wp:paragraph {\\"fontSize\\":\\"large\\"} -->
<p class=\\"has-large-font-size\\">Line 1 to be made large</p>
<!-- /wp:paragraph --></div>
Expand Down
4 changes: 2 additions & 2 deletions packages/e2e-tests/specs/editor/various/__snapshots__/multi-block-selection.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ exports[`Multi-block selection should preserve dragged selection on move 1`] = `
`;

exports[`Multi-block selection should properly select multiple blocks if selected nested blocks belong to different parent 1`] = `
"<!-- wp:group {\\"layout\\":{\\"type\\":\\"constrained\\"}} -->
"<!-- wp:group -->
<div class=\\"wp-block-group\\"><!-- wp:paragraph -->
<p>first</p>
<!-- /wp:paragraph -->
Expand All @@ -261,7 +261,7 @@ exports[`Multi-block selection should properly select multiple blocks if selecte
<!-- /wp:paragraph --></div>
<!-- /wp:group -->

<!-- wp:group {\\"layout\\":{\\"type\\":\\"constrained\\"}} -->
<!-- wp:group -->
<div class=\\"wp-block-group\\"><!-- wp:paragraph -->
<p>second</p>
<!-- /wp:paragraph -->
Expand Down
4 changes: 2 additions & 2 deletions packages/e2e-tests/specs/editor/various/block-grouping.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,8 @@ describe( 'Block Grouping', () => {
await clickBlockToolbarButton( 'Options' );
await clickMenuItem( 'Group' );
expect( await getEditedPostContent() ).toMatchInlineSnapshot( `
"<!-- wp:group {\\"layout\\":{\\"type\\":\\"constrained\\"}} -->
<div class=\\"wp-block-group\\"><!-- wp:group {\\"layout\\":{\\"type\\":\\"constrained\\"}} -->
"<!-- wp:group -->
<div class=\\"wp-block-group\\"><!-- wp:group -->
<div class=\\"wp-block-group\\"><!-- wp:paragraph -->
<p>1</p>
<!-- /wp:paragraph --></div>
Expand Down
2 changes: 1 addition & 1 deletion packages/e2e-tests/specs/editor/various/multi-block-selection.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ describe( 'Multi-block selection', () => {
await page.mouse.up();
await page.keyboard.type( 'hi' );
expect( await getEditedPostContent() ).toMatchInlineSnapshot( `
"<!-- wp:group {\\"layout\\":{\\"type\\":\\"constrained\\"}} -->
"<!-- wp:group -->
<div class=\\"wp-block-group\\"><!-- wp:paragraph -->
<p>hih text in group</p>
<!-- /wp:paragraph --></div>
Expand Down
2 changes: 1 addition & 1 deletion .../editor/blocks/__snapshots__/Group-can-be-created-using-the-block-inserter-1-chromium.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<!-- wp:group {"layout":{"type":"constrained"}} -->
<!-- wp:group -->
<div class="wp-block-group"></div>
<!-- /wp:group -->
2 changes: 1 addition & 1 deletion .../editor/blocks/__snapshots__/Group-can-be-created-using-the-slash-inserter-1-chromium.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<!-- wp:group {"layout":{"type":"constrained"}} -->
<!-- wp:group -->
<div class="wp-block-group"></div>
<!-- /wp:group -->
2 changes: 1 addition & 1 deletion ...ots__/Group-can-have-other-blocks-appended-to-it-using-the-button-appender-1-chromium.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<!-- wp:group {"layout":{"type":"constrained"}} -->
<!-- wp:group -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>Group Block with a Paragraph</p>
<!-- /wp:paragraph --></div>
Expand Down
2 changes: 1 addition & 1 deletion ...pecs/editor/blocks/__snapshots__/Group-can-merge-into-group-with-Backspace-1-chromium.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<!-- wp:group {"layout":{"type":"constrained"}} -->
<!-- wp:group -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>1</p>
<!-- /wp:paragraph --></div>
Expand Down
2 changes: 1 addition & 1 deletion ...pecs/editor/blocks/__snapshots__/Group-can-merge-into-group-with-Backspace-2-chromium.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<!-- wp:group {"layout":{"type":"constrained"}} -->
<!-- wp:group -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>1</p>
<!-- /wp:paragraph -->
Expand Down