From e8e1d7d52484400eacb4da7f93a1997d20a3d0d1 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sun, 15 Dec 2024 20:33:04 -0800 Subject: [PATCH 1/3] Add test cases demonstrating erroneous behavior --- .../img-non-native-lazy-loading.php | 86 +++++++++++++++++++ .../tests/test-cases/noscript.php | 31 +++++++ 2 files changed, 117 insertions(+) create mode 100644 plugins/image-prioritizer/tests/test-cases/img-non-native-lazy-loading.php create mode 100644 plugins/optimization-detective/tests/test-cases/noscript.php diff --git a/plugins/image-prioritizer/tests/test-cases/img-non-native-lazy-loading.php b/plugins/image-prioritizer/tests/test-cases/img-non-native-lazy-loading.php new file mode 100644 index 0000000000..37fa084e60 --- /dev/null +++ b/plugins/image-prioritizer/tests/test-cases/img-non-native-lazy-loading.php @@ -0,0 +1,86 @@ + static function ( Test_Image_Prioritizer_Helper $test_case ): void { + $slug = od_get_url_metrics_slug( od_get_normalized_query_vars() ); + + // Populate one URL Metric so that none of the IMG elements are unknown. + OD_URL_Metrics_Post_Type::store_url_metric( + $slug, + $test_case->get_sample_url_metric( + array( + 'viewport_width' => 1000, + 'elements' => array( + array( + 'xpath' => '/*[1][self::HTML]/*[2][self::BODY]/*[1][self::IMG]', + 'isLCP' => true, + ), + array( + 'xpath' => '/*[1][self::HTML]/*[2][self::BODY]/*[2][self::IMG]', + 'isLCP' => false, + ), + ), + ) + ) + ); + }, + 'buffer' => ' + + + + ... + + + + + + + + Bar + + + + ', + 'expected' => ' + + + + ... + + + + + + + + + Bar + + + + + ', +); diff --git a/plugins/optimization-detective/tests/test-cases/noscript.php b/plugins/optimization-detective/tests/test-cases/noscript.php new file mode 100644 index 0000000000..916c270e8a --- /dev/null +++ b/plugins/optimization-detective/tests/test-cases/noscript.php @@ -0,0 +1,31 @@ + static function (): void {}, + 'buffer' => ' + + + + ... + + + + + + ', + 'expected' => ' + + + + ... + + + + + + + ', +); From d6c07efeeac7f98b3a2f8fef166b7395c1214e51 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sun, 15 Dec 2024 20:36:00 -0800 Subject: [PATCH 2/3] Skip visiting any tag inside of a NOSCRIPT since never in DOM for detection --- .../tests/test-cases/img-non-native-lazy-loading.php | 2 +- plugins/optimization-detective/optimization.php | 5 +++++ plugins/optimization-detective/tests/test-cases/noscript.php | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/plugins/image-prioritizer/tests/test-cases/img-non-native-lazy-loading.php b/plugins/image-prioritizer/tests/test-cases/img-non-native-lazy-loading.php index 37fa084e60..d83e4ebb2b 100644 --- a/plugins/image-prioritizer/tests/test-cases/img-non-native-lazy-loading.php +++ b/plugins/image-prioritizer/tests/test-cases/img-non-native-lazy-loading.php @@ -77,7 +77,7 @@ class="lazyload wp-image-1" Bar diff --git a/plugins/optimization-detective/optimization.php b/plugins/optimization-detective/optimization.php index 74548eef12..4d7bec7783 100644 --- a/plugins/optimization-detective/optimization.php +++ b/plugins/optimization-detective/optimization.php @@ -227,6 +227,11 @@ function od_optimize_template_output_buffer( string $buffer ): string { $needs_detection = ! $group_collection->is_every_group_complete(); do { + // Never process anything inside NOSCRIPT since it will never show up in the DOM when scripting is enabled, and thus it can never be detected nor measured. + if ( in_array( 'NOSCRIPT', $processor->get_breadcrumbs(), true ) ) { + continue; + } + $tracked_in_url_metrics = false; $processor->set_bookmark( $current_tag_bookmark ); // TODO: Should we break if this returns false? diff --git a/plugins/optimization-detective/tests/test-cases/noscript.php b/plugins/optimization-detective/tests/test-cases/noscript.php index 916c270e8a..9756210dbe 100644 --- a/plugins/optimization-detective/tests/test-cases/noscript.php +++ b/plugins/optimization-detective/tests/test-cases/noscript.php @@ -22,7 +22,7 @@ From b2d972ce07069e3e8d44b20efcf5735f4937c8ae Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sun, 15 Dec 2024 22:13:32 -0800 Subject: [PATCH 3/3] Fix handling images with JS-based lazy-loading --- ...lass-image-prioritizer-img-tag-visitor.php | 99 ++++++++++++------- .../class-image-prioritizer-tag-visitor.php | 2 +- .../img-non-native-lazy-loading.php | 69 +++++++------ ...ent-with-source-having-media-attribute.php | 23 +---- ...ent-with-source-missing-type-attribute.php | 23 +---- 5 files changed, 114 insertions(+), 102 deletions(-) diff --git a/plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php b/plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php index ba14edf57e..3e84bec1f0 100644 --- a/plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php +++ b/plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php @@ -21,6 +21,14 @@ */ final class Image_Prioritizer_Img_Tag_Visitor extends Image_Prioritizer_Tag_Visitor { + /** + * List of PICTURE XPaths to skip processing of child IMG tags. + * + * @since n.e.x.t + * @var string[] + */ + private $picture_ancestor_xpaths_to_skip = array(); + /** * Visits a tag. * @@ -35,7 +43,11 @@ public function __invoke( OD_Tag_Visitor_Context $context ): bool { $tag = $processor->get_tag(); if ( 'PICTURE' === $tag ) { - return $this->process_picture( $processor, $context ); + $picture_xpath = $processor->get_xpath(); + if ( false === $this->process_picture( $processor, $context ) ) { + $this->picture_ancestor_xpaths_to_skip[] = $picture_xpath; + } + return false; // Because the IMG child is what gets tracked in URL Metrics. } elseif ( 'IMG' === $tag ) { return $this->process_img( $processor, $context ); } @@ -43,6 +55,38 @@ public function __invoke( OD_Tag_Visitor_Context $context ): bool { return false; } + /** + * Determines whether the current IMG is valid for tracking in URL Metrics. + * + * An IMG must have a src attribute which is not a data: URL. And if it has a srcset attribute, it also must not be + * a data: URL. + * + * @since n.e.x.t + * + * @param OD_HTML_Tag_Processor $processor Tag Processor. + * @return bool Whether valid for tracking in URL Metrics. + */ + private function is_img_with_valid_src_and_srcset( OD_HTML_Tag_Processor $processor ): bool { + $src = $this->get_attribute_value( $processor, 'src' ); + $has_src = ( is_string( $src ) && '' !== $src ); + if ( ! $has_src ) { + return false; + } + + $srcset = $this->get_attribute_value( $processor, 'srcset' ); + $has_srcset = ( is_string( $srcset ) && '' !== $srcset ); + + // Abort data: URLs (which may very be JS-based lazy-loading). + if ( $this->is_data_url( $src ) ) { + return false; + } + if ( $has_srcset && $this->is_data_url( $srcset ) ) { + return false; + } + + return true; + } + /** * Process an IMG element. * @@ -53,13 +97,21 @@ public function __invoke( OD_Tag_Visitor_Context $context ): bool { * @return bool Whether the tag should be tracked in URL Metrics. */ private function process_img( OD_HTML_Tag_Processor $processor, OD_Tag_Visitor_Context $context ): bool { - $src = $this->get_valid_src( $processor ); - if ( null === $src ) { + if ( ! $this->is_img_with_valid_src_and_srcset( $processor ) ) { return false; } $xpath = $processor->get_xpath(); + // If the PICTURE's processing was aborted, then abort processing its child IMG as well. + if ( 'PICTURE' === $this->get_parent_tag_name( $context ) ) { + foreach ( $this->picture_ancestor_xpaths_to_skip as $picture_xpath ) { + if ( str_starts_with( $xpath, $picture_xpath ) ) { + return false; + } + } + } + $current_fetchpriority = $this->get_attribute_value( $processor, 'fetchpriority' ); $is_lazy_loaded = 'lazy' === $this->get_attribute_value( $processor, 'loading' ); $updated_fetchpriority = null; @@ -187,7 +239,7 @@ private function process_img( OD_HTML_Tag_Processor $processor, OD_Tag_Visitor_C * * @param OD_HTML_Tag_Processor $processor HTML tag processor. * @param OD_Tag_Visitor_Context $context Tag visitor context. - * @return bool Whether the tag should be tracked in URL Metrics. + * @return bool Whether the PICTURE was processed. */ private function process_picture( OD_HTML_Tag_Processor $processor, OD_Tag_Visitor_Context $context ): bool { /** @@ -218,8 +270,13 @@ private function process_picture( OD_HTML_Tag_Processor $processor, OD_Tag_Visit } // Abort processing if a SOURCE lacks the required srcset attribute. - $srcset = $this->get_valid_src( $processor, 'srcset' ); - if ( null === $srcset ) { + $srcset = $this->get_attribute_value( $processor, 'srcset' ); + if ( ! is_string( $srcset ) ) { + return false; + } + + // Abort if the srcset is a data: URL since there is nothing to optimize. + if ( $this->is_data_url( $srcset ) ) { return false; } @@ -242,8 +299,8 @@ private function process_picture( OD_HTML_Tag_Processor $processor, OD_Tag_Visit // Process the IMG element within the PICTURE. if ( 'IMG' === $tag && ! $processor->is_tag_closer() ) { - $src = $this->get_valid_src( $processor ); - if ( null === $src ) { + // Abort if process_img() won't later be processing this IMG. + if ( ! $this->is_img_with_valid_src_and_srcset( $processor ) ) { return false; } @@ -274,31 +331,7 @@ private function process_picture( OD_HTML_Tag_Processor $processor, OD_Tag_Visit ) ); - return false; - } - - /** - * Gets valid src attribute value for preloading. - * - * Returns null if the src attribute is not a string (i.e. src was used as a boolean attribute was used), if it - * it has an empty string value after trimming, or if it is a data: URL. - * - * @since n.e.x.t - * - * @param OD_HTML_Tag_Processor $processor Processor. - * @param 'src'|'srcset' $attribute_name Attribute name. - * @return non-empty-string|null URL which is not a data: URL. - */ - private function get_valid_src( OD_HTML_Tag_Processor $processor, string $attribute_name = 'src' ): ?string { - $src = $processor->get_attribute( $attribute_name ); - if ( ! is_string( $src ) ) { - return null; - } - $src = trim( $src ); - if ( '' === $src || $this->is_data_url( $src ) ) { - return null; - } - return $src; + return true; } /** diff --git a/plugins/image-prioritizer/class-image-prioritizer-tag-visitor.php b/plugins/image-prioritizer/class-image-prioritizer-tag-visitor.php index ba2850f6a8..a464d29334 100644 --- a/plugins/image-prioritizer/class-image-prioritizer-tag-visitor.php +++ b/plugins/image-prioritizer/class-image-prioritizer-tag-visitor.php @@ -14,7 +14,7 @@ /** * Tag visitor that optimizes image tags. * - * @phpstan-type NormalizedAttributeNames 'fetchpriority'|'loading'|'crossorigin'|'preload'|'referrerpolicy'|'type' + * @phpstan-type NormalizedAttributeNames 'src'|'srcset'|'fetchpriority'|'loading'|'crossorigin'|'preload'|'referrerpolicy'|'type' * * @since 0.1.0 * @access private diff --git a/plugins/image-prioritizer/tests/test-cases/img-non-native-lazy-loading.php b/plugins/image-prioritizer/tests/test-cases/img-non-native-lazy-loading.php index d83e4ebb2b..6cd029b771 100644 --- a/plugins/image-prioritizer/tests/test-cases/img-non-native-lazy-loading.php +++ b/plugins/image-prioritizer/tests/test-cases/img-non-native-lazy-loading.php @@ -1,28 +1,14 @@ static function ( Test_Image_Prioritizer_Helper $test_case ): void { - $slug = od_get_url_metrics_slug( od_get_normalized_query_vars() ); + 'set_up' => static function (): void {}, - // Populate one URL Metric so that none of the IMG elements are unknown. - OD_URL_Metrics_Post_Type::store_url_metric( - $slug, - $test_case->get_sample_url_metric( - array( - 'viewport_width' => 1000, - 'elements' => array( - array( - 'xpath' => '/*[1][self::HTML]/*[2][self::BODY]/*[1][self::IMG]', - 'isLCP' => true, - ), - array( - 'xpath' => '/*[1][self::HTML]/*[2][self::BODY]/*[2][self::IMG]', - 'isLCP' => false, - ), - ), - ) - ) - ); - }, + /* + * Example 1 comes from Avada's Fusion_Images lazy images which replaces the srcset attribute with data-srcset but + * which leaves the src attribute as-is. + * + * Example 2 comes from Speed Optimizer v7.7.2 by Site Ground which uses lazysizes v5.3.1. + * See . + */ 'buffer' => ' @@ -31,7 +17,7 @@ - + - + + + + Foo + + + + Foo + + + + Foo + + + Bar