-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core(image-elements): drop spritesheet logic #8940
Conversation
@paulirish wanted a go at this |
To clarify, @paulirish wanted to review this, or he wanted to make a case for keeping the spritesheet logic? :) |
My understanding is that he thought there was more insight the debugger protocol could provide, but maybe I'm misremembering. You were in that chat too :) |
Oh I thought that was about #8493. This PR doesn't deal with stylesheet content really, just image elements that have background set to image which is problematic even when we have the complete stylesheet content. But maybe I'm mixing up my conversations 😆 |
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.
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.
so is the main thing that's happening is that ImageElements
now includes all background-image
images (some were filtered out before for appearing to be sprite sheets or whatever) while uses-responsive-images
is now going to ignore all images in CSS?
lighthouse-core/audits/byte-efficiency/uses-responsive-images.js
Outdated
Show resolved
Hide resolved
Bingo |
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.
LGTM 👍
Summary
We previously had some broken logic to address spritesheets in CSS for our responsive images but it has several flaws. I think we should just drop it and ignore CSS background images in our responsive image audit.
Here's why:
Related Issues/PRs
fixes #8927