Block bindings: Add support for image caption

[SantosGuillamot]

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

This pull request should be tested together with this other one from Gutenberg, because it needs changes in the image render file.

What?

Add support for the image caption attribute in block bindings.

Why?

There is an issue with pattern overrides caused by this: WordPress/gutenberg#62287.

Additionally, it is a good enhancement.

How?

I created an anonymous class to extend the tag processor and include set_inner_text until there is a similar method provided by default.

Additionally, in Gutenberg pull request, I'm modifying the render file of the image to:

• Remove the figcaption element when it exists, and the binding value is empty.
• Add the figcaption element when it doesn't exist, and the binding value is not empty.

---

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.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[dmsnell]

@SantosGuillamot this is a fine use of sub-classing, though I do want us to be extra careful here, as by adding subclassing code into Core we are setting an example to others, and this is not the example we want to send more broadly, especially since it's coming later.

To that end there are a few things to start with on this. I hope it's okay to extra resilient here.

• There's no need to create the anonymous class outside of the if ( 'core/image' … ) {} block. It's probably fine, but since all of the processing happens within, its scope might be more clear if left inside of it instead of outside of it.

• let's follow all of Core's documentation requirements, such as a docblock for the added method.

• one thing I think is essential for "exceptional cases" like this in Core is full checking to ensure we're not trying to replace the wrong tokens. for example, here we might check that the processor is paused on an opening tag. it would be ideal if we also put a terse but scary warning right at the top indicating that this is a stop-gap measure in Core not to be emulated.

public function set_inner_text( $raw_text_content ) {
	if (
		WP_HTML_Tag_Processor::STATE_MATCHED_TAG !== $this->parser_state ||
		$this->is_tag_closer()
	) {
		return false;
	}
}

• It might be ideal to wait to grab the bookmark until the end. The greater the distance between binding the bookmark offsets and its use, the greater the danger. That is, it's fine to set the bookmark at the top, but we shouldn't read it until we add the lexical_updates[].

• This one isn't important, but since this processor dies immediately we don't need to release the bookmarks.

• The comment on "Visit the closing tag" is nice, but I think it could really help to indicate that this is a best-effort guess and that code should rely on the HTML Processor for this kind of operation.

To that end, we expect to get image blocks here don't we? Is there anything in the image block that the HTML Processor doesn't support? Maybe we should start instead with it and see if we can skip the Tag Processor.

[SantosGuillamot]

Thanks a lot for all the feedback, @dmsnell ! That's really helpful 🙂 I totally agree we should be really careful, so please share any more suggestions you have. Going to your points:

To that end, we expect to get image blocks here don't we? Is there anything in the image block that the HTML Processor doesn't support? Maybe we should start instead with it and see if we can skip the Tag Processor.

Yes, I think it should be safe to use the HTML Processor. I've just changed the code to extend that class instead of the Tag Processor: link.

I had to pass the WP_HTML_Processor::CONSTRUCTOR_UNLOCK_CODE to skip the warning message. I hope that's okay.

There's no need to create the anonymous class outside of the if ( 'core/image' … ) {} block.

I've moved it into the conditional now. At first, I kept it outside because I was considering using it for other blocks like the paragraph, heading, or button. But we can move it out if we decide to take that approach.

let's follow all of Core's documentation requirements, such as a docblock for the added method.

I added a comment to the function here. I added a warning notice trying to push back users from replicating that code. Let me know if that's what you had in mind.

we might check that the processor is paused on an opening tag.

I added this check as well: link.

It might be ideal to wait to grab the bookmark until the end.

I moved this logic just before using lexical updates as suggested: link.

since this processor dies immediately we don't need to release the bookmarks

I've removed the bookmarks now.

I think it could really help to indicate that this is a best-effort guess and that code should rely on the HTML Processor for this kind of operation.

I added another comment trying to clarify this: link.

---

Please let me know further improvements!

[github-actions]

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.

Core Committers: Use this line as a base for the props when committing in SVN:

Props santosguillamot, dmsnell, luisherranz, cbravobernal, ellatrix.

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

[ellatrix]

I'm quite worried about introducing caption support this late in the release cycle. Imo it deserves some testing as a feature in the GB plugin first. I've made this alternative PR that disallows overrides for an image with caption: WordPress/gutenberg#62747.

[SantosGuillamot]

I totally understand the concerns. And thanks a lot for the feedback. I agree that if support for image caption is not added, code to disallowing overrides in images with captions for 6.6 could be a good alternative.

[dmsnell]

this is an interesting side-effect of that choice. I think we should rename that constant to something scarier, like what its value is.

I kind of like that it makes anonymous classes stand out more.

[dmsnell]

@sirreal I think this is a really good example of a part of the HTML Processor we're going to have to improve.

not only is the unlock code ugly here (ugly, because it shouts how this is work-around territory), but also by calling the constructor directly we bypass setting up the context node and the HTML node. in effect, this is starting as a full parser instead of a fragment parser.

I wonder how we can better allow subclassing without this hassle and risk.

[dmsnell]

whoah. this is unrelated to your changes @SantosGuillamot, but this line really confuses me, purely based on how PHP's anonymous classes work. @sirreal maybe what I wrote above is fine, and when we call new class( ...$args ) they don't matter?

yes, I just tested and this is odd. calling $bindings_processor->next_token() will crash because it never finds the context node. this means that it looks like an HTML Processor but isn't. it's better as a HTML Processor Builder.

@SantosGuillamot I wonder if we could take advantage of this in line 338 and instead of passing $block_content pass something like "do not use this, it will not work. it's only here to create a subclass and call the static creator method"

[SantosGuillamot]

I am not 100% this is what you meant, but I made this change to address the feedback. Let me know if that's not what you were suggesting.

[dmsnell]

Thanks @SantosGuillamot for all your adjustments. I think this is worth giving it a try. I'd prefer the name of the method be scarier, like seriously_wait_for_core_to_replace_inner_html() but that's just a personal preference. This will be I think the first subclass going into Core doing dangerous operations; it makes me nervous is all.

[SantosGuillamot]

I'd prefer the name of the method be scarier, like seriously_wait_for_core_to_replace_inner_html() but that's just a personal preference.

While I agree we could change the name to prevent users even more from copying it, seriously_wait_for_core_to_replace_inner_html seems too much to me. Here are some other options that, combined with the warning message, I believe should be scary enough:

• do_not_copy_set_figcaption_inner_html.
• private_set_figcaption_inner_html.
• internal_do_not_use_figcaption_inner_html.

Apart from that, we could always wait to support the image caption until the HTML Processor provides its own method.

Any thoughts?

[cbravobernal]

• private_set_figcaption_inner_html

I would go with private_set_figcaption_inner_html