Lightbox: Re-use existing Tag Processor instance.

[dmsnell]

Status

• as of yet I have not tested the changes in here, but I did test the changes in the companion PR in Core, so unless I misapplied the refactor things seems to be working the way they previously were.
• I know that there could be some interplay here with the missing-image bug fix and changes to ensure that not every image gets the lightbox setting applied to it. I'm happy to keep this updated and handle any conflicts.

Companion work in WordPress/wordpress-develop#5428

Resolves #55123

What?

This patch refactors the use of the HTML API in image lightbox rendering. It replaces creating multiple Tag Processor instances with the use of a single instance that rewinds and jumps around to perform multiple jobs on a single instance.

Also incorporated is a fix for bailing out when no images are in a given image block's output HTML.

Why?

The use of the HTML API in the current Lightbox rendering code is a bit confusing and it's inefficient (pending measurements). Being official code in Core/Gutenberg it sets an example for other developers wanting to learn how to use the HTML API. This refactor exists to ensure that we set a strong example of various uses of the Tag Processor and how to balance performance and usability needs.

How?

Reuses the existing Tag Processor instance and relies on bookmarks and seeking to make successive changes.

Testing Instructions

Review the code changes, test posts with "Expand on click" enabled for an image, for multiple images, for no images. Ensure that the behaviors here and in trunk match.

[dmsnell]

this isn't as important, but I carried it over from my other patch since I've tried to standardize tag names in the HTML API in uppercase, only because that's the form of the tag name that the API hands out

[github-actions]

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

• [Flaky Test] should partially select with shift + click (@webkit) #53524 in /test/e2e/specs/editor/various/multi-block-selection.spec.js
• [Flaky Test] should select synced pattern #55284 in /test/e2e/specs/editor/various/writing-flow.spec.js

[michalczaplinski]

In general this looks all right and the majority of the refactors make it easier to read. Re-using the Tag Processor obviously makes sense.

However, I'm pretty concerned that the need to manually reset each property after calling seek() makes it much harder to update this file in the future. If I want to add a $processor->addClass('newClass') call anywhere in this file in the future, I would also have to find the right place to add $processor->removeClass('newClass'). This is not an acceptable burden on future developers 😅

I would be much happier to merge this, it if there were a built in-method in the Tag Processor that we could call to reset the internal state and we could use it in this PR, like I mention below in #55281 (comment)

[michalczaplinski]

I'm not 100% sure about this assumption. Other elements are also valid children of the <figure>, e.g. there might be a <figcaption>.

[dmsnell]

this will still fall on an IMG, but I'm noting that assumes that the image appears later in the document than the FIGURE, and is also assuming that an image is inside the figure.

in the HTML Processor we'd express this in the following way:

if ( ! $processor->next_tag( array( 'breadcrumbs' => array( 'FIGURE', 'IMG' ) ) ) ) {
	return $block_content;
}

I can update the comment to make this clearer.

[dmsnell]

expanded the comment in 0c0f5bc

[michalczaplinski]

Interesting. This is beyond the scope of this PR of course, but are there any plans to add something like a $processor->reset() method or maybe a $processor->seek( '<tag-name>', $reset = true ) to the Tag Processor so that there's no need to reset each property manually?

[dmsnell]

I don't think it will make sense to include a $reset_changes mode to the tag processor because of the complexity and runtime costs that could introduce. I have explored a chunked mode that could be used here instead, but in this particular case I think most solutions will be a bit messy.

your concern is real though, and I share it. it's a bit nasty having to reset. I did run some initial profiling but it's hard to get a good handle on the runtime impact without a more typical representative page, something I didn't have time to do last week.

[michalczaplinski]

Closing this now as I think it does not apply anymore and the PR would require a significant rewrite because of the changes in the index.php structure of the Image block.

Feel free to re-open if you plan to work on it in the future! 🙂