Add/render layout support updates #3976

tellthemachines · 2023-02-03T05:01:19Z

Trac ticket: https://core.trac.wordpress.org/ticket/57584

Includes changes from:

Try adding layout classnames to inner block wrapper gutenberg#44600
Layout: ensure block content is always returned as a string after processing gutenberg#45330
which change the layout render filter to add layout classes to the inner block wrapper, instead of the outer, and
Add Layout controls to children of Flex layout blocks gutenberg#45364
Layout child fixed size should not be fixed by default and should always have a value set gutenberg#46139
which change the layout render filter to allow inner blocks to receive layout classes and styles. This builds on the former changeset, which is why I thought it simpler to backport both pieces together.

I also added some tests for wp_render_layout_support_flag (from WordPress/gutenberg#47719) and updated some of the server block fixtures because the change to add classnames to the inner block wrapper caused the order in which layout classes are added to switch.

To test the child layout changes, add the following markup in the editor, save and check that child block 1 occupies all the leftover width of its container:

<!-- wp:group {"layout":{"type":"flex","flexWrap":"nowrap","verticalAlignment":"stretch"}} -->
<div class="wp-block-group"><!-- wp:paragraph {"style":{"layout":{"selfStretch":"fill"}},"backgroundColor":"luminous-vivid-amber"} -->
<p class="has-luminous-vivid-amber-background-color has-background">1 </p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"backgroundColor":"luminous-vivid-amber"} -->
<p class="has-luminous-vivid-amber-background-color has-background">2</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Mamaduka · 2023-02-06T11:31:29Z

src/wp-includes/block-supports/layout.php

+function wp_get_classnames_from_last_tag( $html ) {
+	$tags            = new WP_HTML_Tag_Processor( $html );
+	$last_classnames = '';
+
+	while ( $tags->next_tag() ) {
+		$last_classnames = $tags->get_attribute( 'class' );
+	}
+
+	return (string) $last_classnames;
+}


Suggestion: I only see a single use of this method. Maybe we should avoid introducing a public method and use WP_HTML_Tag_Processor directly.

The functions are always public in PHP, and we can only deprecate them after the release.

@Mamaduka Agreed. Can we instead inline this logic in the wp_render_layout_support_flag() function please? This means we can avoid introducing this function.

If there is eventually another change that needs something like this, we can always introduce a new function, but right now it seems unnecessary.

ntsekouras

Besides @Mamaduka 's comment about the new function vs inlining, I tested and this looks good to me. Thanks!

I've also tested the layout changes on the packages update PR and also tested the stack setting(having the updated packages) with this markup:

<!-- wp:group {"style":{"dimensions":{"minHeight":"400px"}},"layout":{"type":"flex","orientation":"vertical"}} -->
<div class="wp-block-group" style="min-height:400px"><!-- wp:heading {"style":{"layout":{"selfStretch":"fill","flexSize":null}},"backgroundColor":"light-green-cyan"} -->
<h2 class="wp-block-heading has-light-green-cyan-background-color has-background">1</h2>
<!-- /wp:heading -->

<!-- wp:paragraph {"style":{"layout":{"selfStretch":"fixed","flexSize":"123px"}},"backgroundColor":"luminous-vivid-orange"} -->
<p class="has-luminous-vivid-orange-background-color has-background">2</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->

dream-encode · 2023-02-06T17:31:08Z

tests/phpunit/tests/block-supports/layout.php

+	 */
+	public function test_layout_support_flag_renders_classnames_on_wrapper( $args, $expected_output ) {
+		$actual_output = wp_render_layout_support_flag( $args['block_content'], $args['block'] );
+		$this->assertEquals( $expected_output, $actual_output );


Suggested change

$this->assertEquals( $expected_output, $actual_output );

$this->assertSame( $expected_output, $actual_output );

dream-encode · 2023-02-06T17:33:38Z

src/wp-includes/block-supports/layout.php

+	$outer_class_names = array();
+
+	if ( $has_child_layout && ( 'fixed' === $block['attrs']['style']['layout']['selfStretch'] || 'fill' === $block['attrs']['style']['layout']['selfStretch'] ) ) {
+


Suggested change

dream-encode · 2023-02-06T17:33:52Z

src/wp-includes/block-supports/layout.php

+	if ( ! $support_layout
+	&& ! $has_child_layout ) {


Suggested change

if ( ! $support_layout

&& ! $has_child_layout ) {

if ( ! $support_layout && ! $has_child_layout ) {

dream-encode · 2023-02-06T17:34:14Z

src/wp-includes/block-supports/layout.php

+		);
+
+		$outer_class_names[] = $container_content_class;
+


Suggested change

dream-encode · 2023-02-06T17:35:05Z

src/wp-includes/block-supports/layout.php

+
+	/**
+	* The first chunk of innerContent contains the block markup up until the inner blocks start.
+	* We want to target the opening tag of the inner blocks wrapper, which is the last tag in that chunk.


Suggested change

* We want to target the opening tag of the inner blocks wrapper, which is the last tag in that chunk.

* This targets the opening tag of the inner blocks wrapper, which is the last tag in that chunk.

dream-encode · 2023-02-06T17:39:27Z

src/wp-includes/block-supports/layout.php

+ * @param string $html markup to be processed.
+ * @return string String of inner wrapper classnames.
+ */
+function wp_get_classnames_from_last_tag( $html ) {


Obligatory "Naming things is hard", but I wonder if more context in this function name is needed.

Why?

"Tag" could mean one of at least two things when it comes to WordPress: a taxonomy type or a HTML element.

@hellofromtonya @felixarntz Any thoughts on the naming here?

I think the name is okay (WordPress core already uses the word "tag" for both of the two meanings in various function names), but more importantly I think this conversation shouldn't really be relevant given @Mamaduka's feedback that we should not need to introduce this function in the first place.

felixarntz

@tellthemachines Looks almost good to go from my perspective, though I agree with @Mamaduka that we should not unnecessarily introduce a new function here when its logic is rather specific and only needed in a single place.

felixarntz · 2023-02-06T22:09:37Z

src/wp-includes/block-supports/layout.php

+function wp_get_classnames_from_last_tag( $html ) {
+	$tags            = new WP_HTML_Tag_Processor( $html );
+	$last_classnames = '';
+
+	while ( $tags->next_tag() ) {
+		$last_classnames = $tags->get_attribute( 'class' );
+	}
+
+	return (string) $last_classnames;
+}


@Mamaduka Agreed. Can we instead inline this logic in the wp_render_layout_support_flag() function please? This means we can avoid introducing this function.

If there is eventually another change that needs something like this, we can always introduce a new function, but right now it seems unnecessary.

felixarntz · 2023-02-06T22:10:34Z

src/wp-includes/block-supports/layout.php

+ * @param string $html markup to be processed.
+ * @return string String of inner wrapper classnames.
+ */
+function wp_get_classnames_from_last_tag( $html ) {


I think the name is okay (WordPress core already uses the word "tag" for both of the two meanings in various function names), but more importantly I think this conversation shouldn't really be relevant given @Mamaduka's feedback that we should not need to introduce this function in the first place.

felixarntz · 2023-02-06T22:11:20Z

src/wp-includes/block-supports/layout.php

+	* The first chunk of innerContent contains the block markup up until the inner blocks start.
+	* We want to target the opening tag of the inner blocks wrapper, which is the last tag in that chunk.
+	*/
+	$inner_content_classnames = isset( $block['innerContent'][0] ) && 'string' === gettype( $block['innerContent'][0] ) ? wp_get_classnames_from_last_tag( $block['innerContent'][0] ) : '';


See above, can we inline that logic here in a dedicated if ( isset( $block['innerContent'][0] ) && 'string' === gettype( $block['innerContent'][0] ) ) block?

tellthemachines · 2023-02-07T00:00:36Z

Thanks for the feedback folks! I've made all the requested changes, so this should be ready for another round.

felixarntz · 2023-02-07T17:09:15Z

@tellthemachines I tried to fix the merge conflict in the fixtures with latest trunk here, hopefully I did it right 🤞

If there are test failures, I'll check and can hopefully fix it. Apologies, not really familiar with the fixtures.

dream-encode

Seems all feedback was addressed. Looks good to merge.

felixarntz

Thanks @tellthemachines!

felixarntz · 2023-02-07T17:39:32Z

Noting that the test failures here are unrelated and intermittent, fixed via https://core.trac.wordpress.org/changeset/55280.

felixarntz · 2023-02-07T17:42:16Z

Committed in https://core.trac.wordpress.org/changeset/55282.

…lasses. In #45364 (WordPress/wordpress-develop#3976) the Block Supports was extended to add layout class names using the HTML API, new in WordPress 6.2. The initial patch opened up two opportunities to refine the code, however: - There are multiple instances of the `WP_HTML_Tag_Processor` created when a single one suffices. (There is an exception in that a second processor is necessary in order to find an inner block wrapper). - The code relies on the incidental fact that searching by a whitespace-separated list of class names works if the class names in the target tag are in the same order. In this patch the use of the HTML API is refactored to address these opportunities and clean up a few places where there could be stronger consistency with other use patterns of the HTML API: - Multiple instances of the Tag Processor have been combined to remove overhead, extra variables, and code confusion. The new flow is more linear throughout the function instead of branching. - Updated HTML is returned via `get_updated_html()` instead of casting to a string. - The matching logic to find the inner block wrapper has been commented and the condition uses the null-coalescing operator now that WordPress requires PHP 7.0+. - When attempting to find the inner block wrapper at the end, a custom comparison is made against the `class` attribute instead of relying on `next_tag()` to find a tag with the given set of class names. The last refactor is important as a preliminary step to WordPress/wordpress-develop#5096 where `has_class()` and `class_list()` methods are being introduced to the Tag Processor. In that patch the implicit functionality of matching `'class_name' => 'more than one class'` is removed since that's not a single class name, but many.

tellthemachines added 3 commits February 3, 2023 14:08

Inner block layout enhancements

d284f61

Add tests for wp_render_layout_support_flag

13d4ea3

Update classname order in fixtures

9b12e22

tellthemachines requested review from hellofromtonya, Mamaduka, noisysocks and ntsekouras February 3, 2023 05:01

tellthemachines self-assigned this Feb 3, 2023

Fix whitespace.

aed3ad6

ntsekouras requested a review from dream-encode February 3, 2023 09:28

Revert whitespace removal in fixture files.

2d883e7

Mamaduka reviewed Feb 6, 2023

View reviewed changes

ntsekouras reviewed Feb 6, 2023

View reviewed changes

Mamaduka mentioned this pull request Feb 6, 2023

Plugin: Backport PHP changes for WordPress 6.2 release WordPress/gutenberg#47187

Closed

85 tasks

dream-encode requested changes Feb 6, 2023

View reviewed changes

felixarntz requested changes Feb 6, 2023

View reviewed changes

tellthemachines added 2 commits February 7, 2023 10:47

Inline function and fix whitespace.

45b8776

Change test assertion

77c9305

tellthemachines requested review from dream-encode and felixarntz February 6, 2023 23:59

Merge branch 'trunk' into add/render-layout-support-updates

b16224b

dream-encode approved these changes Feb 7, 2023

View reviewed changes

Fix fixtures.

26b1031

felixarntz approved these changes Feb 7, 2023

View reviewed changes

Merge branch 'trunk' into add/render-layout-support-updates

51b453c

felixarntz closed this Feb 7, 2023

tellthemachines mentioned this pull request Mar 1, 2023

Only add layout classes to inner wrapper if block is a container. WordPress/gutenberg#48611

Merged

dmsnell mentioned this pull request Aug 30, 2023

Block Supports: Re-use instance of Tag Processor when adding layout classes. WordPress/gutenberg#54075

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add/render layout support updates #3976

Add/render layout support updates #3976

tellthemachines commented Feb 3, 2023 •

edited

Loading

Mamaduka Feb 6, 2023

felixarntz Feb 6, 2023

ntsekouras left a comment

dream-encode Feb 6, 2023

dream-encode Feb 6, 2023

dream-encode Feb 6, 2023

dream-encode Feb 6, 2023

dream-encode Feb 6, 2023

dream-encode Feb 6, 2023

felixarntz Feb 6, 2023

felixarntz left a comment

felixarntz Feb 6, 2023

felixarntz Feb 6, 2023

felixarntz Feb 6, 2023

tellthemachines commented Feb 7, 2023

felixarntz commented Feb 7, 2023

dream-encode left a comment

felixarntz left a comment

felixarntz commented Feb 7, 2023

felixarntz commented Feb 7, 2023

	$this->assertEquals( $expected_output, $actual_output );
	$this->assertSame( $expected_output, $actual_output );

		$outer_class_names = array();

		if ( $has_child_layout && ( 'fixed' === $block['attrs']['style']['layout']['selfStretch'] \|\| 'fill' === $block['attrs']['style']['layout']['selfStretch'] ) ) {

	if ( ! $support_layout
	&& ! $has_child_layout ) {
	if ( ! $support_layout && ! $has_child_layout ) {

	* We want to target the opening tag of the inner blocks wrapper, which is the last tag in that chunk.
	* This targets the opening tag of the inner blocks wrapper, which is the last tag in that chunk.

Add/render layout support updates #3976

Add/render layout support updates #3976

Conversation

tellthemachines commented Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntsekouras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tellthemachines commented Feb 7, 2023

felixarntz commented Feb 7, 2023

dream-encode left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz commented Feb 7, 2023

felixarntz commented Feb 7, 2023

tellthemachines commented Feb 3, 2023 •

edited

Loading