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 for LCP element with background-image #914
Preload image for LCP element with background-image #914
Conversation
d54c6c2
to
0942fa5
Compare
fb86490
to
c6ff3c0
Compare
…om/WordPress/performance into add/ilo-background-image-optimization * 'feature/image-loading-optimization' of https://github.com/WordPress/performance: (177 commits) Add additional line breaks in phpdoc Add line break before dataProvider tag Add missing since tag Move ilo_can_optimize_response() to optimization.php Update ilo_verify_url_metrics_storage_nonce() to return bool Re-run composer update with PHP 8.1 Run composer update Fix PHPStan errors in tests Remove scheduler from globals since not yet used Fix or ignore eslint rules Remove modules/images/webp-uploads/fallback.js from ignorePatterns Run format-js on JS files Cherry pick fixes to JS linting from feature/image-loading-optimization Unset REQUEST_URI in tests that call go_to Reset SERVER global after each test Unrevert 2181d88 for audit-enqueued-assets-test.php Prevent sending header during test Unrevert 2181d88 for server-timing-tests.php Revert test changes moved to #924 Manually fix remaining phpcs issues ...
* Include screen media type * Remove whitespace padding from media features * Omit needless min-width:0 media feature
Co-authored-by: Pascal Birchler <pascalb@google.com>
*/ | ||
$background_image = null; | ||
if ( $style && preg_match( '/background(-image)?\s*:[^;]*?url\(\s*[\'"]?(?!data:)(?<background_image>.+?)[\'"]?\s*\)/', $style, $matches ) ) { | ||
$background_image = $matches['background_image']; |
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.
nice use of a named sub-pattern, I forgot that was possible.
$processor->remove_attribute( 'loading' ); | ||
// Never include loading=lazy on the LCP image common across all breakpoints. | ||
if ( 'lazy' === $processor->get_attribute( 'loading' ) ) { | ||
$processor->set_attribute( 'data-ilo-removed-loading', $processor->get_attribute( 'loading' ) ); |
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.
nice, I like how the change is marked with a data attribute
$processor->set_attribute( 'data-ilo-removed-loading', $processor->get_attribute( 'loading' ) ); | ||
$processor->remove_attribute( 'loading' ); | ||
} | ||
} elseif ( $all_breakpoints_have_url_metrics && $processor->get_attribute( 'fetchpriority' ) ) { |
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 bit is unclear to me. if all breakpoints have data, you are falling back to the server heuristics?
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.
If all breakpoints have data, then we can reliably use the preload
links to prioritize loading of the LCP image. In that case, we can remove the fetchpriority
attribute from the LCP image here.
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 see, so you are favoring preload over fetchpriority? maybe we discussed this before, couldn't you use both of them? or is high priority already implied when using preload?
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.
The preload
links also include fetchpriority=high
, so having it on the element is redundant. Also, it can be counter productive to be on the img
in the case where there are different LCP images for different viewports. If there is an image with fetchpriority=high
but it is only for the desktop viewport then it will get loaded with high priority even for mobile users. The preload links also include media
attribute with media queries so they enable fetchpriority=high
to be conditional on the viewport size.
</head> | ||
<body> | ||
<div class="header desktop" style="background: red no-repeat center/80% url(\'https://example.com/desktop-bg.jpg\'); width:100%; height: 200px;">This is the desktop background!</div> | ||
<div class="header tablet" style=\'background-image:url( "https://example.com/tablet-bg.jpg" ); width:100%; height: 200px;\'>This is the tablet background!</div> |
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.
the quoting on this line is reversed from all of the others which makes it stand out. is that required for the test to pass?
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.
It's just testing that single-quoted and double-quoted strings both work in the CSS url()
.
</head> | ||
<body> | ||
<div class="header desktop" style="background: red no-repeat center/80% url(\'https://example.com/desktop-bg.jpg\'); width:100%; height: 200px;">This is the desktop background!</div> | ||
<div class="header tablet" style=\'background-image:url( "https://example.com/tablet-bg.jpg" ); width:100%; height: 200px;\'>This is the tablet background!</div> |
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.
same Q this line
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.
Ditto. Just to make sure that the regex works with single quotes as well as double quotes.
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.
Looks great, left some small comments/questions
3ff542e
into
feature/image-loading-optimization
Summary
Add image
preload
links for thebackground-image
on the LCP element.Doing a quick test with the parallax Cover block, the changes in this PR reduce median LCP-TTFB from 71.1ms to 64.8ms over 25 requests respectively: almost a 9% reduction.
Relevant technical choices
Adding
preload
links was already being done for LCPimg
elements. When all breakpoints had the sameimg
as the LCP element, it also addsfetchpriority=high
to thatimg
. Since a CSSbackground-image
cannot getfetchpriority=high
, this is only added via thepreload
link.Both the
background-image
property andbackground
shorthand property are supported. Aurl()
containing a data: URL is skipped.Note that CSS allows for a
background
/background-image
to have multipleurl()
CSS functions, resulting in multiple background images being layered on top of each other. This ability is not employed in core. Here is a regex to search WPDirectory for instances of this:background(-image)?:[^;}]+?url\([^;}]+?[^_]url\(
. It is used in Jetpack with the second background image being a gradient. To support multiple background images, the logic would need to be modified to make$background_image
an array and to have a more robust parser of theurl()
functions from the property value.The
preload
links are improved to add ascreen
media type and to omit the needlessmin-width:0px
media feature.This PR also adds server timing for
image-loading-optimization
. When checking withbenchmark-server-timing
over 100 requests, enabling Image Loading Optimization only increases the response time by ~2ms.Also, this removes the workaround for browsers that don't support
imagesrcset
since >95% do. See #117 (comment).Checklist
[Focus]
orInfrastructure
label.[Type]
label.no milestone
label.