Update OD_HTML_Tag_Processor::next_tag() to allow $query arg and prepare to skip visiting tag closers by default

[westonruter]

I realized that the restriction to disallow passing the $query arg to next_tag() is obsolete since the breadcrumbs are tracked by calls to next_token(). The disallowing of the $query arg was a mistake ever since the subclass was introduced in #1302.

The test_next_tag_with_query test case confirms that the breadcrumbs (XPaths) are still computed correctly even when passing queries to next_token().

Before OD_HTML_Tag_Processor was introduced there was the OD_HTML_Tag_Walker class which wrapped an instance of WP_HTML_Tag_Processor. This wrapper walker class at that time was responsible for keeping track of the breadcrumbs and computing the XPaths. It only ever called $processor->next_tag( array( 'tag_closers' => 'visit' ) ) as it didn't integrate with the lower-level next_token() method to be able to observe the consumption of all tokens. This logic was ported over to the OD_HTML_Tag_Processor subclass of WP_HTML_Tag_Processor unnecessarily.

Also, since the OD_HTML_Tag_Walker had to visit all tags including tag closers in order to keep track of the breadcrumbs, this meant that the ported logic into the OD_HTML_Tag_Processor::next_tag() method visited tag closers by default even though WP_HTML_Tag_Processor::next_tag() does not visit tag closers by default. In fact, there was a OD_HTML_Tag_Processor::next_open_tag() method introduced which had the behavior for skipping tag closer visiting. All of this seems like it would be confusing for people expecting that the $context->processor supplied to tag visitors would have a next_tag() method that behaves like they expect, to skip visiting tag closers by default. But currently it doesn't: again, it does visit tag closers by default. This PR intends to move toward restoring the base method's logic. (This PR also soft-deprecates the redundant next_open_tag() method.)

The first step is to add a warning when no $query arg is supplied to the next_tag() method. Previously if a plugin tried to supply a $query it would actually throw an exception. Now, however, they'll get a migration warning when they don't supply the $query argument. The migration warning is to inform developers that the default behavior of next_tag() is changing from visiting tag closers by default to skipping tag visitors by default.

See westonruter/od-intrinsic-dimensions#2 for how another plugin can avoid the migration warning while also avoiding a fatal error when attempting to run with an older version of Optimization Detective which doesn't allow passing $query to next_tag().

I think this necessitates adding an optimization-detective 1.0.0-beta3 milestone. This bumps the version to 1.0.0-beta3 to anticipate this.

Lastly, the handling of the admin bar is now consistent with the handling of NOSCRIPT. While no tag visitor will be explicitly be invoked with either the NOSCRIPT tag or any tag in the admin bar, it is possible for tag visitors to manually walk over those tags if they want to. This was done so that the next_tag() method can be (eventually) unmodified in its behavior from the method in the base class.

[codecov]

Codecov Report

Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Project coverage is 66.72%. Comparing base (42bd7a8) to head (7a7411a).
Report is 22 commits behind head on trunk.

Files with missing lines	Patch %	Lines
plugins/optimization-detective/load.php	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk    #1872      +/-   ##
==========================================
+ Coverage   66.70%   66.72%   +0.01%     
==========================================
  Files          88       88              
  Lines        7029     7033       +4     
==========================================
+ Hits         4689     4693       +4     
  Misses       2340     2340              

Flag	Coverage Δ
multisite	66.72% <94.11%> (+0.01%)	⬆️
single	37.16% <17.64%> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[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.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <[email protected]>
Co-authored-by: felixarntz <[email protected]>

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

[felixarntz]

@westonruter Mostly looks good, just one thing I think needs to be changed and one other concern/question.

[felixarntz]

Why is this needed? I thought we had previously decided to omit any tags within the admin bar anyway as part of OD_HTML_Tag_Processor itself.

[westonruter]

See this explanation in the PR description:

Lastly, the handling of the admin bar is now consistent with the handling of NOSCRIPT. While no tag visitor will be explicitly be invoked with either the NOSCRIPT tag or any tag in the admin bar, it is possible for tag visitors to manually walk over those tags if they want to. This was done so that the next_tag() method can be (eventually) unmodified in its behavior from the method in the base class.

This will make it easier to switch to WP_HTML_Processor as well, since the next_tag() method won't need to be overridden for that class either.

[felixarntz]

While no tag visitor will be explicitly be invoked with either the NOSCRIPT tag or any tag in the admin bar, it is possible for tag visitors to manually walk over those tags if they want to.

How would that be possible? Can you share an example?

[westonruter]

Yes, for example, in #1746 is about undoing JS-based lazy loading in favor of native lazy loading. Consider this markup used by lazysizes:

<img src="data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7" data-src="https://example.com/bar.jpg" data-srcset="https://example.com/bar-large.jpg 1000w, https://example.com/bar-large.jpg 1000w" sizes="(max-width: 556px) 100vw, 556px" alt="Bar" class="attachment-large size-large wp-image-2 has-transparency lazyload" width="500" height="300">
<noscript>
	<img src="https://example.com/bar.jpg" srcset="https://example.com/bar-large.jpg 1000w, https://example.com/bar-large.jpg 1000w" sizes="(max-width: 556px) 100vw, 556px" alt="Bar" class="attachment-large size-large wp-image-2 has-transparency lazyload" width="500" height="300">
</noscript>

To optimize this, a tag visitor would need to (1) replace the src with the data-src, (2) replace the srcset with the data-srcset, and finally (3) remove the noscript entirely. A tag visitor implementing this could look like the following:

<?php
add_action( 'od_register_tag_visitors', function ( OD_Tag_Visitor_Registry $registry ) {
	$registry->register( 'native-lazy-loading', function ( OD_Tag_Visitor_Context $context ) {
		$processor = $context->processor;
		if ( $processor->get_tag() !== 'IMG' || ! $processor->has_class( 'lazyload' ) ) {
			return; // Tag is not relevant for this tag visitor.
		}

		$src    = $processor->get_attribute( 'data-src' );
		$srcset = $processor->get_attribute( 'data-srcset' );

		if ( ! is_string( $src ) && ! is_string( $srcset ) ) {
			return;
		}

		// Replace the src attribute with the data-src attribute.
		if ( is_string( $src ) ) {
			$processor->set_attribute( 'src', $src );
			$processor->remove_attribute( 'data-src' );
		}

		// Replace the srcset attribute with the data-srcset attribute.
		if ( is_string( $srcset ) ) {
			$processor->set_attribute( 'srcset', $srcset );
			$processor->remove_attribute( 'data-srcset' );
		}

		$processor->remove_class( 'lazyload' );

		if ( $processor->next_tag() && $processor->get_tag() === 'NOSCRIPT' ) {
			if ( method_exists( $processor, 'remove_tag' ) ) {
				// Note: This is not yet supported by the HTML API! But this is how the NOSCRIPT could in theory be removed in the future.
				$processor->remove_tag();
			} elseif ( $processor->next_tag() && $processor->get_tag() === 'IMG' ) {
				// This alternative prevents the duplicate NOSCRIPT > IMG from being shown.
				$processor->remove_attribute( 'src' ); // Prevent loading the image.
				$processor->remove_attribute( 'srcset' ); // Prevent loading the image.
				$processor->set_attribute( 'hidden', true ); // Prevent the image from being rendered twice.
			}
		}
	} );
} );

So in this case, while tag visitors don't iterate over NOSCRIPT elements or their descendants by default, it is useful for there to still be the possibility to do so during their own iteration outside of the overall "optimization loop".

[felixarntz]

See my other comment - wondering about the need for this based on what we had previously discussed.