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,38 @@ 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.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| if ( $element_max_width > 0 ) { | ||||||
| $size = sprintf( '%dpx', $element_max_width ); | ||||||
|
|
||||||
| $media_feature = od_generate_media_query( $group->get_minimum_viewport_width(), $group->get_maximum_viewport_width() ); | ||||||
| if ( null !== $media_feature ) { | ||||||
| $size = "$media_feature $size"; | ||||||
| } | ||||||
| $sizes[] = $size; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return $sizes; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Process an IMG element. | ||||||
| * | ||||||
|
|
@@ -146,21 +178,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 ) { | ||||||
|
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 Then the original conditions will never apply because
Suggested change
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 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 This means that, since
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 There was a problem hiding this comment.
Done in d2b9a5c
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 Compare this with the |
||||||
| $new_sizes .= ", $sizes"; | ||||||
| } | ||||||
| $sizes = $new_sizes; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| $processor->set_attribute( 'sizes', $sizes ); | ||||||
| } | ||||||
|
|
||||||
| $parent_tag = $this->get_parent_tag_name( $context ); | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.