Block Supports: Re-use instance of Tag Processor when adding layout classes.

[dmsnell]

What?

Refactors the use of the HTML API in Block Supports for layout. Resolves an issue that accidentally works today but will break in future iteration of the HTML API.

Why?

• Improve consistency of example use of the HTML API throughout Core.
• Demonstrate positive hygiene for use of the HTML API, specifically in terms of overhead.
• Prevent this code from introducing issues when correcting a design issue in the Tag Processor.

While searching for multiple class names with $processor->next_tag( array( 'class_name' => 'many class names' ) ) may seem convenient, it only incidentally works today in the Tag Processor because of partial support for matching class names. This can lead to unexpected results, for instance, when class names on the tag are reordered. A more proper method of searching would be resilient to ordering and whitespace in the class attribute, which is what is introduced in WordPress/wordpress-develop#5096. Unfortunately, this code must be changed before that merges otherwise it will break layout support.

How?

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.

Testing Instructions

Review the code changes, ensure tests pass.

There should be no functional or visual changes in this patch.

[github-actions]

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ lib/block-supports/layout.php

[tellthemachines]

Thanks for the PR, code changes LGTM! Just left a couple minor comments about the comments 😄

Testing with several different layout blocks shows everything working as expected:

• Cover block - which has multiple wrappers - has layout classes applied to its inner wrapper;
• Navigation block still has classes applied to its outer wrapper, which is expected as it has its own render callback;
• Other single-wrapper blocks such as Group and Buttons have layout classes applied correctly.

[tellthemachines]

Maybe we can be a bit more specific here with something like "In other words, if the block itself doesn't support layout..." instead?

[dmsnell]

Can you explain the original comment in different words? I think my ambiguity here comes from my inability to understand what "if only child layout exists" means.

[dmsnell]

specifically, what does has_child_layout mean?

[tellthemachines]

Hmm yeah I could have done a better job with the original comment 😅

has_child_layout means that the block is inside a block with flex layout that has allowSizingOnChildren set to true, so it has access to certain layout properties, even if the block doesn't itself support layout.

Currently the only properties applied to flex children are selfStretch and flexSize, but more might be added in the future. Any block at all that is a child of a flex block with allowSizingOnChildren (currently the only core blocks that support it are Group variations and Navigation) has access to those properties, through a set of controls in the Dimensions panel:

So at this point in the code we're checking if the block supports layout, because if it doesn't we can just add whatever child layout properties it has and skip all the logic below.

[dmsnell]

thanks for clarifying; I've attempted to explain this in comments in the latest version of the patch.

	/*
	 * A block may not support layout but still be affected by a parent block's layout.
	 *
	 * In these cases add the appropriate class names and then return early; there's
	 * no need to investigate on this block whether additional layout constraints apply.
	 */

[tellthemachines]

Thanks for adding this super detailed explanation!

I'd just clarify here that what we're trying to match is the innermost wrapper for the inner blocks, not the first inner block.

(From what I recall the problem was that when processing the inner blocks we don't have access to the wrapper. It would be great to find a better way of doing this though!)

[dmsnell]

aha, well I was actually intending on leaving the note about matching the inner block because then we can tell when we've crossed into it. identifying a block wrapper can work when there is a wrapper and when a block indicates it, but there is no general concept of an inner block wrapper, whereas there is a concept of an inner block.

I'll clarify the comment to make this clearer why we would want to identify the inner block and not the wrapper.

[tellthemachines]

Oh I see, that makes sense!

[tellthemachines]

I think that the last tag identified before the inner blocks will actually be the outermost tag if the block only has a single wrapper element for its inner blocks.

[dmsnell]

yeah. thanks for clarifying. it was incredibly difficult for me to figure out what the goals of this code was, so my comments have been more or less a record of my attempts to do so, and a document for the future developer who comes in and wants to understand the logic.

[tellthemachines]

Sorry about that, this should have had some more inline comments for clarity. Thanks for adding them!

[github-actions]

Flaky tests detected in 196747eb8cfdd353e76cac7ba9ef36a167c5e569.
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/6053826558
📝 Reported issues:

• [Flaky Test] Should load the view script if the lightbox is enabled via theme.json #53944 in /test/e2e/specs/editor/various/behaviors.spec.js

[mikachan]

@dmsnell 👋 I'm working on backports for WP 6.4 and it looks like this PR is already included. This is the backport branch for 6.4 Beta 2, and all these changes are present.

I wanted to confirm with you as this PR isn't marked for any Gutenberg release milestone which confused me, although I am confident that the changes are in the 6.4 branch!

Edit: Actually, I think this needs to be listed in #54177, as it's a PHP change. Do we already have a trac ticket for this?

[dmsnell]

@mikachan we'll have to talk about release milestones in Gutenberg another time - it's new to me. thankfully this change was backported into Core four days ago by @tellthemachines in WordPress/wordpress-develop#5252 so there should be nothing more that needs to be done in either repo. please let me know if you find that something isn't right about that.

the code on each side remains different but I believe that may be because of other changes done around that file. when I backported the change I only brought this change and not any others.

[mikachan]

@dmsnell Thanks for confirming! Yeah you're right, looks like there's nothing else that needs to be done for this PR. I've removed the Needs PHP backport label.

I think milestones are usually added automatically by GitHub when a PR is merged to make sure the PR is automatically assigned to a release version, and the biggest reason for a PR to be tagged is so it's listed in the correct changelog. So I don't think anything needs to be changed here either, but I guess it was probably missed off a changelog 😅