Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Prioritize loading poster image of video LCP elements #1498

Merged
merged 10 commits into from
Oct 15, 2024
Merged

Prioritize loading poster image of video LCP elements #1498

merged 10 commits into from
Oct 15, 2024

Conversation

thelovekesh
Copy link
Member

Summary

Fixes #1183

Relevant technical choices

  • Preload poster images with high-priority video LCP elements.

@thelovekesh thelovekesh added [Type] Feature A new feature within an existing module [Plugin] Image Prioritizer Issues for the Image Prioritizer plugin (dependent on Optimization Detective) labels Aug 25, 2024
@thelovekesh thelovekesh added this to the image-prioritizer n.e.x.t milestone Aug 25, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 25, 2024 •
edited
Loading

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: thelovekesh <thelovekesh@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@thelovekesh thelovekesh mentioned this pull request Aug 25, 2024
Closed
@thelovekesh
Copy link
Member Author

TODO: Add test cases.

@westonruter
Copy link
Member

Yeah, sounds good.

@westonruter westonruter mentioned this pull request Oct 15, 2024
Closed
@westonruter westonruter added [Type] Enhancement A suggestion for improvement of an existing feature and removed [Type] Feature A new feature within an existing module labels Oct 15, 2024
westonruter
westonruter reviewed Oct 15, 2024
View reviewed changes
plugins/image-prioritizer/class-image-prioritizer-tag-visitor.php
* @param string $attribute_name Attribute name.
* @return string|true|null Normalized attribute value.
*/
protected function get_attribute_value( OD_HTML_Tag_Processor $processor, string $attribute_name ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to move this eventually into OD_HTML_Tag_Processor/OD_HTML_Processor. Also @dmsnell has suggested this could eventually be part of the core class as well, as most HTML attributes have their own dedicated logic to get back a normalized value.

swissspidy
swissspidy reviewed Oct 15, 2024
View reviewed changes
plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php
*
* @return bool Whether the tag should be tracked in URL metrics.
*/
public function __invoke( OD_Tag_Visitor_Context $context ): bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter I suppose this is where I would add the logic for #1575?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes exactly!

swissspidy
swissspidy reviewed Oct 15, 2024
View reviewed changes
plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php

$xpath = $processor->get_xpath();

// TODO: If $context->url_metric_group_collection->get_element_max_intersection_ratio( $xpath ) is 0.0, then the video is not in any initial viewport and the VIDEO tag could get the preload=none attribute added.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking out loud for a future enhancement:

autoplay takes precedence over preload. Since we here know that the video is not in the initial viewport, we could remove autoplay in favor of an IntersectionObserver-based implementation that only autoplays the video when actually visible.

Thoughts? Worth creating an issue for it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, interesting idea. Yes, I think that warrants an issue.

Related to #1035 which involves IntersectionObserver to lazy-load background images.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: #1590

swissspidy
swissspidy approved these changes Oct 15, 2024
View reviewed changes
Copy link
Member

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks pretty good, seems like a great base for further enhancements like #1575

@westonruter westonruter merged commit c173a50 into trunk Oct 15, 2024
18 of 20 checks passed
@westonruter westonruter deleted the add/lcp-poster-image-prioritize branch October 15, 2024 18:46
@swissspidy swissspidy mentioned this pull request Oct 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swissspidy swissspidy swissspidy approved these changes

@westonruter westonruter westonruter left review comments

@felixarntz felixarntz Awaiting requested review from felixarntz felixarntz is a code owner

Assignees
No one assigned
Labels
[Plugin] Image Prioritizer Issues for the Image Prioritizer plugin (dependent on Optimization Detective) [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Milestone
image-prioritizer 0.2.0
Development

Successfully merging this pull request may close these issues.

Prioritize loading poster image of video LCP elements
3 participants
@thelovekesh @westonruter @swissspidy