-
Notifications
You must be signed in to change notification settings - Fork 103
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
Changes from all commits
8439753
cee5c24
da12ebd
cf4b6e9
a75b94f
f2959cd
f4b766a
dcb7a5c
f078723
d3339f0
ecac937
494e4aa
1348145
94b3fee
74f9241
94e527b
bc81588
e642c99
d2e90d3
f546731
ec59d85
514c513
fbb6076
5c3e698
b4f1172
e77bd53
b8b79de
3ac0253
47d06dc
2add133
3abf92a
02c6673
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
*/ | ||
|
@@ -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. | ||
* | ||
|
@@ -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 ) { | ||
$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 ] ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this not impact the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had wondered that as well, but I found that Examples: But this could be made less concerning upon inspection if this iterates over There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
* | ||
|
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is the case. The fix in #1723 is to allow
fetchpriority=high
to be on theIMG
element when only the mobile and desktop viewport groups have URL Metrics collected. Preload links are still only being added forIMG
for the actual viewport groups that have URL metrics collected for them:performance/plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php
Lines 342 to 348 in d13f33c
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?
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 returnstrue
for a tag during iterator), then as part of the stored metadata for that tracked element will be whether itisLCP
. 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.