-
Notifications
You must be signed in to change notification settings - Fork 101
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
Lazy load videos and video posters #1596
Conversation
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. |
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.
❤️ A few suggestions.
plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php
Outdated
Show resolved
Hide resolved
plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php
Outdated
Show resolved
Hide resolved
plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php
Outdated
Show resolved
Hide resolved
|
||
$intersection_ratio = $context->url_metric_group_collection->get_element_max_intersection_ratio( $xpath ); | ||
|
||
if ( $intersection_ratio > 0 ) { |
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 wonder if $intersection_ratio > 0
is the right condition here because it could very well be that the URL Metrics are not fully populated. For example, it could be that if URL Metrics were gathered for mobile visitors in which the video is not in the viewport. For desktop visitors, the video could be in the viewport but no URL Metrics have been collected yet for it. Here the $intersection_ratio
would be 0.0
since mobile the video is not visible on any mobile visitors but this would result in the video being lazy-loaded which would end up hurting desktop visitors for whom the video is visible in the viewport.
I realize this may be a problem for lazy-loaded images and embeds as well.
To deal with this, the lazy-loading optimization should perhaps depend on the smallest viewport group (mobile) being populated as well as the largest group (desktop). But if a site only ever gets mobile or desktop visitors, then this could indefinitely delay the optimization from being applied, which isn't good. Since this is a wholistic problem/concern/question for videos, images, and embeds, it probably isn't something needing to be addressed in this PR and we can open an issue to consider further.
Related: #1595 (comment)
For prioritizing the loading of LCP images the situation is a bit different since we aren't dependent on the one fetchpriority=high
attribute being on the IMG
tag. Instead, we're able to add preload links with fetchpriority=high
that also have media queries to ensure that the populated URL Metrics will ensure the image gets loaded properly.
Also related #1404
Co-authored-by: Weston Ruter <westonruter@google.com>
// Set preload="auto" if the video is the LCP element among all viewports. | ||
$common_lcp_element = $context->url_metric_group_collection->get_common_lcp_element(); | ||
if ( null !== $common_lcp_element && $xpath === $common_lcp_element['xpath'] ) { | ||
$processor->set_attribute( 'preload', 'auto' ); | ||
return; | ||
} |
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.
Todo: this should happen even if it's not the LCP element but still in the viewport. But then only if that's the case across all viewports.
Something like this perhaps:
// Set preload="auto" if the video is the LCP element among all viewports. | |
$common_lcp_element = $context->url_metric_group_collection->get_common_lcp_element(); | |
if ( null !== $common_lcp_element && $xpath === $common_lcp_element['xpath'] ) { | |
$processor->set_attribute( 'preload', 'auto' ); | |
return; | |
} | |
// Set preload="auto" if the video is in the initial viewport among all breakpoints. | |
if ( $context->url_metric_group_collection->is_every_group_populated() && $intersection_ratio > 0 ) { | |
$processor->set_attribute( 'preload', 'auto' ); | |
return; | |
} |
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.
The problem with $context->url_metric_group_collection->is_every_group_populated()
is often tablets or phablets don't end up getting URL Metrics gathered for them. So I worry this would never end up returning true. Similar to #1404.
So maybe this should use $context->url_metrics_group_collection->is_any_group_populated()
as well?
Maybe it should be either of those two conditions. Either:
- it is the common LCP element among gathered URL metrics, or
- there are URL Metrics gathered for any group and the max intersection ratio is greater than 0.
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.
@westonruter Given that this problem is now becoming relevant in a few places and the relatively low usage of tablet devices appears to be the root of the issue, maybe we should provide an additional helper method in OD? Maybe something like is_every_critical_group_populated()
(where only certain breakpoints would be considered critical, could be filtered)? Or, we could consider giving the breakpoints more human-friendly names like phone
, tablet
, desktop
for instance, and then have a method like is_every_group_populated( array $groups )
where you could pass those you care about as a string?
is_every_group_populated()
has the problem we're seeing here, but is_any_group_populated()
doesn't sound like an adequate solution either because why would we arbitrarily consider it fine to have either desktop or mobile data fully populated?
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 like that idea. In #1602 I've proposed adding a get_first_group()
and get_last_group()
to OD_URL_Metric_Group_Collection
which would return the mobile and desktop URL metric groups respectively. Based on the low usage of tablets relative to desktop and mobile, I think in reality the critical groups will always be the first and last, representing the outer bounds of the viewport widths.
So with that PR, the result would be to determine whether the mobile and desktop groups are populated, this logic could be employed:
(
$context->url_metric_group_collection->get_first_group()->count() > 0
&&
$context->url_metric_group_collection->get_last_group()->count() > 0
)
I think this could be used here instead of $context->url_metric_group_collection->is_every_group_populated()
and $context->url_metric_group_collection->is_any_group_populated()
.
Caveat: It could be that for a particular site the traffic is almost exclusively coming by tablet visitors. If this is the case, a site owner could indicate this by making it so that there is only one group:
add_filter( 'od_breakpoint_max_widths', '__return_empty_array' );
In this case the first and last group are the same group.
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.
Looks great so far!
… fix/lazy-load-videos
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.
@swissspidy This is looking great!
On a page which has a featured image and a video with poster outside the viewport:
Before
After
* metrics collected for mobile then the VIDEO will get lazy-loaded which is good for mobile but for desktop | ||
* it will hurt performance. So this is why it is important to have URL metrics collected for both desktop and | ||
* mobile to verify whether maximum intersectionRatio is accounting for both screen sizes. | ||
* TODO: Add this same condition to IMG lazy-loading and Embed lazy-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 has been implemented in the follow-up PR: #1604
Amazing, thank you @westonruter! 🎉 |
Summary
This builds upon #1595 / #1575 which chooses a more suitable poster image size depending on the viewport dimension.
What this does:
Disable autoplay for videos outside the initial viewport and trigger autoplay using
IntersectionObserver
Fixes #1590
Remove
poster
attribute for videos outside the initial viewport, adding it back usingIntersectionObserver
.Fixes #1591
Set
preload=none
for videos outside the initial viewport, adding back the original value usingIntersectionObserver
.Fixes #1592
In my test post with 7 videos, this caused only 1 video & poster image to fully load. This massively reduces the amount of bytes loaded.
To-do:
Relevant technical choices
I looked at how Embed Optimizer does the lazy loading. It works both with and without Optimization Detective. For Image Prioritizer I am not sure it's worth adding a separate implementation that also works without OD. So I left this out for now.