Compute responsive sizes attribute based on the width from the boundingClientRect in captured URL Metrics
#1840
Conversation
westonruter
added
[Type] Enhancement
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
westonruter
requested review from
joemcgill,
swissspidy,
adamsilverstein and
mukeshpanchal27
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #1840 +/- ##
==========================================
+ Coverage 66.36% 66.55% +0.18%
==========================================
Files 88 88
Lines 6975 6999 +24
==========================================
+ Hits 4629 4658 +29
+ Misses 2346 2341 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| // 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 ) ); | ||
| } |
There was a problem hiding this comment.
Now that #1839 has been merged, the resulting source size list has overlapping max/min values, for example:
(max-width: 480px) 432px,
(min-width: 480px and max-width: 600px) 540px,
(min-width: 600px and max-width: 782px) 704px,
(min-width: 782px) 900px
Nevertheless, this should be fine since sizes processing stops at the first condition that matches.
However, if there aren't URL Metrics captured for a viewport group (e.g. mobile), then it could be that if the viewport is exactly 480px wide, then an image would be selected for what is intended to be the phablet viewport (which starts from 480px exclusive). If there is a different layout for the image at the next breakpoint, then this could result in the unexpected image size being chosen. For example, on desktop when images are in the columns block they appear narrower than they do on non-desktop in which case they are vertically stacked, so it's not the case that a wider viewport always results in a wider image.
To address this issue, it would seem that the CSS range syntax from #1833 should be used in the sizes attribute.
| $computed_sizes = $this->compute_sizes( $context ); | ||
| if ( count( $computed_sizes ) > 0 ) { | ||
| $new_sizes = join( ', ', $computed_sizes ); | ||
| if ( '' !== $sizes ) { |
There was a problem hiding this comment.
Should this be preserving the original sizes at the end? It can result in some conditions that are never matched. For example, if the original sizes attribute contains (max-width: 600px) 480px, 800px and then we prefix this value with:
(max-width: 480px) 432px,
(min-width: 480px and max-width: 600px) 540px,
(min-width: 600px and max-width: 782px) 704px,
(min-width: 782px) 900px
Then the original conditions will never apply because (min-width: 782px) 900px is matched first. The only scenario where they could apply is if there aren't URL Metrics gathered for some of the viewport widths, and in this case leaving the original in place would serve as a fallback. Otherwise, we could remove it if all viewport groups are populated:
| 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:
westonruter
changed the title
sizes attribute based on the width from the boundingClientRect in captured URL Metricssizes attribute based on the width from the boundingClientRect in captured URL Metrics
| $outside_viewport_rect = array_merge( | ||
| $this->get_sample_dom_rect(), | ||
| array( | ||
| 'top' => 1000, | ||
| ) | ||
| ); | ||
|
|
There was a problem hiding this comment.
This is removed because now in plugins/optimization-detective/tests/class-optimization-detective-test-helpers.php it will handle sparse boundingClientRect by merging on top of a full one.
There was a problem hiding this comment.
The other changes similar changes in this PR are for the same reason.
| ), | ||
| // All are outside all initial viewports. | ||
| array( | ||
| 'xpath' => '/HTML/BODY/DIV[@id=\'page\']/*[5][self::IMG]', | ||
| 'isLCP' => false, | ||
| 'intersectionRatio' => 0.0, | ||
| 'intersectionRect' => $outside_viewport_rect, |
There was a problem hiding this comment.
Note that the intersectionRect now automatically is populated to be zeroed out when the interectionRatio is 0. Previously it was not making sense to have a DOMRect containing non-zero values when the intersectionRatio was zero.
There was a problem hiding this comment.
@westonruter This looks quite good. A few questions and feedback.
plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php
Outdated
Show resolved
Hide resolved
plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php
Outdated
Show resolved
Hide resolved
| $computed_sizes = $this->compute_sizes( $context ); | ||
| if ( count( $computed_sizes ) > 0 ) { | ||
| $new_sizes = join( ', ', $computed_sizes ); | ||
| if ( '' !== $sizes ) { |
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.
Co-authored-by: felixarntz <[email protected]>
plugins/image-prioritizer/readme.txt
Outdated
| @@ -27,7 +27,9 @@ The current optimizations include: | |||
| 1. Apply lazy loading to `IMG` tags based on whether they appear in any breakpoint’s initial viewport. | |||
| 2. Implement lazy loading of CSS background images added via inline `style` attributes. | |||
| 3. Lazy-load `VIDEO` tags by setting the appropriate attributes based on whether they appear in the initial viewport. If a `VIDEO` is the LCP element, it gets `preload=auto`; if it is in an initial viewport, the `preload=metadata` default is left; if it is not in an initial viewport, it gets `preload=none`. Lazy-loaded videos also get initial `preload`, `autoplay`, and `poster` attributes restored when the `VIDEO` is going to enter the viewport. | |||
| 5. Ensure that [`sizes=auto`](https://make.wordpress.org/core/2024/10/18/auto-sizes-for-lazy-loaded-images-in-wordpress-6-7/) is added to all lazy-loaded `IMG` elements. | |||
| 5. Responsive image sizes: | |||
| 1. Ensure [`sizes=auto`](https://make.wordpress.org/core/2024/10/18/auto-sizes-for-lazy-loaded-images-in-wordpress-6-7/) is added to all lazy-loaded `IMG` elements. | |||
There was a problem hiding this comment.
I know this text isn't new, but might be worth clarifying why that's important. Based on how it's currently phrased here, people might argue "But WordPress Core already does this!".
So maybe this can be rephrased to clarify that it's about ensuring that sizes=auto is applied correctly after Image Prioritizer has improved/fixed lazy-loading of images as done by WordPress.
Since the sizes=auto part isn't really a feature this plugin does better than WordPress (it's the lazy-loading portion it does better than WordPress, but not how it applies sizes=auto), I would put the point of computing the correct sizes above this, as it's a more powerful feature of the plugin.
There was a problem hiding this comment.
Like so?
| 1. Ensure [`sizes=auto`](https://make.wordpress.org/core/2024/10/18/auto-sizes-for-lazy-loaded-images-in-wordpress-6-7/) is added to all lazy-loaded `IMG` elements. | |
| 1. Ensure [`sizes=auto`](https://make.wordpress.org/core/2024/10/18/auto-sizes-for-lazy-loaded-images-in-wordpress-6-7/) is set on `IMG` tags after setting correct lazy-loading (above). |
There was a problem hiding this comment.
Yeah, plus moving it to #2 in the list as mentioned :)
There was a problem hiding this comment.
Ah yes! Done: 4d0251a
Co-authored-by: felixarntz <[email protected]>
Summary
Given a post that contains a columns block with three images in it, which are displayed without columns on a non-desktop viewport:
Block markup
By default, much larger versions of these images are downloaded on desktop than are necessary. For example, even though the first image on desktop only has a rendered size of 136x90, the selected image is much larger at 1536x1010. Nevertheless, the
srcsethas a much smaller 300x197 pixel image available which would be suitable for rendering instead. The selected image here is 458 KB but the appropriately-sized image is much less at just 22 KB. This represents a significant amount of wasted bytes. This is a difficult problem to solve (as seen in #760) since WordPress does not have layout information readily available to know how large the image will actually be when the page is served across various device form factors. Nevertheless, this information is available on sites using Optimization Detective.When visitors load a page, the Optimization Detective plugin's detection logic can capture the dimensions of the
IMGelements on the page (specifically theboundingClientRectvia Intersection Observer). This includes thewidthof theIMGelement on the rendered page for the user's device. This information is then stored in URL Metrics which are put into groups for mobile, phablet, tablet, and desktop. With this URL Metric information stored, when a page is loaded the plugin's optimization logic can use that information to apply relevant performance changes, such as to calculate thesizesbased on the collected URL Metrics. This PR explores implementing this as part of the Image Prioritizer extension for Optimization Detective.Note that Optimization Detective was similarly used to pick a more appropriate image size for a video's
posterin #1595 for #1575.This renders
sizesin pixel values, so it may not be as precise/dynamic as what is being explored in #1511.The following results are specifically looking at desktop. There is no reduction in bytes on mobile because WordPress displays these images as full width on a mobile viewport, and WordPress's default
sizesattribute already selects the correct size in that case.Results
Hovering over the
IMGelements in DevTools:Images downloaded in Network panel show a ~90% reduction in downloaded bytes:
Before:
After:
Rendered Markup
Note the change to the
sizesattribute.Before
(This is with Optimization Detective and Image Prioritizer completely disabled by adding
?optimization_detective_disabledto the URL.)After
Properly Size Images Lighthouse Audit