Block Library: Refactor core blocks to use HTML Tag Processor

[gziolo]

What?

The full proposal for the new HTM Tag Processor API is available at https://make.wordpress.org/core/2022/08/19/a-new-system-for-simply-and-reliably-updating-html-attributes/.

Why?

This branch presents the usage of the new HTML Tag Processor API introduced by @adamziel and @dmsnell in #42485.

How?

Updated core blocks:

• Cover
• Image
• Site Logo
• Social Link (code style changes only)

Testing Instructions

All updated core blocks should work as before.

[gziolo]

I rebase this branch with trunk. Refactored code to use the new class name WP_HTML_Tag_Processor. One of the core blocks Post Featured Image got refactored and it no longer is necessary to use Tag Processor there.

I wrapped all values passed to set_attribute with esc_attr to present how this would look in practice if we leave it up to developers.

[adamziel]

Esc_attr shouldn’t be needed thanks to #44447. Let’s remove it to avoid escaping the value twice.

[gziolo]

Esc_attr shouldn’t be needed thanks to #44447. Let’s remove it to avoid escaping the value twice.

Nice, it still might be necessary to use some sanitization in some cases so we need to have a good documentation that covers all possible scenarios.

[gziolo]

@adamziel and @dmsnell, I think this PR depends on how #44447 ends up handling escaping attributes. It would be great if you could bring it to the finish line. 1983b8f removes esc_attr usage, so if we need to add it back, we only need to remove this commit.

[dmsnell]

would $tags or $tag_stream be more useful names here? I think we discussed this at one point. seeing $processor here reads to me as if things are more complicated than they are.

$tag_stream = new WP_HTML_Tag_Processor( $content );
$tag_stream->next_tag();
$tag_stream->set_attribute( 'style', $styles );
$content = (string) $tag_stream;

not that I care how people name the instance of this class, but given that it's core code I recognize that we're setting the examples and how we do it will influence how people use it; I want our examples to be super clear. (not saying $tag_stream is universally clearer, just raising the point to discuss)

[gziolo]

Good thinking. It's one of the reasons I wanted to have this PR opened early on so we can exercise this API in the codebase.

We still have some time to apply changes to the API or how we will promote usage. From the reader's perspective, I might prefer:

$tag_stream = new WP_HTML_Tag_Stream( $content );
$tag_stream->next();
$tag_stream->set_attribute( 'style', $styles );
$content = (string) $tag_stream;

I changed the name of the class to align with the variable. I also renamed next_tag to avoid repeating the word tag twice.

$tags is shorter, so it might be more desirable. It also feels similar to taxonomy tags, but I don't think they will be used too often together.

[dmsnell]

I also renamed next_tag to avoid repeating the word tag twice.

double-check with @adamziel. that seems like a much bigger change since it impacts the API and not only the application-side. if someone does create $p = new WP_HTML_Tag_Processor(); then they'll have $p->next()

in my original proposal I used ->find() as the query function but @adamziel had feelings about how ->next_tag() communicated forward progression through the document.

I could be fine with next(), given that we now have WP_HTML_Tag_Processor indicating what "next" is, but it does feel slightly sparser in the self-documenting way (and at the same time, repeating "tag" here doesn't feel like any real inconvenience)

[adamziel]

It feels weird to say $tag_stream = new WP_HTML_Tag_Processor(); – I think I'd rather say something like $tags = new WP_HTML_Tag_Processor() or $tags = new WP_HTML_Tag_Stream(). The latter doesn't communicate the processing capabilities, though, so I'd go with $tags = new WP_HTML_Tag_Processor().

If we use $tags as the variable name, it makes sense to say ->next(), but if we don't, then I'd rather say ->next_tag(). Since we can only suggest these conventions but not enforce them, I feel like ->next_tag() will be more useful for the larger audience, even if a tiny bit more annoying to type.

[gziolo]

I'm inclined to leave it as is for now. The following part doesn't look ideal:

$tags->set_attribute( 'foo', 'boo' );
$content = $tags->get_updated_html();

[dmsnell]

places like this make me appreciate the new interface.

[gziolo]

We are in full agreement here 💯

[adamziel]

💯 indeed

[dmsnell]

If we want these in WordPress 6.2 we should hurry and merge them, otherwise I don't think it matters if we merge sooner or later.

[gziolo]

I refreshed this PR to use the latest API. It doesn't matter if we include it in WordPress 6.2. Actually, it's probably better to test it in the plugin first.

[adamziel]

Still looks great to me @gziolo, feel free to merge or postpone as you see fit.

[github-actions]

Flaky tests detected in 9bb0a03.
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/4231586927
📝 Reported issues:

• [Flaky Test] should always expand single line selection #39787 in specs/editor/various/multi-block-selection.test.js

[gziolo]

Testing

Cover block

✅ Minimum height style applied correctly

Image block

✅ Aria label and target applied correctly

Social Icon

✅ Still properly handles rel and target attributes

[adamziel]

Still looking good 👍

[carolinan]

I'd like to know if hurry was the only reason the regex for removing the link from the site logo was left as is?
That regex was what was flagged as incorrect and used as one of the reasons for including the tag processor in the first place :).
If that is the only reason, that is not a problem! No complaints. I am only asking to learn if there was another reason or if I can continue with replacing the regex.

[gziolo]

I'd like to know if hurry was the only reason the regex for removing the link from the site logo was left as is?

HTML Tag Processor doesn't support removing HTML tags as of today, it only allows operations on HTML attributes.

[carolinan]

Aha! Thank you @gziolo. We need to wait then.