Compute responsive sizes attribute based on the width from the boundingClientRect in captured URL Metrics
#1840
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -44,6 +44,43 @@ public function __invoke( OD_Tag_Visitor_Context $context ): bool { | |||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Computes responsive sizes for the current element based on its boundingClientRect width captured in URL Metrics. | ||||||
| * | ||||||
| * @since n.e.x.t | ||||||
| * | ||||||
| * @param OD_Tag_Visitor_Context $context Context. | ||||||
| * @return non-empty-string[] Computed sizes. | ||||||
| */ | ||||||
| private function compute_sizes( OD_Tag_Visitor_Context $context ): array { | ||||||
| $sizes = array(); | ||||||
|
|
||||||
| $xpath = $context->processor->get_xpath(); | ||||||
| foreach ( $context->url_metric_group_collection as $group ) { | ||||||
| $element_max_width = 0; | ||||||
| foreach ( $group->get_xpath_elements_map()[ $xpath ] ?? array() as $element ) { | ||||||
| $element_max_width = max( $element_max_width, $element->get_bounding_client_rect()['width'] ); | ||||||
| } | ||||||
felixarntz marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
|
|
||||||
| if ( $element_max_width > 0 ) { | ||||||
| // TODO: The min-width will need to be adjusted after <https://github.com/WordPress/performance/pull/1839>. | ||||||
| // TODO: Consider reusing od_generate_media_query() here but note the extra parentheses should be removed from its output. | ||||||
| $media_queries = array(); | ||||||
| if ( $group->get_minimum_viewport_width() !== 0 ) { | ||||||
| $media_queries[] = sprintf( 'min-width: %dpx', $group->get_minimum_viewport_width() ); | ||||||
| } | ||||||
| if ( $group->get_maximum_viewport_width() !== PHP_INT_MAX && $group->get_maximum_viewport_width() !== null ) { | ||||||
| $media_queries[] = sprintf( 'max-width: %dpx', $group->get_maximum_viewport_width() ); | ||||||
| } | ||||||
| if ( count( $media_queries ) > 0 ) { | ||||||
| $sizes[] = sprintf( '(%s) %dpx', join( ' and ', $media_queries ), round( $element_max_width ) ); | ||||||
| } | ||||||
|
||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return $sizes; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Process an IMG element. | ||||||
| * | ||||||
|
|
@@ -146,21 +183,40 @@ private function process_img( OD_HTML_Tag_Processor $processor, OD_Tag_Visitor_C | |||||
| $processor->remove_attribute( 'fetchpriority' ); | ||||||
| } | ||||||
|
|
||||||
| // Ensure that sizes is set properly when it is a responsive image (it has a srcset attribute). | ||||||
| if ( is_string( $processor->get_attribute( 'srcset' ) ) ) { | ||||||
felixarntz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| $sizes = $processor->get_attribute( 'sizes' ); | ||||||
| if ( ! is_string( $sizes ) ) { | ||||||
| $sizes = ''; | ||||||
| } | ||||||
|
|
||||||
| $is_lazy = 'lazy' === $this->get_attribute_value( $processor, 'loading' ); | ||||||
| $has_auto = $this->sizes_attribute_includes_valid_auto( $sizes ); | ||||||
|
|
||||||
| if ( $is_lazy && ! $has_auto ) { | ||||||
| $new_sizes = 'auto'; | ||||||
| if ( '' !== trim( $sizes, " \t\f\r\n" ) ) { | ||||||
| $new_sizes .= ', '; | ||||||
| } | ||||||
| $sizes = $new_sizes . $sizes; | ||||||
| } elseif ( ! $is_lazy && $has_auto ) { | ||||||
| // Remove auto from the beginning of the list. | ||||||
| $sizes = (string) preg_replace( '/^[ \t\f\r\n]*auto[ \t\f\r\n]*(,[ \t\f\r\n]*)?/i', '', $sizes ); | ||||||
| } | ||||||
|
|
||||||
| // Compute more accurate sizes when it isn't lazy-loaded and sizes=auto isn't taking care of it. | ||||||
| if ( ! $is_lazy ) { | ||||||
| $computed_sizes = $this->compute_sizes( $context ); | ||||||
| if ( count( $computed_sizes ) > 0 ) { | ||||||
| $new_sizes = join( ', ', $computed_sizes ); | ||||||
| if ( '' !== $sizes ) { | ||||||
|
||||||
| if ( '' !== $sizes ) { | |
| if ( '' !== $sizes && ! $context->url_metric_group_collection->is_every_group_populated() ) { |
But it doesn't seem to hurt to keep the original at the end.
There was a problem hiding this comment.
Wouldn't the media queries generated by Image Prioritizer always cover all viewport ranges? I don't see a reason to include the original sizes if they do. Of course if only some viewport groups are populated, it would be better to have the original sizes included.
That said, I think it would be safer to only alter the sizes attribute anyway when there's sufficient samples for all viewport groups. With the media queries it includes and the original sizes media queries, I'm wary of conflicts. Since we don't want to make anything worse but only better, I would be in favor to start with only computing sizes if all viewport groups are populated.
There was a problem hiding this comment.
They would only include viewport ranges for collected URL Metrics. So if nobody gets phablet or tablet traffic, then those viewport ranges would be missing. Nevertheless, the media features computed here include the minimum and the maximum. So let's say that sizes attribute is originally (max-width: 600px) 480px, 800px from WordPress and then we prepend (width <= 480px) 400px, (width > 872px) 800px since we only have URL metrics for mobile and desktop. When these are combined with the original sizes, we get:
(width <= 480px) 400px, (width > 782px) 800px, (max-width: 600px) 480px, 800px
This means that, since sizes are processed left to right:
- Mobile viewports will get an image sized at 400px.
- Desktop viewports will get an image sized at 800px.
- Tablet and phablet visitors will fall back to the original sizes:
(max-width: 600px) 480px, 800px, which means it will be no worse than WordPress currently does by default.
Since processing happens left-to-right, and the first condition matched is used, then there won't be a conflict.
There was a problem hiding this comment.
Thanks for clarifying, that makes sense. May be worth adding an inline comment to mention that because of every viewport group providing minimum and maximum sizes, any viewport groups that are missing would simply still behave like before.
I like the idea of leaving out the original sizes value though if all viewport groups are populated, because then the original sizes would have no value at all and just look confusing in the frontend. If that's straightforward, let's add it.
There was a problem hiding this comment.
Thanks for clarifying, that makes sense. May be worth adding an inline comment to mention that because of every viewport group providing minimum and maximum sizes, any viewport groups that are missing would simply still behave like before.
Done in d2b9a5c
I like the idea of leaving out the original
sizesvalue though if all viewport groups are populated, because then the originalsizeswould have no value at all and just look confusing in the frontend. If that's straightforward, let's add it.
Done in 93961fa. (Note my original suggestion above was incorrect in that I forgot to include the negation for the second condition, which I've now fixed in my suggestion.)
Note how now the common-lcp-image-and-lazy-loaded-image-outside-viewport-with-fully-populated-sample-data test case will omit the original sizes since all viewports are populated:
Compare this with the only-mobile-and-desktop-groups-are-populated test case which retains the original sizes as a fallback since the phablet and tablet URL Metrics are absent:
Uh oh!
There was an error while loading. Please reload this page.