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.

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

Preload image URLs for LCP elements with external background images #1697

Merged
merged 32 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
8439753
Move lazy-load script func from hooks.php to helper.php
westonruter Nov 22, 2024
cee5c24
Export webVitalsLibrarySrc to extensions
westonruter Nov 22, 2024
da12ebd
Add client-side extension to Image Prioritizer to detect LCP external…
westonruter Nov 22, 2024
cf4b6e9
Add preload links for external background images logged in URL Metrics
westonruter Nov 23, 2024
a75b94f
Prevent submitting URL Metric if viewport size changed
westonruter Nov 23, 2024
f2959cd
Introduce od_store_url_metric_validity filter so Image Prioritizer ca…
westonruter Nov 25, 2024
f4b766a
Implement get_sample_size() and get_freshness_ttl() on OD_URL_Metric_…
westonruter Nov 25, 2024
dcb7a5c
Eliminate use of static variable
westonruter Nov 25, 2024
f078723
Write up description, add warn helper function, and remove todo
westonruter Nov 25, 2024
d3339f0
Let extension initialize functions always be async
westonruter Nov 25, 2024
ecac937
Improve logic for initializing and finalizing extensions
westonruter Nov 25, 2024
494e4aa
Add missing since tags
westonruter Nov 25, 2024
1348145
Add maxLength constraint for tag name
westonruter Nov 25, 2024
94b3fee
Account for initialize or finalize not returning a Promise
westonruter Nov 25, 2024
74f9241
Improve typing for externalBackgroundImages
westonruter Nov 25, 2024
94e527b
Eliminate truncation of ID and className
westonruter Nov 25, 2024
bc81588
Merge branch 'trunk' of https://github.com/WordPress/performance into…
westonruter Nov 26, 2024
e642c99
Merge branch 'trunk' of https://github.com/WordPress/performance into…
westonruter Dec 2, 2024
d2e90d3
Increase tag max length to 100
westonruter Dec 2, 2024
f546731
Remove erroneous function from merge conflict in e642c9945
westonruter Dec 2, 2024
ec59d85
Reduce concern about unsetting array keys during iteration
westonruter Dec 2, 2024
514c513
Remove od_store_url_metric_validity filter to be re-added in follow-u…
westonruter Dec 2, 2024
fbb6076
Rename add_preload_link to add_image_preload_link
westonruter Dec 2, 2024
5c3e698
Merge branch 'trunk' of https://github.com/WordPress/performance into…
westonruter Dec 3, 2024
b4f1172
Restore viewport capturing to its original location
westonruter Dec 3, 2024
e77bd53
Merge branch 'trunk' of https://github.com/WordPress/performance into…
westonruter Dec 5, 2024
b8b79de
Clarify comment about URL metric groups not changing across a single …
felixarntz Dec 9, 2024
3ac0253
Merge branch 'trunk' of https://github.com/WordPress/performance into…
westonruter Dec 9, 2024
47d06dc
Export web-vitals callback functions to extensions rather than webVit…
westonruter Dec 9, 2024
2add133
Add tests for hooks being added
westonruter Dec 11, 2024
3abf92a
Add missing unit tests for functions
westonruter Dec 11, 2024
02c6673
Add test cases for external background images
westonruter Dec 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion plugins/embed-optimizer/detect.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ const loadedElementContentRects = new Map();
* @type {InitializeCallback}
* @param {InitializeArgs} args Args.
*/
export function initialize( { isDebug } ) {
export async function initialize( { isDebug } ) {
/** @type NodeListOf<HTMLDivElement> */
const embedWrappers = document.querySelectorAll(
'.wp-block-embed > .wp-block-embed__wrapper[data-od-xpath]'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@
/**
* Tag visitor that optimizes elements with background-image styles.
*
* @phpstan-type LcpElementExternalBackgroundImage array{
* url: non-empty-string,
* tag: non-empty-string,
* id: string|null,
* class: string|null,
* }
*
* @since 0.1.0
* @access private
*/
Expand All @@ -35,6 +42,14 @@ final class Image_Prioritizer_Background_Image_Styled_Tag_Visitor extends Image_
*/
private $added_lazy_assets = false;

/**
* Tuples of URL Metric group and the common LCP element external background image.
*
* @since n.e.x.t
* @var array<array{OD_URL_Metric_Group, LcpElementExternalBackgroundImage}>
*/
private $group_common_lcp_element_external_background_images;

/**
* Visits a tag.
*
Expand Down Expand Up @@ -65,33 +80,128 @@ public function __invoke( OD_Tag_Visitor_Context $context ): bool {
}

if ( is_null( $background_image_url ) ) {
$this->maybe_preload_external_lcp_background_image( $context );
return false;
}

$xpath = $processor->get_xpath();

// If this element is the LCP (for a breakpoint group), add a preload link for it.
foreach ( $context->url_metric_group_collection->get_groups_by_lcp_element( $xpath ) as $group ) {
Copy link
Member

@felixarntz felixarntz Dec 9, 2024

Choose a reason for hiding this comment

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

This is missing the fix we're including via #1723. I think we should merge that PR since it's more of a fix while this is an enhancement, and then update this PR to follow suit. Otherwise we're introducing the same problem for LCP background images that #1723 is fixing for the LCP generally.

This re-emphasizes to me what I mentioned in my previous point: If we were using Optimization Detective's own LCP handling, we would be able to get this kind of correct behavior "for free", rather than having to re-implement it here too. So I'm curious why we're not doing that.

At a minimum, this needs to be fixed here to be aligned with what #1723 does.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is missing the fix we're including via #1723.

I'm not sure this is the case. The fix in #1723 is to allow fetchpriority=high to be on the IMG element when only the mobile and desktop viewport groups have URL Metrics collected. Preload links are still only being added for IMG for the actual viewport groups that have URL metrics collected for them:

foreach ( $context->url_metric_group_collection->get_groups_by_lcp_element( $xpath ) as $group ) {
$context->link_collection->add_link(
$attributes,
$group->get_minimum_viewport_width(),
$group->get_maximum_viewport_width()
);
}

Or are you saying that if the mobile and desktop viewport groups have the same LCP element, that we should go ahead and also preload the same URL for that LCP Element even for phablet and tablet when no URL Metrics are yet confirming that they also have the same LCP element?

This re-emphasizes to me what I mentioned in my previous point: If we were using Optimization Detective's own LCP handling, we would be able to get this kind of correct behavior "for free", rather than having to re-implement it here too. So I'm curious why we're not doing that.

I'm not sure I understand. For elements which are tracked in URL Metrics (and an element appears in elements when there is a tag visitor which returns true for a tag during iterator), then as part of the stored metadata for that tracked element will be whether it isLCP. So here it's looking up which viewport groups have the current tag as the LCP element, and for each one a preload link is added.

$link_attributes = array(
'rel' => 'preload',
'fetchpriority' => 'high',
'as' => 'image',
'href' => $background_image_url,
'media' => 'screen',
);

$context->link_collection->add_link(
$link_attributes,
$group->get_minimum_viewport_width(),
$group->get_maximum_viewport_width()
);
$this->add_image_preload_link( $context->link_collection, $group, $background_image_url );
}

$this->lazy_load_bg_images( $context );

return true;
}

/**
* Gets the common LCP element external background image for a URL Metric group.
*
* @since n.e.x.t
*
* @param OD_URL_Metric_Group $group Group.
* @return LcpElementExternalBackgroundImage|null
*/
private function get_common_lcp_element_external_background_image( OD_URL_Metric_Group $group ): ?array {

// If the group is not fully populated, we don't have enough URL Metrics to reliably know whether the background image is consistent across page loads.
// This is intentionally not using $group->is_complete() because we still will use stale URL Metrics in the calculation.
if ( $group->count() !== $group->get_sample_size() ) {
return null;
}

$previous_lcp_element_external_background_image = null;
foreach ( $group as $url_metric ) {
/**
* Stored data.
*
* @var LcpElementExternalBackgroundImage|null $lcp_element_external_background_image
*/
$lcp_element_external_background_image = $url_metric->get( 'lcpElementExternalBackgroundImage' );
if ( ! is_array( $lcp_element_external_background_image ) ) {
return null;
}
if ( null !== $previous_lcp_element_external_background_image && $previous_lcp_element_external_background_image !== $lcp_element_external_background_image ) {
return null;
}
$previous_lcp_element_external_background_image = $lcp_element_external_background_image;
}

return $previous_lcp_element_external_background_image;
}

/**
* Maybe preloads external background image.
*
* @since n.e.x.t
*
* @param OD_Tag_Visitor_Context $context Context.
*/
private function maybe_preload_external_lcp_background_image( OD_Tag_Visitor_Context $context ): void {
// Gather the tuples of URL Metric group and the common LCP element external background image.
felixarntz marked this conversation as resolved.
Show resolved Hide resolved
// Note the groups of URL Metrics do not change across invocations, we just need to compute this once for all.
if ( ! is_array( $this->group_common_lcp_element_external_background_images ) ) {
$this->group_common_lcp_element_external_background_images = array();
foreach ( $context->url_metric_group_collection as $group ) {
$common = $this->get_common_lcp_element_external_background_image( $group );
if ( is_array( $common ) ) {
$this->group_common_lcp_element_external_background_images[] = array( $group, $common );
}
}
}
felixarntz marked this conversation as resolved.
Show resolved Hide resolved

// There are no common LCP background images, so abort.
if ( count( $this->group_common_lcp_element_external_background_images ) === 0 ) {
return;
}

$processor = $context->processor;
$tag_name = strtoupper( (string) $processor->get_tag() );
foreach ( array_keys( $this->group_common_lcp_element_external_background_images ) as $i ) {
list( $group, $common ) = $this->group_common_lcp_element_external_background_images[ $i ];
if (
// Note that the browser may send a lower-case tag name in the case of XHTML or embedded SVG/MathML, but
// the HTML Tag Processor is currently normalizing to all upper-case. The HTML Processor on the other
// hand may return the expected case.
strtoupper( $common['tag'] ) === $tag_name
&&
$processor->get_attribute( 'id' ) === $common['id'] // May be checking equality with null.
&&
$processor->get_attribute( 'class' ) === $common['class'] // May be checking equality with null.
felixarntz marked this conversation as resolved.
Show resolved Hide resolved
) {
$this->add_image_preload_link( $context->link_collection, $group, $common['url'] );

// Now that the preload link has been added, eliminate the entry to stop looking for it while iterating over the rest of the document.
unset( $this->group_common_lcp_element_external_background_images[ $i ] );
Copy link
Member

Choose a reason for hiding this comment

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

Can this not impact the foreach loop in a problematic way, given this same array we're iterating through? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I had wondered that as well, but I found that foreach() operates on a copy of the array, so it is safe to unset keys in the loop.

Examples:
https://3v4l.org/OOlpa
https://3v4l.org/oQF2h

But this could be made less concerning upon inspection if this iterates over array_keys() instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in ec59d85

}
}
}

/**
* Adds an image preload link for the group.
*
* @since n.e.x.t
*
* @param OD_Link_Collection $link_collection Link collection.
* @param OD_URL_Metric_Group $group URL Metric group.
* @param non-empty-string $url Image URL.
*/
private function add_image_preload_link( OD_Link_Collection $link_collection, OD_URL_Metric_Group $group, string $url ): void {
$link_collection->add_link(
array(
'rel' => 'preload',
'fetchpriority' => 'high',
'as' => 'image',
'href' => $url,
'media' => 'screen',
),
$group->get_minimum_viewport_width(),
$group->get_maximum_viewport_width()
);
}

/**
* Optimizes an element with a background image based on whether it is displayed in any initial viewport.
*
Expand Down
Loading
Loading