Merge pull request #1376 from WordPress/fix/od-seek

Fix logic for seeking during optimization loop to prevent emitting seek() notices
WordPress · Jul 19, 2024 · f70b0a2 · f70b0a2
2 parents d4cc524 + 1bb8bbc
commit f70b0a2
Show file tree

Hide file tree

Showing 9 changed files with 179 additions and 20 deletions.
diff --git a/plugins/auto-sizes/tests/test-optimization-detective.php b/plugins/auto-sizes/tests/test-optimization-detective.php
@@ -87,7 +87,7 @@ public function data_provider_test_od_optimize_template_output_buffer(): array {
 					'intersectionRatio' => 1,
 				),
 				'buffer'          => '<img src="https://example.com/foo.jpg" alt="Foo" width="1200" height="800" loading="lazy" srcset="https://example.com/foo-480w.jpg 480w, https://example.com/foo-800w.jpg 800w" sizes="auto, (max-width: 600px) 480px, 800px">',
-				'expected'        => '<img data-od-replaced-sizes="auto, (max-width: 600px) 480px, 800px" data-od-removed-loading="lazy" src="https://example.com/foo.jpg" alt="Foo" width="1200" height="800"  srcset="https://example.com/foo-480w.jpg 480w, https://example.com/foo-800w.jpg 800w" sizes="(max-width: 600px) 480px, 800px">',
+				'expected'        => '<img data-od-removed-loading="lazy" data-od-replaced-sizes="auto, (max-width: 600px) 480px, 800px" src="https://example.com/foo.jpg" alt="Foo" width="1200" height="800"  srcset="https://example.com/foo-480w.jpg 480w, https://example.com/foo-800w.jpg 800w" sizes="(max-width: 600px) 480px, 800px">',
 			),
 		);
 	}

diff --git a/plugins/embed-optimizer/tests/test-optimization-detective.php b/plugins/embed-optimizer/tests/test-optimization-detective.php
@@ -476,14 +476,14 @@ function ( OD_Tag_Visitor_Context $context ): bool {
 						<body>
 							<figure data-od-xpath="/*[1][self::HTML]/*[2][self::BODY]/*[1][self::FIGURE]" class="wp-block-embed is-type-video">
 								<div class="wp-block-embed__wrapper">
-									<video data-od-xpath="/*[1][self::HTML]/*[2][self::BODY]/*[1][self::FIGURE]/*[1][self::DIV]/*[1][self::VIDEO]" data-od-added-preload preload="auto" src="https://example.com/video1.mp4" poster="https://example.com/poster1.jpg" width="640" height="480"></video>
+									<video data-od-added-preload data-od-xpath="/*[1][self::HTML]/*[2][self::BODY]/*[1][self::FIGURE]/*[1][self::DIV]/*[1][self::VIDEO]" preload="auto" src="https://example.com/video1.mp4" poster="https://example.com/poster1.jpg" width="640" height="480"></video>
 								</div>
 							</figure>
 							<figure data-od-xpath="/*[1][self::HTML]/*[2][self::BODY]/*[2][self::FIGURE]" class="wp-block-embed is-type-rich is-provider-figurine wp-block-embed-figurine">
 								<div class="wp-block-embed__wrapper">
 									<figure>
 										<p>So I heard you like <code>FIGURE</code>?</p>
-										<video data-od-xpath="/*[1][self::HTML]/*[2][self::BODY]/*[2][self::FIGURE]/*[1][self::DIV]/*[1][self::FIGURE]/*[2][self::VIDEO]" data-od-added-preload preload="none" src="https://example.com/video2.mp4" poster="https://example.com/poster2.jpg" width="640" height="480"></video>
+										<video data-od-added-preload data-od-xpath="/*[1][self::HTML]/*[2][self::BODY]/*[2][self::FIGURE]/*[1][self::DIV]/*[1][self::FIGURE]/*[2][self::VIDEO]" preload="none" src="https://example.com/video2.mp4" poster="https://example.com/poster2.jpg" width="640" height="480"></video>
 										<figcaption>Tagline from Figurine embed.</figcaption>
 									</figure>
 									<iframe data-od-added-loading loading="lazy" src="https://example.com/" width="640" height="480"></iframe>
@@ -522,7 +522,7 @@ function ( OD_Tag_Visitor_Context $context ): bool {
 										// Set a bunch of bookmarks to fill up the total allowed.
 										$remaining_bookmark_count = WP_HTML_Tag_Processor::MAX_BOOKMARKS - count( $bookmarks );
 										for ( $i = 0; $i < $remaining_bookmark_count; $i++ ) {
-											$processor->set_bookmark( "body_bookmark_{$i}" );
+											$this->assertTrue( $processor->set_bookmark( "body_bookmark_{$i}" ) );
 										}
 										return true;
 									}

diff --git a/plugins/image-prioritizer/tests/test-helper.php b/plugins/image-prioritizer/tests/test-helper.php
@@ -48,7 +48,7 @@ public function data_provider_test_filter_tag_visitors(): array {
 							<title>...</title>
 						</head>
 						<body>
-							<img data-od-xpath="/*[1][self::HTML]/*[2][self::BODY]/*[1][self::IMG]" data-od-unknown-tag src="https://example.com/foo.jpg" alt="Foo" width="1200" height="800" loading="lazy">
+							<img data-od-unknown-tag data-od-xpath="/*[1][self::HTML]/*[2][self::BODY]/*[1][self::IMG]" src="https://example.com/foo.jpg" alt="Foo" width="1200" height="800" loading="lazy">
 							<script type="module">/* import detect ... */</script>
 						</body>
 					</html>
@@ -340,8 +340,8 @@ public function data_provider_test_filter_tag_visitors(): array {
 							<link data-od-added-tag rel="preload" fetchpriority="high" as="image" href="https://example.com/foo.jpg" media="screen and (min-width: 783px)">
 						</head>
 						<body>
-							<img data-od-xpath="/*[1][self::HTML]/*[2][self::BODY]/*[1][self::IMG]" data-od-removed-loading="lazy" src="https://example.com/foo.jpg" alt="Foo" width="1200" height="800" >
-							<img data-od-xpath="/*[1][self::HTML]/*[2][self::BODY]/*[2][self::IMG]" data-od-removed-loading="lazy" src="https://example.com/bar.jpg" alt="Bar" width="10" height="10"  fetchpriority="high">
+							<img data-od-removed-loading="lazy" data-od-xpath="/*[1][self::HTML]/*[2][self::BODY]/*[1][self::IMG]" src="https://example.com/foo.jpg" alt="Foo" width="1200" height="800" >
+							<img data-od-removed-loading="lazy" data-od-xpath="/*[1][self::HTML]/*[2][self::BODY]/*[2][self::IMG]" src="https://example.com/bar.jpg" alt="Bar" width="10" height="10"  fetchpriority="high">
 							<script type="module">/* import detect ... */</script>
 						</body>
 					</html>

diff --git a/plugins/optimization-detective/class-od-html-tag-processor.php b/plugins/optimization-detective/class-od-html-tag-processor.php
@@ -107,6 +107,7 @@ final class OD_HTML_Tag_Processor extends WP_HTML_Tag_Processor {
 	/**
 	 * Bookmark for the end of the HEAD.
 	 *
+	 * @todo Consider reserving this.
 	 * @since 0.4.0
 	 * @var string
 	 */
@@ -115,6 +116,7 @@ final class OD_HTML_Tag_Processor extends WP_HTML_Tag_Processor {
 	/**
 	 * Bookmark for the end of the BODY.
 	 *
+	 * @todo Consider reserving this.
 	 * @since 0.4.0
 	 * @var string
 	 */
@@ -176,6 +178,15 @@ final class OD_HTML_Tag_Processor extends WP_HTML_Tag_Processor {
 	 */
 	private $buffered_text_replacements = array();
 
+	/**
+	 * Count for the number of times next_token() was called
+	 *
+	 * @since 0.4.1
+	 * @var int
+	 * @see self::next_token()
+	 */
+	private $next_token_count = 0;
+
 	/**
 	 * Finds the next tag.
 	 *
@@ -247,6 +258,7 @@ public function expects_closer( ?string $tag_name = null ): bool {
 	 */
 	public function next_token(): bool {
 		$this->current_xpath = null; // Clear cache.
+		++$this->next_token_count;
 		if ( ! parent::next_token() ) {
 			$this->open_stack_tags    = array();
 			$this->open_stack_indices = array();
@@ -325,6 +337,18 @@ public function next_token(): bool {
 		return true;
 	}
 
+	/**
+	 * Gets the number of times next_token() was called.
+	 *
+	 * @since 0.4.1
+	 * @see self::next_token()
+	 *
+	 * @return int Count of next_token() calls.
+	 */
+	public function get_next_token_count(): int {
+		return $this->next_token_count;
+	}
+
 	/**
 	 * Updates or creates a new attribute on the currently matched tag with the passed value.
 	 *
@@ -408,6 +432,18 @@ public function seek( $bookmark_name ): bool {
 		return $result;
 	}
 
+	/**
+	 * Gets the number of times seek() was called.
+	 *
+	 * @since 0.4.1
+	 * @see self::seek()
+	 *
+	 * @return int Count of seek() calls.
+	 */
+	public function get_seek_count(): int {
+		return $this->seek_count;
+	}
+
 	/**
 	 * Sets a bookmark in the HTML document.
 	 *
@@ -539,8 +575,11 @@ public function get_updated_html(): string {
 			}
 			if ( ! $this->has_bookmark( $bookmark ) ) {
 				$this->warn(
-					/* translators: %s is the bookmark name */
-					__( 'Unable to append markup to %s since the bookmark no longer exists.', 'optimization-detective' )
+					sprintf(
+						/* translators: %s is the bookmark name */
+						__( 'Unable to append markup to %s since the bookmark no longer exists.', 'optimization-detective' ),
+						$bookmark
+					)
 				);
 			} else {
 				$start = $this->bookmarks[ $bookmark ]->start;

diff --git a/plugins/optimization-detective/load.php b/plugins/optimization-detective/load.php
@@ -5,7 +5,7 @@
  * Description: Provides an API for leveraging real user metrics to detect optimizations to apply on the frontend to improve page performance.
  * Requires at least: 6.5
  * Requires PHP: 7.2
- * Version: 0.4.0
+ * Version: 0.4.1
  * Author: WordPress Performance Team
  * Author URI: https://make.wordpress.org/performance/
  * License: GPLv2 or later
@@ -65,7 +65,7 @@ static function ( string $global_var_name, string $version, Closure $load ): voi
 	}
 )(
 	'optimization_detective_pending_plugin',
-	'0.4.0',
+	'0.4.1',
 	static function ( string $version ): void {
 
 		// Define the constant.

diff --git a/plugins/optimization-detective/optimization.php b/plugins/optimization-detective/optimization.php
@@ -176,14 +176,18 @@ function od_optimize_template_output_buffer( string $buffer ): string {
 	$visitors             = iterator_to_array( $tag_visitor_registry );
 	while ( $processor->next_open_tag() ) {
 		$did_visit = false;
-		$processor->set_bookmark( $current_tag_bookmark );
-		foreach ( $visitors as $visitor ) {
-			$did_visit = $visitor( $tag_visitor_context ) || $did_visit;
+		$processor->set_bookmark( $current_tag_bookmark ); // TODO: Should we break if this returns false?
 
-			// Since the visitor may have traversed HTML tags, we need to make sure we go back to this tag so that
-			// in the next iteration any relevant tag visitors may apply, in addition to properly setting the data-od-xpath
-			// on this tag below.
-			$processor->seek( $current_tag_bookmark );
+		foreach ( $visitors as $visitor ) {
+			$seek_count       = $processor->get_seek_count();
+			$next_token_count = $processor->get_next_token_count();
+			$did_visit        = $visitor( $tag_visitor_context ) || $did_visit;
+
+			// If the visitor traversed HTML tags, we need to go back to this tag so that in the next iteration any
+			// relevant tag visitors may apply, in addition to properly setting the data-od-xpath on this tag below.
+			if ( $seek_count !== $processor->get_seek_count() || $next_token_count !== $processor->get_next_token_count() ) {
+				$processor->seek( $current_tag_bookmark ); // TODO: Should this break out of the optimization loop if it returns false?
+			}
 		}
 		$processor->release_bookmark( $current_tag_bookmark );
 

diff --git a/plugins/optimization-detective/readme.txt b/plugins/optimization-detective/readme.txt
@@ -2,7 +2,7 @@
 
 Contributors: wordpressdotorg
 Tested up to: 6.6
-Stable tag:   0.4.0
+Stable tag:   0.4.1
 License:      GPLv2 or later
 License URI:  https://www.gnu.org/licenses/gpl-2.0.html
 Tags:         performance, optimization, rum
@@ -133,6 +133,16 @@ The [plugin source code](https://github.com/WordPress/performance/tree/trunk/plu
 
 == Changelog ==
 
+= 0.4.1 =
+
+**Enhancements**
+
+* Upgrade web-vitals.js from [v3.5.0](https://github.com/GoogleChrome/web-vitals/blob/main/CHANGELOG.md#v350-2023-09-28) to [v4.2.1](https://github.com/GoogleChrome/web-vitals/blob/main/CHANGELOG.md#v422-2024-07-17).
+
+**Bug Fixes**
+
+* Fix logic for seeking during optimization loop to prevent emitting seek() notices. ([1376](https://github.com/WordPress/performance/pull/1376))
+
 = 0.4.0 =
 
 **Enhancements**

diff --git a/plugins/optimization-detective/tests/test-class-od-html-tag-processor.php b/plugins/optimization-detective/tests/test-class-od-html-tag-processor.php
@@ -519,6 +519,7 @@ public function test_bookmarking_and_seeking(): void {
 		);
 
 		$actual_figure_contents = array();
+		$this->assertSame( 0, $processor->get_seek_count() );
 
 		$bookmarks = array();
 		while ( $processor->next_open_tag() ) {
@@ -580,6 +581,7 @@ public function test_bookmarking_and_seeking(): void {
 				'depth' => $processor->get_current_depth(),
 			);
 		}
+		$this->assertSame( count( $bookmarks ), $processor->get_seek_count() );
 
 		$this->assertSame( $expected_figure_contents, $sought_actual_contents );
 
@@ -602,6 +604,38 @@ public function test_bookmarking_and_seeking(): void {
 		// TODO: Try adding too many bookmarks.
 	}
 
+	/**
+	 * Test get_seek_count.
+	 *
+	 * @covers ::get_seek_count
+	 */
+	public function test_get_next_token_count(): void {
+		$processor = new OD_HTML_Tag_Processor(
+			trim(
+				'
+				<html>
+					<head></head>
+					<body></body>
+				</html>
+				'
+			)
+		);
+		$this->assertSame( 0, $processor->get_next_token_count() );
+		$this->assertTrue( $processor->next_tag() );
+		$this->assertSame( 'HTML', $processor->get_tag() );
+		$this->assertSame( 1, $processor->get_next_token_count() );
+		$this->assertTrue( $processor->next_tag() );
+		$this->assertSame( 'HEAD', $processor->get_tag() );
+		$this->assertSame( 3, $processor->get_next_token_count() ); // Note that next_token() call #2 was for the whitespace between <html> and <head>.
+		$this->assertTrue( $processor->next_tag() );
+		$this->assertSame( 'HEAD', $processor->get_tag() );
+		$this->assertTrue( $processor->is_tag_closer() );
+		$this->assertSame( 4, $processor->get_next_token_count() );
+		$this->assertTrue( $processor->next_tag() );
+		$this->assertSame( 'BODY', $processor->get_tag() );
+		$this->assertSame( 6, $processor->get_next_token_count() ); // Note that next_token() call #5 was for the whitespace between </head> and <body>.
+	}
+
 	/**
 	 * Export an array as a PHP literal to use as a snapshot.
 	 *

diff --git a/plugins/optimization-detective/tests/test-optimization.php b/plugins/optimization-detective/tests/test-optimization.php
@@ -262,7 +262,6 @@ public function data_provider_test_od_optimize_template_output_buffer(): array {
 			'video'                => array(
 				'set_up'   => function (): void {
 					$slug = od_get_url_metrics_slug( od_get_normalized_query_vars() );
-					$sample_size = od_get_url_metrics_breakpoint_sample_size();
 					foreach ( array_merge( od_get_breakpoint_max_widths(), array( 1000 ) ) as $viewport_width ) {
 						OD_URL_Metrics_Post_Type::store_url_metric(
 							$slug,
@@ -309,6 +308,79 @@ public function data_provider_test_od_optimize_template_output_buffer(): array {
 				',
 			),
 
+			'many_images'          => array(
+				'set_up'   => function (): void {
+					$slug = od_get_url_metrics_slug( od_get_normalized_query_vars() );
+					foreach ( array_merge( od_get_breakpoint_max_widths(), array( 1000 ) ) as $viewport_width ) {
+
+						$elements = array();
+						for ( $i = 1; $i < WP_HTML_Tag_Processor::MAX_SEEK_OPS; $i++ ) {
+							$elements[] = array(
+								'xpath' => sprintf( '/*[1][self::HTML]/*[2][self::BODY]/*[%d][self::IMG]', $i ),
+								'isLCP' => false,
+							);
+						}
+
+						OD_URL_Metrics_Post_Type::store_url_metric(
+							$slug,
+							$this->get_validated_url_metric(
+								$viewport_width,
+								$elements
+							)
+						);
+					}
+				},
+				'buffer'   => '
+					<html lang="en">
+						<head>
+							<meta charset="utf-8">
+							<title>...</title>
+						</head>
+						<body>
+							' .
+							join(
+								"\n",
+								call_user_func(
+									static function () {
+										$tags = array();
+										for ( $i = 1; $i < WP_HTML_Tag_Processor::MAX_SEEK_OPS + 1; $i++ ) {
+											$tags[] = sprintf( '<img src="https://example.com/foo.jpg" alt="Foo" width="1200" height="800" loading="lazy">' );
+										}
+										return $tags;
+									}
+								)
+							) .
+							'
+						</body>
+					</html>
+				',
+				'expected' => '
+					<html lang="en">
+						<head>
+							<meta charset="utf-8">
+							<title>...</title>
+						</head>
+						<body>
+							' .
+							join(
+								"\n",
+								call_user_func(
+									static function () {
+										$tags = array();
+										for ( $i = 1; $i < WP_HTML_Tag_Processor::MAX_SEEK_OPS + 1; $i++ ) {
+											$tags[] = sprintf( '<img data-od-xpath="/*[1][self::HTML]/*[2][self::BODY]/*[%d][self::IMG]" src="https://example.com/foo.jpg" alt="Foo" width="1200" height="800" loading="lazy">', $i );
+										}
+										return $tags;
+									}
+								)
+							) .
+							'
+							<script type="module">/* import detect ... */</script>
+						</body>
+					</html>
+				',
+			),
+
 			'rss-response'         => array(
 				'set_up'   => static function (): void {
 					ini_set( 'default_mimetype', 'application/rss+xml' ); // phpcs:ignore WordPress.PHP.IniSet.Risky