From 2039994a4d70682bdef2f9dfec3b33ed015607aa Mon Sep 17 00:00:00 2001 From: Shyamsundar Gadde Date: Fri, 6 Dec 2024 21:38:07 +0530 Subject: [PATCH 1/7] Consider an LCP element as common if common in first and last groups and other groups are empty Signed-off-by: Shyamsundar Gadde --- .../class-od-url-metric-group-collection.php | 49 ++++++++++--------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/plugins/optimization-detective/class-od-url-metric-group-collection.php b/plugins/optimization-detective/class-od-url-metric-group-collection.php index 9b50846eb..204c205c5 100644 --- a/plugins/optimization-detective/class-od-url-metric-group-collection.php +++ b/plugins/optimization-detective/class-od-url-metric-group-collection.php @@ -437,38 +437,41 @@ public function get_common_lcp_element(): ?OD_Element { $result = ( function () { - // If every group isn't populated, then we can't say whether there is a common LCP element across every viewport group. - if ( ! $this->is_every_group_populated() ) { + // Ensure both the narrowest (first) and widest (last) viewport groups are populated. + $first_group = $this->get_first_group(); + $last_group = $this->get_last_group(); + if ( $first_group->count() === 0 || $last_group->count() === 0 ) { return null; } - // Look at the LCP elements across all the viewport groups. - $groups_by_lcp_element_xpath = array(); - $lcp_elements_by_xpath = array(); - $group_has_unknown_lcp_element = false; - foreach ( $this->groups as $group ) { - $lcp_element = $group->get_lcp_element(); - if ( $lcp_element instanceof OD_Element ) { - $groups_by_lcp_element_xpath[ $lcp_element->get_xpath() ][] = $group; - $lcp_elements_by_xpath[ $lcp_element->get_xpath() ][] = $lcp_element; - } else { - $group_has_unknown_lcp_element = true; - } - } + $first_group_lcp_element = $first_group->get_lcp_element(); + $last_group_lcp_element = $last_group->get_lcp_element(); + // Validate LCP elements exist and have matching XPaths in the extreme viewport groups. if ( - // All breakpoints share the same LCP element. - 1 === count( $groups_by_lcp_element_xpath ) - && - // The breakpoints don't share a common lack of a detected LCP element. - ! $group_has_unknown_lcp_element + ! $first_group_lcp_element instanceof OD_Element + || + ! $last_group_lcp_element instanceof OD_Element + || + $first_group_lcp_element->get_xpath() !== $last_group_lcp_element->get_xpath() ) { - $xpath = key( $lcp_elements_by_xpath ); + return null; // No common LCP element across the narrowest and widest viewports. + } - return $lcp_elements_by_xpath[ $xpath ][0]; + // Check intermediate viewport groups for conflicting LCP elements. + $num_groups = count( $this->groups ); + for ( $i = 1; $i < $num_groups - 1; $i++ ) { + $group_lcp_element = $this->groups[ $i ]->get_lcp_element(); + if ( + $group_lcp_element instanceof OD_Element + && + $group_lcp_element->get_xpath() !== $first_group_lcp_element->get_xpath() + ) { + return null; // Conflicting LCP element found in an intermediate group. + } } - return null; + return $first_group_lcp_element; } )(); $this->result_cache[ __FUNCTION__ ] = $result; From ac723e140dfc81480cfc35326da582657f6f1e1d Mon Sep 17 00:00:00 2001 From: Shyamsundar Gadde Date: Fri, 6 Dec 2024 22:06:32 +0530 Subject: [PATCH 2/7] Add test case for new logic Signed-off-by: Shyamsundar Gadde --- ...th-url-metrics-missing-in-other-groups.php | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 plugins/image-prioritizer/tests/test-cases/fetch-priority-high-on-lcp-image-common-on-mobile-and-desktop-with-url-metrics-missing-in-other-groups.php diff --git a/plugins/image-prioritizer/tests/test-cases/fetch-priority-high-on-lcp-image-common-on-mobile-and-desktop-with-url-metrics-missing-in-other-groups.php b/plugins/image-prioritizer/tests/test-cases/fetch-priority-high-on-lcp-image-common-on-mobile-and-desktop-with-url-metrics-missing-in-other-groups.php new file mode 100644 index 000000000..251a550fa --- /dev/null +++ b/plugins/image-prioritizer/tests/test-cases/fetch-priority-high-on-lcp-image-common-on-mobile-and-desktop-with-url-metrics-missing-in-other-groups.php @@ -0,0 +1,68 @@ + static function ( Test_Image_Prioritizer_Helper $test_case ): void { + $breakpoint_max_widths = array( 480, 600, 782 ); + + add_filter( + 'od_breakpoint_max_widths', + static function () use ( $breakpoint_max_widths ) { + return $breakpoint_max_widths; + } + ); + + OD_URL_Metrics_Post_Type::store_url_metric( + od_get_url_metrics_slug( od_get_normalized_query_vars() ), + $test_case->get_sample_url_metric( + array( + 'viewport_width' => 375, + 'elements' => array( + array( + 'xpath' => '/*[1][self::HTML]/*[2][self::BODY]/*[1][self::IMG]', + 'isLCP' => true, + ), + ), + ) + ) + ); + + OD_URL_Metrics_Post_Type::store_url_metric( + od_get_url_metrics_slug( od_get_normalized_query_vars() ), + $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, + ), + ), + ) + ) + ); + }, + 'buffer' => ' + + + + ... + + + Foo + + + ', + 'expected' => ' + + + + ... + + + + + Foo + + + + ', +); From 55ac18ab55186a229c4879eedbb20a01953410a7 Mon Sep 17 00:00:00 2001 From: Shyamsundar Gadde Date: Fri, 6 Dec 2024 22:06:55 +0530 Subject: [PATCH 3/7] Add missing since tag Signed-off-by: Shyamsundar Gadde --- .../class-od-url-metric-group-collection.php | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/optimization-detective/class-od-url-metric-group-collection.php b/plugins/optimization-detective/class-od-url-metric-group-collection.php index 204c205c5..b80d85b26 100644 --- a/plugins/optimization-detective/class-od-url-metric-group-collection.php +++ b/plugins/optimization-detective/class-od-url-metric-group-collection.php @@ -427,6 +427,7 @@ public function get_groups_by_lcp_element( string $xpath ): array { * Gets common LCP element. * * @since 0.3.0 + * @since n.e.x.t An LCP element is also considered common if it is the same in the narrowest and widest viewport groups, and all intermediate groups are empty. * * @return OD_Element|null Common LCP element if it exists. */ From 25cb61cd55e3db89ddab79a0610a98d731b06db3 Mon Sep 17 00:00:00 2001 From: Shyamsundar Gadde Date: Fri, 6 Dec 2024 23:23:10 +0530 Subject: [PATCH 4/7] Refactor LCP conflict check to use `foreach` Co-authored-by: Weston Ruter --- .../class-od-url-metric-group-collection.php | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/plugins/optimization-detective/class-od-url-metric-group-collection.php b/plugins/optimization-detective/class-od-url-metric-group-collection.php index b80d85b26..ed7fe6928 100644 --- a/plugins/optimization-detective/class-od-url-metric-group-collection.php +++ b/plugins/optimization-detective/class-od-url-metric-group-collection.php @@ -456,19 +456,18 @@ public function get_common_lcp_element(): ?OD_Element { || $first_group_lcp_element->get_xpath() !== $last_group_lcp_element->get_xpath() ) { - return null; // No common LCP element across the narrowest and widest viewports. + return null; // No common LCP element across the narrowest and widest viewports. } // Check intermediate viewport groups for conflicting LCP elements. - $num_groups = count( $this->groups ); - for ( $i = 1; $i < $num_groups - 1; $i++ ) { - $group_lcp_element = $this->groups[ $i ]->get_lcp_element(); + foreach ( array_slice( $this->groups, 1, -1 ) as $group ) { + $group_lcp_element = $group->get_lcp_element(); if ( $group_lcp_element instanceof OD_Element && $group_lcp_element->get_xpath() !== $first_group_lcp_element->get_xpath() ) { - return null; // Conflicting LCP element found in an intermediate group. + return null; // Conflicting LCP element found in an intermediate group. } } From 3540d9ee3a202286218f97a47124b068a037c1d5 Mon Sep 17 00:00:00 2001 From: Shyamsundar Gadde Date: Sat, 7 Dec 2024 07:33:10 +0530 Subject: [PATCH 5/7] Update test case test_get_common_lcp_element Signed-off-by: Shyamsundar Gadde --- ...-class-od-url-metrics-group-collection.php | 124 ++++++++++++++---- 1 file changed, 102 insertions(+), 22 deletions(-) diff --git a/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php b/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php index 066f34f8b..abbfd3756 100644 --- a/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php +++ b/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php @@ -721,45 +721,125 @@ public function test_get_groups_by_lcp_element(): void { $this->assertNull( $group_collection->get_common_lcp_element() ); } + /** + * Data provider. + * + * @return array + */ + public function data_provider_test_get_common_lcp_element(): array { + $xpath1 = '/*[1][self::HTML]/*[2][self::BODY]/*[1][self::IMG]/*[1]'; + $xpath2 = '/*[1][self::HTML]/*[2][self::BODY]/*[1][self::IMG]/*[2]'; + + $get_sample_url_metric = function ( int $viewport_width, string $lcp_element_xpath, bool $is_lcp ): OD_URL_Metric { + return $this->get_sample_url_metric( + array( + 'viewport_width' => $viewport_width, + 'element' => array( + 'isLCP' => $is_lcp, + 'xpath' => $lcp_element_xpath, + ), + ) + ); + }; + + return array( + 'all_groups_have_common_lcp' => array( + 'url_metrics' => array( + $get_sample_url_metric( 400, $xpath1, true ), + $get_sample_url_metric( 600, $xpath1, true ), + $get_sample_url_metric( 1000, $xpath1, true ), + ), + 'expected' => array( + 'type' => OD_Element::class, + 'xpath' => $xpath1, + ), + ), + 'no_url_metrics' => array( + 'url_metrics' => array(), + 'expected' => null, + ), + 'empty_first_group' => array( + 'url_metrics' => array( + $get_sample_url_metric( 600, $xpath1, true ), + $get_sample_url_metric( 1000, $xpath1, true ), + ), + 'expected' => null, + ), + 'empty_last_group' => array( + 'url_metrics' => array( + $get_sample_url_metric( 400, $xpath1, true ), + $get_sample_url_metric( 600, $xpath1, true ), + ), + 'expected' => null, + ), + 'first_and_last_common_lcp_others_empty' => array( + 'url_metrics' => array( + $get_sample_url_metric( 400, $xpath1, true ), + $get_sample_url_metric( 1000, $xpath1, true ), + ), + 'expected' => array( + 'type' => OD_Element::class, + 'xpath' => $xpath1, + ), + ), + 'intermediate_groups_conflict' => array( + 'url_metrics' => array( + $get_sample_url_metric( 400, $xpath1, true ), + $get_sample_url_metric( 600, $xpath2, true ), + $get_sample_url_metric( 1000, $xpath1, true ), + ), + 'expected' => null, + ), + 'first_and_last_lcp_mismatch' => array( + 'url_metrics' => array( + $get_sample_url_metric( 400, $xpath1, true ), + $get_sample_url_metric( 600, $xpath1, true ), + $get_sample_url_metric( 1000, $xpath2, true ), + ), + 'expected' => null, + ), + 'no_lcp_metrics' => array( + 'url_metrics' => array( + $get_sample_url_metric( 400, $xpath1, false ), + $get_sample_url_metric( 600, $xpath1, false ), + $get_sample_url_metric( 1000, $xpath1, false ), + ), + 'expected' => null, + ), + ); + } + /** * Test get_common_lcp_element(). * * @covers ::get_common_lcp_element + * + * @dataProvider data_provider_test_get_common_lcp_element + * + * @param OD_URL_Metric[] $url_metrics URL Metrics. + * @param mixed $expected Expected. */ - public function test_get_common_lcp_element(): void { + public function test_get_common_lcp_element( array $url_metrics, $expected ): void { $breakpoints = array( 480, 800 ); $sample_size = 3; $current_etag = md5( '' ); $group_collection = new OD_URL_Metric_Group_Collection( - array(), + $url_metrics, $current_etag, $breakpoints, $sample_size, HOUR_IN_SECONDS ); - $lcp_element_xpath = '/*[1][self::HTML]/*[2][self::BODY]/*[1][self::IMG]/*[1]'; - - foreach ( array_merge( $breakpoints, array( 1000 ) ) as $viewport_width ) { - for ( $i = 0; $i < $sample_size; $i++ ) { - $group_collection->add_url_metric( - $this->get_sample_url_metric( - array( - 'viewport_width' => $viewport_width, - 'element' => array( - 'isLCP' => true, - 'xpath' => $lcp_element_xpath, - ), - ) - ) - ); - } - } - $this->assertCount( 3, $group_collection ); + $common_lcp_element = $group_collection->get_common_lcp_element(); - $this->assertInstanceOf( OD_Element::class, $common_lcp_element ); - $this->assertSame( $lcp_element_xpath, $common_lcp_element['xpath'] ); + if ( is_array( $expected ) ) { + $this->assertInstanceOf( $expected['type'], $common_lcp_element ); + $this->assertSame( $expected['xpath'], $common_lcp_element->get_xpath() ); + } else { + $this->assertNull( $common_lcp_element ); + } } /** From 952f04c28281dca3a2d2d85b0bd16f1effd10bbc Mon Sep 17 00:00:00 2001 From: Shyamsundar Gadde Date: Mon, 9 Dec 2024 10:33:36 +0530 Subject: [PATCH 6/7] Set default parameter value for test case helper function Signed-off-by: Shyamsundar Gadde --- ...-class-od-url-metrics-group-collection.php | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php b/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php index abbfd3756..026048df0 100644 --- a/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php +++ b/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php @@ -730,7 +730,7 @@ public function data_provider_test_get_common_lcp_element(): array { $xpath1 = '/*[1][self::HTML]/*[2][self::BODY]/*[1][self::IMG]/*[1]'; $xpath2 = '/*[1][self::HTML]/*[2][self::BODY]/*[1][self::IMG]/*[2]'; - $get_sample_url_metric = function ( int $viewport_width, string $lcp_element_xpath, bool $is_lcp ): OD_URL_Metric { + $get_sample_url_metric = function ( int $viewport_width, string $lcp_element_xpath, bool $is_lcp = true ): OD_URL_Metric { return $this->get_sample_url_metric( array( 'viewport_width' => $viewport_width, @@ -745,9 +745,9 @@ public function data_provider_test_get_common_lcp_element(): array { return array( 'all_groups_have_common_lcp' => array( 'url_metrics' => array( - $get_sample_url_metric( 400, $xpath1, true ), - $get_sample_url_metric( 600, $xpath1, true ), - $get_sample_url_metric( 1000, $xpath1, true ), + $get_sample_url_metric( 400, $xpath1 ), + $get_sample_url_metric( 600, $xpath1 ), + $get_sample_url_metric( 1000, $xpath1 ), ), 'expected' => array( 'type' => OD_Element::class, @@ -760,22 +760,22 @@ public function data_provider_test_get_common_lcp_element(): array { ), 'empty_first_group' => array( 'url_metrics' => array( - $get_sample_url_metric( 600, $xpath1, true ), - $get_sample_url_metric( 1000, $xpath1, true ), + $get_sample_url_metric( 600, $xpath1 ), + $get_sample_url_metric( 1000, $xpath1 ), ), 'expected' => null, ), 'empty_last_group' => array( 'url_metrics' => array( - $get_sample_url_metric( 400, $xpath1, true ), - $get_sample_url_metric( 600, $xpath1, true ), + $get_sample_url_metric( 400, $xpath1 ), + $get_sample_url_metric( 600, $xpath1 ), ), 'expected' => null, ), 'first_and_last_common_lcp_others_empty' => array( 'url_metrics' => array( - $get_sample_url_metric( 400, $xpath1, true ), - $get_sample_url_metric( 1000, $xpath1, true ), + $get_sample_url_metric( 400, $xpath1 ), + $get_sample_url_metric( 1000, $xpath1 ), ), 'expected' => array( 'type' => OD_Element::class, @@ -784,17 +784,17 @@ public function data_provider_test_get_common_lcp_element(): array { ), 'intermediate_groups_conflict' => array( 'url_metrics' => array( - $get_sample_url_metric( 400, $xpath1, true ), - $get_sample_url_metric( 600, $xpath2, true ), - $get_sample_url_metric( 1000, $xpath1, true ), + $get_sample_url_metric( 400, $xpath1 ), + $get_sample_url_metric( 600, $xpath2 ), + $get_sample_url_metric( 1000, $xpath1 ), ), 'expected' => null, ), 'first_and_last_lcp_mismatch' => array( 'url_metrics' => array( - $get_sample_url_metric( 400, $xpath1, true ), - $get_sample_url_metric( 600, $xpath1, true ), - $get_sample_url_metric( 1000, $xpath2, true ), + $get_sample_url_metric( 400, $xpath1 ), + $get_sample_url_metric( 600, $xpath1 ), + $get_sample_url_metric( 1000, $xpath2 ), ), 'expected' => null, ), From 5af021de189af16615c4ecff23f75d2cb9888ecc Mon Sep 17 00:00:00 2001 From: Shyamsundar Gadde Date: Mon, 9 Dec 2024 23:29:59 +0530 Subject: [PATCH 7/7] Simplify data provider for `test_comment_lcp_element` Co-authored-by: Weston Ruter --- ...-class-od-url-metrics-group-collection.php | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php b/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php index 026048df0..179d282c4 100644 --- a/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php +++ b/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php @@ -749,10 +749,7 @@ public function data_provider_test_get_common_lcp_element(): array { $get_sample_url_metric( 600, $xpath1 ), $get_sample_url_metric( 1000, $xpath1 ), ), - 'expected' => array( - 'type' => OD_Element::class, - 'xpath' => $xpath1, - ), + 'expected' => $xpath1, ), 'no_url_metrics' => array( 'url_metrics' => array(), @@ -777,10 +774,7 @@ public function data_provider_test_get_common_lcp_element(): array { $get_sample_url_metric( 400, $xpath1 ), $get_sample_url_metric( 1000, $xpath1 ), ), - 'expected' => array( - 'type' => OD_Element::class, - 'xpath' => $xpath1, - ), + 'expected' => $xpath1, ), 'intermediate_groups_conflict' => array( 'url_metrics' => array( @@ -817,9 +811,9 @@ public function data_provider_test_get_common_lcp_element(): array { * @dataProvider data_provider_test_get_common_lcp_element * * @param OD_URL_Metric[] $url_metrics URL Metrics. - * @param mixed $expected Expected. + * @param string|null $expected Expected. */ - public function test_get_common_lcp_element( array $url_metrics, $expected ): void { + public function test_get_common_lcp_element( array $url_metrics, ?string $expected ): void { $breakpoints = array( 480, 800 ); $sample_size = 3; $current_etag = md5( '' ); @@ -834,9 +828,9 @@ public function test_get_common_lcp_element( array $url_metrics, $expected ): vo $this->assertCount( 3, $group_collection ); $common_lcp_element = $group_collection->get_common_lcp_element(); - if ( is_array( $expected ) ) { - $this->assertInstanceOf( $expected['type'], $common_lcp_element ); - $this->assertSame( $expected['xpath'], $common_lcp_element->get_xpath() ); + if ( is_string( $expected ) ) { + $this->assertInstanceOf( OD_Element::class, $common_lcp_element ); + $this->assertSame( $expected, $common_lcp_element->get_xpath() ); } else { $this->assertNull( $common_lcp_element ); }