-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize lazy-loading of IMG elements in Image Prioritizer #1261
Conversation
35850bd
to
7b781fb
Compare
plugins/optimization-detective/class-od-url-metrics-group-collection.php
Show resolved
Hide resolved
43403a5
to
c0ed712
Compare
$walker->set_attribute( 'loading', 'lazy' ); | ||
} | ||
} | ||
// TODO: If an image is the LCP element for one breakpoint but not another, add loading=lazy. This won't hurt performance since the image is being preloaded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is, if the image is not the LCP element for all breakpoints and the image is not in the initial viewport.
$walker->remove_attribute( 'fetchpriority' ); | ||
} | ||
|
||
// TODO: If the image is visible (intersectionRatio!=0) in any of the URL metrics, remove loading=lazy. | ||
// TODO: Conversely, if an image is the LCP element for one breakpoint but not another, add loading=lazy. This won't hurt performance since the image is being preloaded. | ||
// TODO: Also if the element isLCPCandidate it should never by lazy-loaded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in this case, clearly the intersectionRatio
will be non-zero. So this is redundant.
c0ed712
to
9527d38
Compare
9527d38
to
9389a98
Compare
} else { | ||
$walker->set_attribute( 'fetchpriority', 'high' ); | ||
$walker->set_attribute( 'data-od-added-fetchpriority', true ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This data-od-
meta attribute is now added automatically when calling set_attribute
on the walker instance.
// Never include loading=lazy on the LCP image common across all breakpoints. | ||
if ( 'lazy' === $walker->get_attribute( 'loading' ) ) { | ||
$walker->set_attribute( 'data-od-removed-loading', $walker->get_attribute( 'loading' ) ); | ||
$walker->remove_attribute( 'loading' ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is obsolete because the loading
attribute is now removed for any element which has an non-zero intersectionRatio
in any viewport group.
$walker->set_attribute( 'loading', 'lazy' ); | ||
} | ||
} | ||
// TODO: If an image is visible in one breakpoint but not another, add loading=lazy AND add a regular-priority preload link with media queries (unless LCP in which case it should already have a fetchpriority=high link) so that the image won't be eagerly-loaded for viewports on which it is not shown. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be filing an issue for this. It may be a bit confusing when looking at the markup because they may well see an LCP img
element which has loading=lazy
, but it will still get prioritized via the high-fetchpriority preload link (with a media query). The Lighthouse audit will need to be updated to account for this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my testing at #117 (comment) I found that adding loading=lazy
to the LCP image does not degrade performance if there is a preload link with fetchpriority=high
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened this as #1270
<img data-od-removed-fetchpriority="high" src="https://example.com/bar.jpg" alt="Bar" width="10" height="10" loading="lazy" > | ||
<p>Pretend this is a super long paragraph that pushes the next image mostly out of the initial viewport.</p> | ||
<img data-od-removed-fetchpriority="high" data-od-removed-loading="lazy" src="https://example.com/bar.jpg" alt="Bar" width="10" height="10" > | ||
<p>Now the following image is definitely outside the initial viewport.</p> | ||
<img data-od-added-loading data-od-removed-fetchpriority="high" loading="lazy" src="https://example.com/baz.jpg" alt="Baz" width="10" height="10" > | ||
<img data-od-removed-fetchpriority="high" data-od-replaced-loading="eager" src="https://example.com/qux.jpg" alt="Qux" width="10" height="10" loading="lazy"> | ||
<img src="https://example.com/quux.jpg" alt="Quux" width="10" height="10" loading="lazy"><!-- This one is all good. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the key test showing how the new functionality is optimizing lazy-loading. Compare this markup with the input markup above.
@@ -553,6 +579,7 @@ static function () use ( $mobile_breakpoint, $tablet_breakpoint ): array { | |||
', | |||
), | |||
|
|||
// TODO: Eventually the images in this test should all be lazy-loaded, leaving the prioritization to the preload links. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to be done in this PR. See https://github.com/WordPress/performance/pull/1261/files#r1625015100
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. |
* @param string|true $value Value. | ||
* @return bool Whether an attribute was set. | ||
*/ | ||
public function set_meta_attribute( string $name, $value ): bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I love the term "meta" here, but can't think of a better term
Co-authored-by: Pascal Birchler <[email protected]>
Co-authored-by: Pascal Birchler <[email protected]>
Fixes #1077
This implements optimization of lazy-loading based on the
intersectionRatio
of each image captured in the URL metrics. In particular, if an image is visible in the initial viewport for any viewport size, thenloading=lazy
should be omitted. In contrast, if an image is not visible in any viewport, thenloading=lazy
should be added.This PR also improves setting the
data-od-
meta attributes by automatically setting them when callingOD_HTML_Tag_Walker::set_attribute()
andOD_HTML_Tag_Walker::remove_attribute()
. These attributes provide a window into what operations were performed to optimize the page. Additionally, anOD_HTML_Tag_Walker::set_meta_attribute()
method is added which allows you to set anydata-od-
-prefixed attribute for additional diagnostics.This also takes an initial stab at writing the readme for Image Prioritizer, and updating the readme for Optimization Detective to account for the new dependent plugin.