Layout Support: Replace incremental IDs with hashes #68210

DAreRodz · 2024-12-20T18:44:20Z

What?

Changes how class names for layout containers are generated, replacing per-block, incrementally-generated IDs with hashes obtained from the layout styles definition.

Will fix #67308.

Why?

This currently affects the style of elements outside of router regions, even when navigating between pages generated with the same template, because layout class names depend on the number of blocks rendered using them (see #67826), which can vary.

How?

The PR adds a new function named gutenberg_unique_id_from_values, which basically generates a short hash from the passed array.

This function is used to generate the layout container class, receiving all the data required to generate the style output. That guarantees hashes to be unique for each layout style combination.

Similar examples can be found in the WordPress codebase already:

Testing Instructions

Create a new post.
Add four different groups to test each of the different types: flow, constrained, flex and grid. The layout settings can be found in the right-side panel under the "block" tab. Follow these steps to configure each of the types:
- Flow: disable "Inner blocks use content width".
- Constrained: enable "Inner blocks use content width".
- Flex: transform the Group into a Row or a Stack.
- Grid: transform the Group into a Grid.
For the flow and constrained types, you would have to add more settings in order to generate classes with hashes:
- Flow: add a custom block spacing in the block styles.
- Constrained: change justification to left or right.
Save and visit the post.
Ensure the div container for these groups has a class name similar to wp-container-core-group-is-layout-634f0b9d (a different hash is OK).
Edit the post and duplicate all Group elements. Save and visit the post.
Ensure the duplicated Group elements have the same hashes as the original Group elements.
Edit the post again, modifying some layout settings in the duplicated elements.
Ensure the duplicated Group elements now have different hashes.

Screenshots or screencast

Before	After
Screen.Recording.2024-12-20.at.13.45.58.mov	Screen.Recording.2024-12-20.at.13.48.13.mov

github-actions · 2024-12-20T19:06:21Z

Flaky tests detected in eb456cb.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/13332123167
📝 Reported issues:

[Flaky Test] Redirection after template creation #60678 in /test/e2e/specs/site-editor/site-editor-url-navigation.spec.js

talldan · 2024-12-23T04:11:03Z

Added some potential reviewers for when this is ready.

lib/block-supports/layout.php

gziolo · 2025-02-12T11:23:40Z

There are more places where incremental IDs are used. Example for layouts in child blocks. Should the strategy change there as well?

gutenberg/lib/block-supports/layout.php

Line 624 in 9c568ad

$container_content_class = wp_unique_prefixed_id( 'wp-container-content-' );

lib/block-supports/layout.php

DAreRodz · 2025-02-12T15:54:33Z

There are more places where incremental IDs are used. Example for layouts in child blocks. Should the strategy change there as well?

gutenberg/lib/block-supports/layout.php

Line 624 in 9c568ad

$container_content_class = wp_unique_prefixed_id( 'wp-container-content-' );

@gziolo Good catch! Yes, gutenberg_incremental_id_per_prefix and wp_unique_prefixed_id have the same problem. We should also replace wp_unique_prefixed_id calls.

Mamaduka · 2025-02-13T02:48:19Z

@westonruter, do you have any thoughts on this proposal? I remember you suggested switching to wp_unique_id, which improved performance in specific scenarios.

westonruter · 2025-02-13T03:38:55Z

@Mamaduka I fully endorse this proposal! Long ago Gutenberg was using uniqid() which was problems because the CSS on the page could not be cached since it was different with every page load (#38889). So switching to wp_unique_id() fixed that problem in that the class names were then stable across page loads. However, creating a class name out of hashed data will be even more stable since changes to the block order then won't matter. So yeah, +1 for using a hash to compute the class name.

Related, the Embed Optimizer plugin in Performance Lab is also using md5() in a similar way, except to create an element ID instead of a class name.

Mamaduka · 2025-02-13T06:43:34Z

lib/block-supports/layout.php

+ *
+ * @return string The generated unique ID for the array.
+ */
+function gutenberg_unique_prefixed_id_from_array( array $data, string $prefix = '' ): string {


Nit (non-blocking): I think we could use a more generic name here, wp_unique_id_from_values. Since the prefix is optional, we could also remove it.

I see the name was changed to gutenberg_unique_id_from_values. @DAreRodz, can you clarify your intentions regarding the prefix?

github-actions · 2025-02-17T13:22:13Z

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: DAreRodz <darerodz@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org>
Co-authored-by: talldan <talldanwp@git.wordpress.org>
Co-authored-by: luisherranz <luisherranz@git.wordpress.org>
Co-authored-by: Gulamdastgir-Momin <gulamdastgir04@git.wordpress.org>
Co-authored-by: Mayank-Tripathi32 <mayanktripathi32@git.wordpress.org>
Co-authored-by: kmanijak <karolmanijak@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

DAreRodz · 2025-02-17T13:26:25Z

Setting this PR ready for review.

The logic for generating layout styles is complex, so I've added some test cases instead of testing the feature extensively. More test cases are welcome. 🙂

gziolo · 2025-02-17T13:34:22Z

phpunit/block-supports/layout-test.php

@@ -501,7 +501,47 @@ public function data_layout_support_flag_renders_classnames_on_wrapper() {
 						),
 					),
 				),
-				'expected_output' => '<p class="wp-container-content-1">Some text.</p>', // The generated classname number assumes `wp_unique_prefixed_id( 'wp-container-content-' )` will not have run previously in this test.
+				'expected_output' => '/<p class="wp-container-content-[a-f0-9]{8}">Some text.<\\/p>/',


md5 is predictable so it should stay the same between the test runs. What is the reasoning behind using pattern matching here? I expected the tests would ensure that these values don't accidentally change as long as params provided remain the same.

No problem. I can change that. I wasn't sure if it would be a good idea to update the tests every time the passed parameters change, but that's not going to happen often, right? 😅

PS: I copied what is being done for the duotone classes (although, in that case, the IDs might not be predictable).

I've just updated the tests to check the generated hashes with their expected values.

Cool, it seems like it's doable for the use cases in this PR. We could leave a note in the tests that the hash might change if the conditions provided get refined in the future.

phpunit/block-supports/layout-test.php

gziolo · 2025-02-18T13:40:10Z

In my opinion, this PR is shaping up nicely, and my recent comments are mostly stylistic. I don't think I have any major feedback left at this point. I would appreciate some additional review on how the hash is calculated by someone more familiar with layout processing.

gziolo · 2025-02-26T10:09:46Z

lib/block-supports/layout.php

+				$fallback_gap_value,
+				$block_spacing,
+			),
+			'wp-container-' . sanitize_title( $block['blockName'] ) . '-is-layout-'


Not a blocker: I have a general thought after looking at the generated CSS. The fact that the class name contains the name of the block might be no longer needed. It's currently a general-purpose class that groups similar styles. It might be interesting to explore in the follow-up PR whether the class name prefix can be simplified to the wp-container-block-is-layout-. That would be also similar to the child layouts and the wp-container-content- prefix.

gziolo · 2025-02-26T10:26:15Z

Overall, everything works as expected on the frontend. PHP code properly generates predictable hashes taking into account how the block is configured. I noticed that the editor uses different algorithm for generating class names. I would expect that JavaScript gets updated to mirror the changes in PHP. Example:

Markup:

<!-- wp:group {"borderColor":"contrast","layout":{"type":"constrained"}} -->
<div class="wp-block-group has-border-color has-contrast-border-color"><!-- wp:paragraph -->
<p>Group</p>
<!-- /wp:paragraph -->

<!-- wp:group {"borderColor":"accent-3","layout":{"type":"default"}} -->
<div class="wp-block-group has-border-color has-accent-3-border-color"><!-- wp:paragraph -->
<p>Nested Group</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"style":{"layout":{"columnSpan":2}}} -->
<p>Child layout</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div>
<!-- /wp:group -->

<!-- wp:group {"borderColor":"contrast","layout":{"type":"flex","flexWrap":"nowrap"}} -->
<div class="wp-block-group has-border-color has-contrast-border-color"><!-- wp:paragraph -->
<p>Row</p>
<!-- /wp:paragraph -->

<!-- wp:group {"borderColor":"accent-3","layout":{"type":"flex","flexWrap":"nowrap","justifyContent":"space-between","orientation":"horizontal"}} -->
<div class="wp-block-group has-border-color has-accent-3-border-color"><!-- wp:paragraph -->
<p>Nested Row</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"style":{"layout":{"columnSpan":2}}} -->
<p>Child layout</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div>
<!-- /wp:group -->

<!-- wp:group {"borderColor":"contrast","layout":{"type":"flex","orientation":"vertical"}} -->
<div class="wp-block-group has-border-color has-contrast-border-color"><!-- wp:paragraph -->
<p>Stack</p>
<!-- /wp:paragraph -->

<!-- wp:group {"borderColor":"accent-3","layout":{"type":"flex","flexWrap":"nowrap"}} -->
<div class="wp-block-group has-border-color has-accent-3-border-color"><!-- wp:paragraph -->
<p>Nested Row</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"style":{"layout":{"columnSpan":2}}} -->
<p>Child layout</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div>
<!-- /wp:group -->

<!-- wp:group {"borderColor":"contrast","layout":{"type":"grid"}} -->
<div class="wp-block-group has-border-color has-contrast-border-color"><!-- wp:paragraph -->
<p>Grid</p>
<!-- /wp:paragraph -->

<!-- wp:group {"borderColor":"accent-3","layout":{"type":"grid","columnCount":null,"minimumColumnWidth":"23rem"}} -->
<div class="wp-block-group has-border-color has-accent-3-border-color"><!-- wp:paragraph -->
<p>Nested Grid</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"style":{"layout":{"columnSpan":2}}} -->
<p>Child layout</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->

<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->

Editor:

Frontend:

What are your thoughts on these differences wp-container-content-b5f5adbb vs wp-container-content-17?

Edit: it looks like there is also wp-elements-17 that has no styles assigned to it, which isn't present on the front end. I see a similar nuanced difference for the wp-container-core-group-is-layout-55 in the editor vs the version with hash on the front end.

fabiankaegy

From my POV this here is a great fix that works as advertised that we should get into WordPress 6.8.

I'm about to start the RC for 20.4 which is the last release to make it into 6.8 so ideally I would have wanted to merge this before then. But since it is a bugfix we should be able to still get it in after today. So I'll hold merging and wait a few more days for additional feedback 👍

gziolo · 2025-02-28T14:39:38Z

Fabian, feel free to land it if you confirmed it works correctly. We can discuss my feedback separately as the more I think about it, the less it's of an issue that editors encapsulates the styles under different class names. It doesn't impact visual aspects and developers can't target them anyway.

fabiankaegy · 2025-02-28T15:52:04Z

Fabian, feel free to land it if you confirmed it works correctly. We can discuss my feedback separately as the more I think about it, the less it's of an issue that editors encapsulates the styles under different class names. It doesn't impact visual aspects and developers can't target them anyway.

Yeah I agree we should align the class name generation in JS to match. But since they are auto generated already and users cannot target them for me the two are separate and one doesn't need to block the other :)

And yes in my testing this solves the issue it was intended to solve and I am not noticing any regressions on the sample content I have as testing data :)

DAreRodz · 2025-02-28T18:20:35Z

@fabiankaegy, feel free to merge this PR once it's ready. 😊

Mamaduka · 2025-03-03T09:32:37Z

I think aligning class name generation between JS and PHP would be a lovely addition. @DAreRodz, @gziolo, do you have time to work on this?

Considering the bug status of this PR, it should be safe to merge in release betas.

@joemcgill, @desrosj, what do you think?

gziolo · 2025-03-03T11:20:57Z

I think aligning class name generation between JS and PHP would be a lovely addition. @DAreRodz, @gziolo, do you have time to work on this?

Unfortunately, I don't have the capacity to work on it during the Beta phase.

luisherranz · 2025-03-03T12:14:42Z

Yeah I agree we should align the class name generation in JS to match. But since they are auto generated already and users cannot target them for me the two are separate and one doesn't need to block the other :)

I agree with Fabian that, although aligning JS class name generation would be a nice addition, it shouldn't block merging this PR.

Mamaduka · 2025-03-06T05:48:56Z

What are the remaining items here? Just aligning JS class name generation? Also, is the core backport PR up to date?

@desrosj, @joemcgill, what do you think about shipping this fix in WP 6.8?

DAreRodz added [Type] Bug An existing feature does not function as intended [Feature] Layout Layout block support, its UI controls, and style output. labels Dec 20, 2024

DAreRodz mentioned this pull request Dec 20, 2024

Interactivity API iteration for WordPress 6.8 #67276

Open

49 tasks

talldan requested review from talldan, ramonjd and aaronrobertshaw December 23, 2024 04:10

gziolo reviewed Feb 11, 2025

View reviewed changes

lib/block-supports/layout.php Show resolved Hide resolved

gziolo reviewed Feb 12, 2025

View reviewed changes

lib/block-supports/layout.php Show resolved Hide resolved

Mamaduka reviewed Feb 13, 2025

View reviewed changes

DAreRodz added 4 commits February 14, 2025 16:19

Replace incremental IDs with hashes

2a8e575

Rename function

c2c56f7

Use function to generate container content class

5f6b21b

Update tests

eb456cb

DAreRodz force-pushed the update/block-supports-layout-container-class-with-hash branch from eb70891 to eb456cb Compare February 14, 2025 15:19

DAreRodz added 2 commits February 14, 2025 17:22

Add more test cases

406ee24

Add more test cases

7a90b32

DAreRodz marked this pull request as ready for review February 17, 2025 13:22

DAreRodz requested a review from tellthemachines as a code owner February 17, 2025 13:22

DAreRodz requested review from westonruter, gziolo and Mamaduka February 17, 2025 13:22

gziolo reviewed Feb 17, 2025

View reviewed changes

Update tests to check actual hashes

01e9549

DAreRodz requested a review from gziolo February 18, 2025 12:44

gziolo reviewed Feb 18, 2025

View reviewed changes

phpunit/block-supports/layout-test.php Outdated Show resolved Hide resolved

DAreRodz added 2 commits February 19, 2025 11:23

Fix PHP lint issue

dc4046b

Test the whole class instead of just the hash code

ed07a17

DAreRodz added a commit to DAreRodz/wordpress-develop that referenced this pull request Feb 19, 2025

Backport changes from WordPress/gutenberg#68210

b92477d

DAreRodz mentioned this pull request Feb 19, 2025

Layout Support: Replace incremental IDs with hashes WordPress/wordpress-develop#8353

Draft

DAreRodz added 2 commits February 19, 2025 12:37

Add backport-changelog file

73b22bd

Fix WP version for the backport-changelog file

99c021b

DAreRodz requested a review from luisherranz February 19, 2025 12:32

gziolo reviewed Feb 26, 2025

View reviewed changes

fabiankaegy approved these changes Feb 28, 2025

View reviewed changes

luisherranz mentioned this pull request Mar 11, 2025

Layout styles are incompatible with Interactivity API client-side navigation #67308

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Layout Support: Replace incremental IDs with hashes #68210

Layout Support: Replace incremental IDs with hashes #68210

DAreRodz commented Dec 20, 2024 •

edited by luisherranz

Loading

github-actions bot commented Dec 20, 2024 •

edited

Loading

talldan commented Dec 23, 2024

gziolo commented Feb 12, 2025

DAreRodz commented Feb 12, 2025 •

edited

Loading

Mamaduka commented Feb 13, 2025

westonruter commented Feb 13, 2025

Mamaduka Feb 13, 2025

gziolo Feb 26, 2025

github-actions bot commented Feb 17, 2025 •

edited

Loading

DAreRodz commented Feb 17, 2025

gziolo Feb 17, 2025

DAreRodz Feb 18, 2025

DAreRodz Feb 18, 2025

gziolo Feb 18, 2025

gziolo commented Feb 18, 2025

gziolo Feb 26, 2025 •

edited

Loading

gziolo commented Feb 26, 2025 •

edited

Loading

fabiankaegy left a comment

gziolo commented Feb 28, 2025

fabiankaegy commented Feb 28, 2025

DAreRodz commented Feb 28, 2025

Mamaduka commented Mar 3, 2025

gziolo commented Mar 3, 2025

luisherranz commented Mar 3, 2025

Mamaduka commented Mar 6, 2025

Layout Support: Replace incremental IDs with hashes #68210

Are you sure you want to change the base?

Layout Support: Replace incremental IDs with hashes #68210

Conversation

DAreRodz commented Dec 20, 2024 • edited by luisherranz Loading

What?

Why?

How?

Testing Instructions

Screenshots or screencast

github-actions bot commented Dec 20, 2024 • edited Loading

talldan commented Dec 23, 2024

gziolo commented Feb 12, 2025

DAreRodz commented Feb 12, 2025 • edited Loading

Mamaduka commented Feb 13, 2025

westonruter commented Feb 13, 2025

Mamaduka Feb 13, 2025

Choose a reason for hiding this comment

gziolo Feb 26, 2025

Choose a reason for hiding this comment

github-actions bot commented Feb 17, 2025 • edited Loading

DAreRodz commented Feb 17, 2025

gziolo Feb 17, 2025

Choose a reason for hiding this comment

DAreRodz Feb 18, 2025

Choose a reason for hiding this comment

DAreRodz Feb 18, 2025

Choose a reason for hiding this comment

gziolo Feb 18, 2025

Choose a reason for hiding this comment

gziolo commented Feb 18, 2025

gziolo Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

gziolo commented Feb 26, 2025 • edited Loading

fabiankaegy left a comment

Choose a reason for hiding this comment

gziolo commented Feb 28, 2025

fabiankaegy commented Feb 28, 2025

DAreRodz commented Feb 28, 2025

Mamaduka commented Mar 3, 2025

gziolo commented Mar 3, 2025

luisherranz commented Mar 3, 2025

Mamaduka commented Mar 6, 2025

DAreRodz commented Dec 20, 2024 •

edited by luisherranz

Loading

github-actions bot commented Dec 20, 2024 •

edited

Loading

DAreRodz commented Feb 12, 2025 •

edited

Loading

github-actions bot commented Feb 17, 2025 •

edited

Loading

gziolo Feb 26, 2025 •

edited

Loading

gziolo commented Feb 26, 2025 •

edited

Loading