feat(image-usage): add support for CSS images #1868
Conversation
patrickhulce
force-pushed
the
css_images
branch
from
6da797a
to
01823f6
Compare
| isPicture: element.parentElement.tagName === 'PICTURE', | ||
| }; | ||
| }); | ||
|
|
||
| // Only match basic background-image: url("http://host/image.jpeg") declarations | ||
| const ALLOWED_IMAGE = /^\s*url\("([^"]+)"\)\s*$/; |
There was a problem hiding this comment.
lets do a trim() on the string before running this regex so we can drop the \s* on the ends
There was a problem hiding this comment.
quotes aren't required inside of a url().. And i personally rarely use them.
also some people use single quotes.
so let's allow these combos. this guy (thanks SO) seems good:
/(?:\(['"]?)(.*?)(?:['"]?\))/There was a problem hiding this comment.
and naming nit: how about css_url_regex instead of allowed_image
There was a problem hiding this comment.
in getComputedStyle Chrome normalizes the URL to full URL in quotes :) in fact the spacing is probably totally useless too since I bet those are eliminated as well (just not as thoroughly tested by me) I'll add a comment saying so
|
Ah that's cool to know. :)
|
patrickhulce
force-pushed
the
css_images
branch
from
053b3fc
to
f8b0d5d
Compare
| const CSS_URL_REGEX = /^url\("([^"]+)"\)$/; | ||
| // Only find images that aren't specifically scaled | ||
| const CSS_SIZE_REGEX = /(auto|contain|cover)/; | ||
| const cssImages = [...document.querySelectorAll('*')].reduce((images, element) => { |
There was a problem hiding this comment.
we should probably make this shadow dom-aware: document.querySelectorAll('html /deep/ *').
There was a problem hiding this comment.
Collecting all the dom nodes on the page + running gCS on them....wowza. How heavy is that?
There was a problem hiding this comment.
heh yeah...you have any better ideas? i'm all ears :D
| clientWidth: element.clientWidth, | ||
| clientHeight: element.clientHeight, | ||
| naturalWidth: Number.MAX_VALUE, | ||
| naturalHeight: Number.MAX_VALUE, |
There was a problem hiding this comment.
Is this b/c "css images have no natural size"? Might want to add that comment here too. I was expecting these to be element.clientWidth/Height.
There was a problem hiding this comment.
Collecting all the dom nodes on the page + running gCS on them....wowza. How heavy is that?
heh yeah...you have any better ideas? i'm all ears :D
lol, so turns out this does slow things way down for some sites since we've introduced CPU throttling and that apparently slows down our own injected code too. D'oh. Not sure what we should do because we do need the network throttling from being in this pass.
Details:
I was testing https://santatracker.google.com, but only the desktop version has a ton of CSS images. So if I run node lighthouse-cli/index.js https://santatracker.google.com --disable-device-emulation:
- On master, the ImageUsage gatherer takes 17ms
- On this branch, the ImageUsage gatherer takes 6 seconds
While playing with network throttling I accidentally disabled CPU throttling. If I run node lighthouse-cli/index.js https://santatracker.google.com --disable-device-emulation --disable-cpu-throttling:
- On master, the ImageUsage gatherer takes 7ms
- On this branch, the ImageUsage gatherer takes 261ms
So, still a significant increase in execution time, but nothing like it is with CPU throttling.
I would suggest moving it to another pass, but we do want the throttled network data. Is there something else we can do here to cut down execution time?
|
Just to be clear, this isn't really a problem on many sites, even ones with a similar number of DOM nodes, so not sure exactly what's going on yet. And of course it's next to impossible to profile the code we run in the context of the browser to find out... |
|
@paulirish @brendankenny more feedback? |
|
bump PTAL :) |
There was a problem hiding this comment.
LGTM. It's still a little heavyweight even after #1901, but a few hundred milliseconds for a very image heavy site isn't so bad. Still would be interesting to see where the time was going if we could easily profile the js code we run in the browser context.
we can if we just keep the trace on during this time.. just some gatherer juggling. :) |
fixes #978
Supports an MVP of responsive CSS images. The area here is a lot more gray than with HTML image elements, so I've kept it very limited in scope, but I believe it should cover the most naive case that's likely to have bloated images.
After this lands, I'd like to do some cleanup so that we're not potentially fetching images multiple times in the optimized image gatherer and this one.