new_audit: check images are big enough

[sk-]

Summary
New audit checking visible images are big enough. This is a complement to UsesResponsiveImages.

Related Issues/PRs
Fixes #10434. Check that issue for the New Audit Proposal.

[patrickhulce]

looks great @sk- thanks for pumping this out so quickly!

[brendankenny]

I vaguely recall there being discussion in the past about this kind of audit and there being issues with a lot of use cases where high DPI images don't exist (old assets or whatever), aren't needed (smaller asset scales fine enough and it saves bytes), or aren't used on purpose (beyond just lazy loading...graphical effects or whatever). (edit: the first case is probably fine to fail, I guess, even if the site can't fix them)

I could see us launching and then gradually adding exceptions to the audit, but there may be a lot of users upset over failing an audit in Best Practices in the meantime :)

Maybe we need to collect some data on how a decent sample of sites will fare on this audit?

[sk-]

I vaguely recall there being discussion in the past about this kind of audit and there being issues with a lot of use cases where high DPI images don't exist (old assets or whatever), aren't needed (smaller asset scales fine enough and it saves bytes), or aren't used on purpose (beyond just lazy loading...graphical effects or whatever). (edit: the first case is probably fine to fail, I guess, even if the site can't fix them)

I could see us launching and then gradually adding exceptions to the audit, but there may be a lot of users upset over failing an audit in Best Practices in the meantime :)

Maybe we need to collect some data on how a decent sample of sites will fare on this audit?

@brendankenny it'd be great if you can find a link to that conversation.

With regard to point number 2) ((smaller asset scales fine enough and it saves bytes)), it'd be great if we could actually have some of those assets. In my experience small assets do not scale well, compared to large assets. That's the reason that for small images <= 64x64, we require the image to have a pixel density == DPR. But for larger images we only ask for 1.75 * DPR.

The only case I know in which an asset with a lower pixel density scales well is when you want it to be pixelated and you specify:

image-rendering: pixelated;
image-rendering: -moz-crisp-edges;
image-rendering: crisp-edges;

Point number 3 is also strange in mi opinion, as right now there are many audits that developers may decide to not address on purpose (for example the ResponsiveImages or AspectRatio), however we do not see a way to disable them for those cases.

I guess we could change the weight of the audit, if the score is what ultimately annoys people.

With the current settings, the audit is not reporting that many offending images.

For example for the top 10 US site according to Alexa, the results are the following:

google.com ❌2 offending images (doodle and share inline icon) both look displeasing to me.
youtube.com ❌3 offending images (avatars are 68x68 instead of 72x72)
amazon.com ❌3 offending images. They all look bad to me.
facebook.com ✅
yahoo.com ❌1 offending image. Image actually has a passing density, the problem is that the aspect ratio is not correct.
reddit.com ✅
wikipedia.org ❌1 "offending" image. Problem seem to be a bug with LH in which the actual size is not correct in presence of a `srcset`. Will investigate. 
ebay.com ✅
netflix.com ✅
bing.com ✅

Note also, that the values provided are up for discussion and tweaking.

[sk-]

@patrickhulce naturalWidth and naturalHeight do not take into account the size of the actual image used in the srcset. You can try it for example on https://www.wikipedia.org and on https://www.wikipedia.org/portal/wikipedia.org/assets/img/Wikipedia-logo-v2@2x.png, by running

document.querySelector('image').naturalWidth

However, on the gatherer I see that there's some code to compute the natural size for some images (https://github.com/GoogleChrome/lighthouse/blob/abee0c0/lighthouse-core/gather/gatherers/image-elements.js#L213-L223). Should we include in that check images in which currentSrc !== src, what would happen for images not in the top 50?

So I guess, we would need to add a field usesSrcset to signal the image element is using srcset and hence responsive images, so we can skip them.

We may also want to add another field called usesPixelScaling to signal the image element wants to be upscaled in pixel art form. That happens when any of the following CSS properties are set:

image-rendering: pixelated;
image-rendering: -moz-crisp-edges;
image-rendering: crisp-edges;

Let me know WDYT, I'm open to suggestions.

[brendankenny]

@brendankenny it'd be great if you can find a link to that conversation.

Sorry, can't find anything in chat or email. It was probably back when uses-responsive-images landed. A lot of time to lose things since 2017 :) I don't think there was anything substantial beyond the points you raise now.

Point number 3 is also strange in mi opinion, as right now there are many audits that developers may decide to not address on purpose (for example the ResponsiveImages or AspectRatio), however we do not see a way to disable them for those cases.

I guess we could change the weight of the audit, if the score is what ultimately annoys people.

The tension is that users really don't like when Lighthouse asks them to fix something they think is a legitimate technique. Even unscored things come across as criticism/instructions from Google to fix something. That doesn't mean we shouldn't do it, but it is a cost that has to be weighed against the benefit of an audit.

[patrickhulce]

So I guess, we would need to add a field usesSrcset to signal the image element is using srcset and hence responsive images, so we can skip them.

Yeah usesSrcset property that forces it to fall into that same bucket of isPicture SGTM! 👍

what would happen for images not in the top 50

I think the safeguard of passing usesSrcset works here.

We may also want to add another field called usesPixelScaling to signal the image element wants to be upscaled in pixel art form.

Yeah SGTM too! Good catch 👍

[sk-]

Great, will then add those 2 properties in 2 PRs first.

[patrickhulce]

LGTM % nits

thanks again @sk-!

[patrickhulce]

still LGTM!

[sk-]

@patrickhulce I added the requested smoke test, let me know if everything looks fine.

I am now skipping all images with a density descriptor, as there is no way to get the actual size, and I did not want to reload them, as that could break some of the assumptions about naturalWidth/naturalHeight. If you have a better strategy here, that'd be great.

The only missing thing that may be a little annoying is the fact that if an image has wrong aspect ratio and is at the same time too small, it will reported in both categories. The problem of fixing that is that it would require pretty much the same logic present in the ImageAspectRatio audit. I guess one alternative would be to have a public static function in that audit that tells wether the aspect ratio is valid or not.

Is there a list of websites that you would like me to test the audit.

From the list I posted earlier, the only change is that now wikipedia is passing.