Preload image URLs for LCP elements with external background images #1697
Conversation
westonruter
added
[Type] Enhancement
|
@felixarntz This is getting close! I'm excited about this one since it unlocks a very common use case which we've seen especially among page builders. This unlocks potential optimization of the |
|
I've just updated the description with findings on how this impacts Elementor. In short, I'm seeing a ~20% improvement in LCP on both desktop and mobile! |
…n validate background-image URL
westonruter
force-pushed
the
add/external-bg-preload
branch
from
c872c1b to
f2959cd
Compare
| }; | ||
|
|
||
| export type InitializeCallback = ( args: InitializeArgs ) => void; | ||
| export type InitializeCallback = ( args: InitializeArgs ) => Promise< void >; |
There was a problem hiding this comment.
This is now consistent with the FinalizeCallback.
There was a problem hiding this comment.
@westonruter This looks solid as far as I can tell, but I have a few questions as it's a large and complex PR to review.
plugins/image-prioritizer/class-image-prioritizer-background-image-styled-tag-visitor.php
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 ] ); |
There was a problem hiding this comment.
Can this not impact the foreach loop in a problematic way, given this same array we're iterating through? 🤔
There was a problem hiding this comment.
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.
plugins/image-prioritizer/class-image-prioritizer-background-image-styled-tag-visitor.php
Show resolved
Hide resolved
| * @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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good idea. This is closely related to the direction that #1707 is going as well.
There was a problem hiding this comment.
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.
plugins/image-prioritizer/helper.php
Outdated
| * | ||
| * Handles 'autoplay' and 'preload' attributes accordingly. | ||
| * | ||
| * @since 0.2.0 |
There was a problem hiding this comment.
Where does this function come from? It seems unrelated and the PR diff makes it look like it's new? 🤔
plugins/image-prioritizer/class-image-prioritizer-background-image-styled-tag-visitor.php
Show resolved
Hide resolved
…page load. Co-authored-by: Weston Ruter <[email protected]>
There was a problem hiding this comment.
@westonruter I may be realizing this a bit late, but now I'm curious why this is not more closely tying into what Optimization Detective already provides.
| * @return array<string, array{type: string}> Additional properties. | ||
| */ | ||
| function image_prioritizer_add_element_item_schema_properties( array $additional_properties ): array { | ||
| $additional_properties['lcpElementExternalBackgroundImage'] = array( |
There was a problem hiding this comment.
Maybe I should have asked this before... But why are we introducing this in Image Prioritizer, when Optimization Detective already has awareness of the LCP element?
It feels like we're duplicating a bit of what Optimization Detective already handles (e.g. in the detect.js script the onLCP etc.). Why not enhance this via Optimization Detective? Or provide a more targeted extension point for the LCP element specifically that Image Prioritizer could leverage?
There was a problem hiding this comment.
Humm. Well, the normal case for how data is collected in Optimization Detective is for a tag visitor to mark tags of interest (which result in data-od-xpath attributes being emitted on them on the frontend), and then for OD's detect.js to collect data about those specific elements to then send back for storage with the URL Metric.
What is needed here is different because we don't know what the element of interest will be, since any element could be LCP with a background image applied with an external stylesheet.
So that's why this new lcpElementExternalBackgroundImage root property is being introduced in the URL Metric schema. There is no XPath because the element is not explicitly visited by tag visitors, so it doesn't necessarily appear in elements. Now, if it just so happens that there is a tag which shows up in elements (which was processed by a tag visitor) which is also the LCP element... it would make sense to store the LCP metric's url there as well. However, this would be coincidental and not something we can rely on specifically for optimizing non-inline background images in LCP elements.
In regards to onLCP, we could potentially expose that onLCP callback (in addition to onCLS, onFCP, onINP, and onTTFB) to the initialize function for each extension. This would eliminate the need to pass the webVitalsLibrarySrc.
There was a problem hiding this comment.
I may be missing some understanding here, so apologies in advance. But don't we only care about the background image that's relevant to the LCP element?
So from that perspective, couldn't we have an extension point that allows extensions to expand what data is collected for the LCP element? And then Image Prioritizer could add the background image data as needed?
There was a problem hiding this comment.
The problem is that the tag visitors don't know what the LCP element with an external background image could possibly be, so they don't know to return true to flag the tag for capturing in URL Metrics. I mean, there could be a tag visitor that just returns true for every visited tag, which would result in data-od-xpath attributes being added to every single tag and then for every single tag to be stored in the elements of URL Metrics. But this would be wasteful.
In the case of an IMG being the LCP element, this is covered because the tag visitors for images are visiting every IMG so they are all being captured in the elements of a URL Metric. When such an IMG is the LCP element, we don't even need the url from the LCP Metric because the URL is right there in the src and srcset. The same goes for an element which has an inline background-image style: the tag visitor can see it so optimization can preload what is defined right there in the HTML.
But for elements with external background images, this is a special case.
There was a problem hiding this comment.
I've exposed the Web Vitals functions to extensions in 47d06dc.
There was a problem hiding this comment.
47d06dc#diff-ec3ffd3a54ce7df9d216b18a1dee27e4c41785e16698bf3229efcef5e7072fb2R64 looks good to me. It's definitely a nice little enhancement to not have to reimplement that part.
I guess I had not considered this scenario before - so is it correct that, in case the LCP element is not one of the specific elements that are being visited, it will be unknown what the LCP element is for that page load?
In that case, it makes sense that the extension has to implement it. Though I wonder whether we could make this a bit more ergonomic and centralize the implementation, e.g. to get benefits like "common LCP element" out of the box (with what #1723 improved). Doesn't have to be in this PR, but maybe we could implement the LCP element logic centrally so that every URL metric receives the LCP element, whichever it is? If we're worried about overhead of doing this when it's not needed, we could make it opt-in so that it's only recorded if at least one extension opts in to requiring it.
It is a bit specific to implement this directly in OD, but on the other hand my concern is that not doing so will have any extension that needs something LCP related to reinvent the wheel, when instead we could have a general LCP element handler and extensions could only extend what data is included as part of it. WDYT?
There was a problem hiding this comment.
I guess I had not considered this scenario before - so is it correct that, in case the LCP element is not one of the specific elements that are being visited, it will be unknown what the LCP element is for that page load?
Yes, that's exactly right. It won't be captured which element is the LCP element since the LCP element is something other than one of the elements being tracked.
In that case, it makes sense that the extension has to implement it. Though I wonder whether we could make this a bit more ergonomic and centralize the implementation, e.g. to get benefits like "common LCP element" out of the box (with what #1723 improved). Doesn't have to be in this PR, but maybe we could implement the LCP element logic centrally so that every URL metric receives the LCP element, whichever it is? If we're worried about overhead of doing this when it's not needed, we could make it opt-in so that it's only recorded if at least one extension opts in to requiring it.
So you're saying to implement a new root-level property in the URL Metric, like lcpElement (alongside the existing elements) which captures the properties in the LargestContentfulPaint interface? In this PR that's what is essentially what is being done, although specifically for a background image. And since the element can't be captured as-is, this PR is capturing the id (also exposed on LargestContentfulPaint), tag, and class as a way to identify the element. Ideally we'd be able to store the server-side XPath to the element, but again it may not be a visited tag, so that xpath property could perhaps be a nullable field so that client-side detection can provide it if it so happens that the LCP element has a data-od-xpath attribute. In this way, we wouldn't technically need to store isLCP with each element, as we could then just look to see if the xpath for an element matches the XPath stored with the LCP data.
And if we're capturing these properties of LargestContentfulPaint, should we also then capture the other properties like size, renderTime, and loadTime?
And then if we're capturing the LCP metrics, should we then also centralize the capturing of the CLS and INP data as well?
Or maybe we reconsider for later since this could go to far. For storing the LCP url, there's also the question of data validation. We'd want to make sure that we only preload safe URLs which is being addressed in #1713. We can always update Image Prioritizer later to reuse LCP metric data stored by Optimization Detective centrally, especially when/if it comes to light that more use cases need access to this centralized data.
So how about going with an Image Prioritizer-specific implementation but then we open a new issue to discuss capturing these metrics centrally in a future version. This would tie in nicely with the Performance dashboard (#1324) since the data would already be there (although the dashboard would probably want to introduce additional data retention to capture more than the sample size).
There was a problem hiding this comment.
So you're saying to implement a new root-level property in the URL Metric, like
lcpElement(alongside the existingelements) which captures the properties in theLargestContentfulPaintinterface?
Yes, exactly.
And if we're capturing these properties of
LargestContentfulPaint, should we also then capture the other properties likesize,renderTime, andloadTime?
Maybe. I think we should probably keep it light in the default implementation, but allow extensions to expand what is detected. For example there should be an extension point where the LCP data is reported in JS so that a plugin could add more data to the lcpElement object that is sent to the REST API, and on the server-side it would need to be possible to extend the schema for what can be in lcpElement.
I also would prefer not to include actual performance numbers, because OD is primarily an optimization tool, not a monitoring tool. A separate extension for monitoring could add such data as needed.
And then if we're capturing the LCP metrics, should we then also centralize the capturing of the CLS and INP data as well?
Potentially, but only if we see a need for it. While it seems to make sense from a metric perspective, I'd argue LCP is different from CLS and INP because there is no "CLS element" or "INP element". The LCP element is directly relevant to things that Optimization Detective allows to do. This also relates to what I mention above about focusing on areas that help optimization, not performance metrics. I find it less applicable for INP and CLS, because what would we send there to help optimization? Possibly there is some useful data (e.g. long tasks), but I feel that this should be separately explored, if at all.
So how about going with an Image Prioritizer-specific implementation but then we open a new issue to discuss capturing these metrics centrally in a future version. This would tie in nicely with the Performance dashboard (#1324) since the data would already be there (although the dashboard would probably want to introduce additional data retention to capture more than the sample size).
That sounds good to me.
There was a problem hiding this comment.
there is no "CLS element"
Well, there are elements that cause CLS. So in theory a top-level CLS property could capture the LayoutShiftAttribution sources. Also, for INP That said, Embed Optimizer isn't using this and is instead using ResizeObserver. Things to explore in the new issue. I've just filed it as #1730.
| @@ -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 ) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.
… add/external-bg-preload
| @@ -424,8 +471,6 @@ export default async function detect( { | |||
| } ); | |||
| } | |||
|
|
|||
| const { onLCP } = await import( webVitalsLibrarySrc ); | |||
|
|
|||
| /** @type {LCPMetric[]} */ | |||
| const lcpMetricCandidates = []; | |||
|
|
|||
There was a problem hiding this comment.
Now that onLCP is typed as OnLCPFunction I shouldn't think that /** @type LCPMetric */ is needed here:
performance/plugins/optimization-detective/detect.js
Lines 479 to 483 in 47d06dc
However, when I remove it, TypeScript in PHPStorm complains:
It doesn't complain about the usage in Image Prioritizer's detect.js here, however:
Fixes #1584
This captures the URL for the background image of the LCP element which is defined in a CSS stylesheet and not in an inline
styleattribute. A new client-side extension module from Image Prioritizer is introduced to implement this. A new root property is added to the URL Schema calledlcpElementExternalBackgroundImagewhich includes not only theurlof the background image but also thetagname,id,classfor the LCP element that has the background image. When theImage_Prioritizer_Background_Image_Styled_Tag_Visitoriterates over tags in the document, it checks to see if there is a matching tag (and thus the original element with the background image is still present). If so, and if all URL Metrics in the group are in agreement on that being the LCP element with that same background image, then the URL for the background image is added as afetchpriority=highpreload link for that viewport group. The requirement that all gathered URL Metrics be in agreement helps ensure that if a randomized background image is used (e.g. as can be done in core's header image), then no preloading will be done (as it is likely the wrong image will be preloaded).Twenty Thirteen Example
Given the Twenty Thirteen theme which has a CSS background image in the header and some images in the post content, on desktop the header is the LCP element whereas on mobile the first image is the LCP element:
With the changes in this PR, the CSS background image gets a
fetchpriority=highpreload link for desktop, whereas the first image in the content gets a preload link for mobile:The impact of preloading the background image on mobile is reflected in the network log, where without the optimization the
circle.pngimage is loaded with an initial low priority long after the other assets on the page have started loading:In contrast to when the image is preloaded, it is the initial resource loaded and has an initial high priority:
Here are the before/after metrics testing with
benchmark-web-vitals(with GoogleChromeLabs/wpp-research#164) without any page caching:Test setup
I created a site in Local and set the theme to Twenty Thirteen. I then created a post which had a 3-columns block with the center column being wider. I put an image in each column, so the middle image was larger. I visited the URL in a desktop viewport and mobile viewport multiple times to ensure the URL Metrics were fully populated. I then created a text file called
preload-bgimage-urls.txtwhich contained:I then ran 100 iterations before/after on desktop and mobile:
So on desktop, the LCP-TTFB is improved by 8.8%. On mobile, the LCP-TTFB is improved by a lesser degree by 0.87% because WordPress was already adding
fetchpriority=highto the first image, so the marginal improvement is gained by the preload link.Elementor Example
As another test, I created a site with Elementor and the Hello Elementor theme. I used the Ceramic Studio "website kit" to create a page, and I added a mobile-specific image to the hero's second container:
Elementor implements the images here as background images pulled from an external CSS file (
http://localhost:10053/wp-content/uploads/elementor/css/post-67.css?ver=1732401999):The element being targeted is:
Running the same tests as before with 100 iterations on both desktop and mobile, before and after the optimizations, yields the following results (again where mobile is 360x800 and desktop is 1920x1080):
The LCP-TTFB on desktop is improved by 18.19% and on mobile it is improved by 21.57%! 🎉 (What is surprising to me as well is that TTFB is reduced when the optimizations are applied, which doesn't make any sense since the HTML Tag Processor spends cycles doing work.)
It's important to note that this page doesn't just have CSS background images. Further down the page outside the viewport of both desktop and mobile, there are three
IMGtags in another section:Elementor is adding
fetchpriority=highto thisIMGeven though it is not even displayed in any initial viewport:The Elementor code responsible is the
maybe_add_fetchpriority_high_attr()method which appears to be heavily inspired by what WordPress core does.Here's a diff of the page (with Prettier formatting). Note how Image Prioritizer is adding responsive preload links for the two different background images while at the same time removing
fetchpriority=highfrom theIMGthat Elementor added it to. In addition, Image Prioritizer is addingloading=lazyandsizes=autoto all of these images since none of them appear in the initial viewport on desktop or mobile:Network log without the optimizations up until the LCP element's background image is loaded:
Compared with after the optimizations applied:
Note how the LCP element's background image is now loaded as early as possible with initial
highpriority, whereas without the optimizations the image is loaded very late and has an initial priority oflow.Takeaway
This represents an critical performance advancement for optimizing LCP in WordPress because on the web a
DIVis the second most common LCP element afterIMG. Since images account for the LCP type 82% desktop and and 72% on mobile, it's likely that most of theDIVLCP elements represent background images. Additionally, page builders like Elementor and Divi leverage external background images extensively, including separate background images for desktop and mobile.Todo