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 10 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,11 +14,25 @@
/**
* 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
*/
final class Image_Prioritizer_Background_Image_Styled_Tag_Visitor extends Image_Prioritizer_Tag_Visitor {

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

/**
* Visits a tag.
*
Expand Down Expand Up @@ -49,28 +63,121 @@ 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(
$this->add_preload_link( $context->link_collection, $group, $background_image_url );
}

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
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 ( $this->group_common_lcp_element_external_background_images as $i => list( $group, $common ) ) {
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_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_preload_link( OD_Link_Collection $link_collection, OD_URL_Metric_Group $group, string $url ): void {
Copy link
Member

Choose a reason for hiding this comment

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

Consider adding this as a public method on OD_Link_Collection directly (then without the first parameter of course), wrapping its add_link() method.

Regardless of whether it remains here or there, I think it would be good to rename it to add_image_preload_link(), as it's only for images but that's not obvious from the method name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. This is closely related to the direction that #1707 is going as well.

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've just renamed it for now in fbb6076.

It's more complicated to make a general method for image preloading because a link may not have an href at all in the case of responsive preloads in which imagesrcset and imagesizes would be used instead (or in addition). It's simpler here for the background image case because it's just a single possible URL value as the href.

I'll keep this in mind for a future enhancement for facilitating image preloads, perhaps as part of #1707 assuming this PR is merged first, or else a separate PR.

$link_collection->add_link(
array(
'rel' => 'preload',
'fetchpriority' => 'high',
'as' => 'image',
'href' => $background_image_url,
'href' => $url,
'media' => 'screen',
);

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

return true;
),
$group->get_minimum_viewport_width(),
$group->get_maximum_viewport_width()
);
}
}
206 changes: 206 additions & 0 deletions plugins/image-prioritizer/detect.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,206 @@
/**
* Image Prioritizer module for Optimization Detective
*
* This extension to Optimization Detective captures the LCP element's CSS background image which is not defined with
* an inline style attribute but rather in either an external stylesheet loaded with a LINK tag or by stylesheet in
* a STYLE element. The URL for this LCP background image and the tag's name, ID, and class are all amended to the
* stored URL Metric so that a responsive preload link with fetchpriority=high will be added for that background image
* once a URL Metric group is fully populated with URL Metrics that all agree on that being the LCP image, and if the
* document has a tag with the same name, ID, and class.
*/

const consoleLogPrefix = '[Image Prioritizer]';

/**
* Detected LCP external background image candidates.
*
* @type {Array<{url: string, tag: string, id: string, class: string}>}
*/
const externalBackgroundImages = [];

/**
* @typedef {import("web-vitals").LCPMetric} LCPMetric
* @typedef {import("../optimization-detective/types.ts").InitializeCallback} InitializeCallback
* @typedef {import("../optimization-detective/types.ts").InitializeArgs} InitializeArgs
* @typedef {import("../optimization-detective/types.ts").FinalizeArgs} FinalizeArgs
* @typedef {import("../optimization-detective/types.ts").FinalizeCallback} FinalizeCallback
*/

/**
* Logs a message.
*
* @since n.e.x.t
*
* @param {...*} message
*/
function log( ...message ) {
// eslint-disable-next-line no-console
console.log( consoleLogPrefix, ...message );
}

/**
* Logs a warning.
*
* @since n.e.x.t
*
* @param {...*} message
*/
function warn( ...message ) {
// eslint-disable-next-line no-console
console.warn( consoleLogPrefix, ...message );
}

/**
* Initializes extension.
*
* @since n.e.x.t
*
* @type {InitializeCallback}
* @param {InitializeArgs} args Args.
*/
export async function initialize( { isDebug, webVitalsLibrarySrc } ) {
const { onLCP } = await import( webVitalsLibrarySrc );
onLCP(
( /** @type {LCPMetric} */ metric ) => {
handleLCPMetric( metric, isDebug );
},
{
// This avoids needing to click to finalize LCP candidate. While this is helpful for testing, it also
// ensures that we always get an LCP candidate reported. Otherwise, the callback may never fire if the
// user never does a click or keydown, per <https://github.com/GoogleChrome/web-vitals/blob/07f6f96/src/onLCP.ts#L99-L107>.
reportAllChanges: true,
adamsilverstein marked this conversation as resolved.
Show resolved Hide resolved
}
);
}

/**
* Gets the performance resource entry for a given URL.
*
* @since n.e.x.t
*
* @param {string} url - Resource URL.
* @return {PerformanceResourceTiming|null} Resource entry or null.
*/
function getPerformanceResourceByURL( url ) {
const entries =
/** @type PerformanceResourceTiming[] */ performance.getEntriesByType(
'resource'
);
for ( const entry of entries ) {
if ( entry.name === url ) {
return entry;
}
}
return null;
}

/**
* Handles a new LCP metric being reported.
*
* @since n.e.x.t
*
* @param {LCPMetric} metric - LCP Metric.
* @param {boolean} isDebug - Whether in debug mode.
*/
function handleLCPMetric( metric, isDebug ) {
for ( const entry of metric.entries ) {
// Look only for LCP entries that have a URL and a corresponding element which is not an IMG or VIDEO.
if (
! entry.url ||
! ( entry.element instanceof HTMLElement ) ||
entry.element instanceof HTMLImageElement ||
entry.element instanceof HTMLVideoElement
) {
continue;
}

// Always ignore data: URLs.
if ( entry.url.startsWith( 'data:' ) ) {
continue;
}

// Skip elements that have the background image defined inline.
// These are handled by Image_Prioritizer_Background_Image_Styled_Tag_Visitor.
if ( entry.element.style.backgroundImage ) {
continue;
}

// Now only consider proceeding with the URL if its loading was initiated with stylesheet or preload link.
const resourceEntry = getPerformanceResourceByURL( entry.url );
if (
! resourceEntry ||
! [ 'css', 'link' ].includes( resourceEntry.initiatorType )
) {
if ( isDebug ) {
warn(
`Skipped considering URL (${ entry.url }) due to unexpected performance resource timing entry:`,
resourceEntry
);
}
return;
}

// Skip URLs that are excessively long. This is the maxLength defined in image_prioritizer_add_element_item_schema_properties().
if ( entry.url.length > 500 ) {
if ( isDebug ) {
log( `Skipping very long URL: ${ entry.url }` );
}
return;
}

// Note that getAttribute() is used instead of properties so that null can be returned in case of an absent attribute.
let id = entry.element.getAttribute( 'id' );
if ( null !== id ) {
id = id.substring( 0, 100 ); // This is the maxLength defined in image_prioritizer_add_element_item_schema_properties().
}
let className = entry.element.getAttribute( 'class' );
if ( null !== className ) {
className = className.substring( 0, 500 ); // This is the maxLength defined in image_prioritizer_add_element_item_schema_properties().
}

// The id and className allow the tag visitor to detect whether the element is still in the document.
// This is used instead of having a full XPath which is likely not available since the tag visitor would not
// know to return true for this element since it has no awareness of which elements have external backgrounds.
const externalBackgroundImage = {
url: entry.url,
tag: entry.element.tagName,
id,
class: className,
};

if ( isDebug ) {
log(
'Detected external LCP background image:',
externalBackgroundImage
);
}

externalBackgroundImages.push( externalBackgroundImage );
}
}

/**
* Finalizes extension.
*
* @since n.e.x.t
*
* @type {FinalizeCallback}
* @param {FinalizeArgs} args Args.
*/
export async function finalize( { extendRootData, isDebug } ) {
if ( externalBackgroundImages.length === 0 ) {
return;
}

// Get the last detected external background image which is going to be for the LCP element (or very likely will be).
const lcpElementExternalBackgroundImage = externalBackgroundImages.pop();

if ( isDebug ) {
log(
'Sending external background image for LCP element:',
lcpElementExternalBackgroundImage
);
}

extendRootData( { lcpElementExternalBackgroundImage } );
}
Loading