-
Notifications
You must be signed in to change notification settings - Fork 107
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
Lazy load background images added via inline style attributes #1708
Lazy load background images added via inline style attributes #1708
Conversation
TODO
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @rcwd. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
$processor->append_head_html( | ||
'<style> | ||
@media (scripting: enabled) { | ||
.has-background.od-lazy-bg-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.
Where is this has-background
class coming from? I don't think we should depend on that being there as any element that has the inline style attribute with the background image is eligible for this lazy 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.
In Gutenberg, the has-background
class is typically added to blocks when a background image or color is applied. However, you're right that this might not be consistent, especially with custom blocks or themes.
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.
Done in ce351c2.
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 looking great. I want to do some benchmarking to see the impact on load time performance to see if reducing network contention improves LCP.
Please also add some test cases for the new logic.
|
||
if ( ! $this->added_lazy_styles ) { | ||
$processor->append_head_html( | ||
'<style> |
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.
Maybe this should be pulled in from a CSS file like we're doing for the lazy script? In this way we can use the minified version when not in SCRIPT_DEBUG
mode.
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.
Currently, the Webpack setup doesn't minify CSS files the same way it handles JavaScript, as the TerserPlugin (used by CopyWebpackPlugin) is specifically for JavaScript minification. I'll explore other approaches to address this.
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 guess wp-scripts
should have some way of doing this? I don't have a lot of experience with setting this up.
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.
While wp-scripts
does handle CSS minification as part of its default build pipeline when importing CSS in JavaScript, it doesn't offer a direct solution for standalone CSS files like these, especially when the goal is to output them in the same directory as the source files.
I think it would be better to use a minifier like cssnano
—which is also used by the postcss-loader
in the Webpack configuration provided by wp-scripts
—through a Webpack plugin like CssMinimizerPlugin
, with identical minimizerOptions
as in wp-scripts
to ensure consistency with the wp-scripts
build process.
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.
Done in a21dec4.
While working on the Webpack configuration, I noticed that most plugin setups (except those copying library files from To streamline this, I thought we could create a separate, generalized Webpack configuration that automates asset minification across all plugins. This approach would:
Here's a potential implementation: /**
* Webpack Config: Minify Plugin Assets
*
* @return {Object} Webpack configuration
*/
const minifyPluginAssets = () => {
return {
...sharedConfig,
name: 'minify-plugin-assets',
plugins: [
new CopyWebpackPlugin({
patterns: [
{
from: `plugins/**/*.js`,
to: ({ absoluteFilename }) =>
absoluteFilename.replace(/\.js$/, '.min.js'),
globOptions: {
ignore: ['**/build/**', '**/*.min.js'],
},
},
{
from: `plugins/**/*.css`,
to: ({ absoluteFilename }) =>
absoluteFilename.replace(/\.css$/, '.min.css'),
transform: {
transformer: cssMinifyTransformer,
cache: false,
},
globOptions: {
ignore: ['**/build/**', '**/*.min.css'],
},
},
],
}),
new WebpackBar({
name: 'Minifying Plugin Assets',
color: '#2196f3',
}),
],
};
}; Would this approach be worth considering for a future enhancement or perhaps as a replacement for some of the existing configurations? It could reduce maintenance overhead and improve consistency. PS: The configuration could also be adjusted to target assets for only the plugin being built during production builds if that aligns better with the workflow. |
@ShyamGadde That sounds good! Please do open a new enhancement issue for that. |
I did some Web Vitals benchmarking. I tested with a page that had a featured image set so that it is the LCP element, and then far down the page I added a Cover block with a parallax background image and a Group block also with a different assigned background image. I did not see a consistent improvement to LCP over testing hundreds of iterations, which is largely to be expected. I did, however, see a large reduction in the initial page weight. Since there is no
After only the |
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.
Great work!
$outside_viewport_rect = array_merge( | ||
$test_case->get_sample_dom_rect(), | ||
array( | ||
'top' => 100000, | ||
) | ||
); |
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.
Good catch
@@ -12,7 +12,7 @@ | |||
</body> | |||
</html> | |||
', | |||
// There should be no data-od-xpath added to the DIV because it is using a data: URL for the background-image. | |||
// There should be no data-od-xpath added to the DIV because it is using a background-color style. |
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.
Good catch
Summary
Fixes #1035
Relevant technical choices
style
attributes.