core(fix): fix incorrect aspect ratio bug if the image is 1x1 pixel

[CodeDem]

fixes incorrect aspect ratio bug if the image is 1x1 pixel by filtering the image less than 5 * 5
pixels dimension

Related Issues/PRs
#6290

[patrickhulce]

Thanks very much for the PR @CodeDem! You'll have to update the unit test for this audit (https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/test/audits/image-aspect-ratio-test.js).

Right now it's asserting that we warn on invalid sizing information, but we've decided to just filter them out instead so we should update the test case accordingly 👍

[CodeDem]

@patrickhulce, Yes I do realise it after my test case failed when I raised the PR. 😅 what I think is instead to update the unit test, what if we allow images with natural size 0 * 0 and the images above 5 * 5 dimension. This way lighthouse can report 'has invalid natural sizing information' as well and also ignore images below 5 * 5 dimensions.

Let me know your thoughts :)

[patrickhulce]

Good thinking @CodeDem! Honestly though, that invalid natural sizing information error has not been very actionable and it's been some overzealous for no good reason over the past year.

I'm fine with filtering them out altogether unless @brendankenny you might have strong opinions here?

[brendankenny]

@brendankenny you might have strong opinions here?

that seems ok, I think. Questions, tho:

1. is it possible under normal conditions to get a natural size of 0x0? Seems like it would usually indicate that something was wrong
2. why < 5x5 and not just allow 1x1? Is it common to use a 4x4 image as a placeholder, for instance?

[wardpeet]

common is actualy 1x1 i think 5x5 is to high as well at least for lazy loading images but I might be wrong here. I only use 1x1 transparent gifs.

[CodeDem]

@brendankenny @wardpeet @patrickhulce

I agree it makes sense to only ignore 1x1 images since this looks more standard in terms of placeholders for lazyloading and thus saves size.

However, 1x1 pixel can only store single color, using 4x4 size placeholder can represent the rough overview of the image content and colors of what they are about to see.

we should let developer to choose placeholder between 1x1 to 5x5 image as per requirements, correct me if I'm wrong here.

Eg of 1x1 image placeholder

Eg of 4x4 image placeholder

Actual image

Let me know your thoughts.

[patrickhulce]

is it possible under normal conditions to get a natural size of 0x0? Seems like it would usually indicate that something was wrong

It's not clear under what conditions we'd be getting a size of 0x0, and the last several times I tried looking at the URLs in Sentry for invalid image sizing none of them repro'd. I'm not sure it's worth tracking at this point.

why < 5x5 and not just allow 1x1? Is it common to use a 4x4 image as a placeholder, for instance?

IMO, this audit is not really all that useful for very small images, there's not much to distort. Heck I'd be willing to say <20x20 could be ignored. There's something to be said for @CodeDem's example of slightly larger placeholder pixelated placeholder that are then blurred to give a better speed index. I'm not sure what our reasonable allowance should be, but I'm in favor of more than just 1x1.

[brendankenny]

I'm convinced. < 5x5 sounds good

[CodeDem]

Alright, @patrickhulce @brendankenny shall I go ahead and commit the changes with removed test case for invalid natural sizing information and image filter for images less than 5x5 ?

[patrickhulce]

changes with removed test case for invalid natural sizing information and image filter for images less than 5x5 ?

Let's do it! I would say "changed test case" instead of removed, but you get the idea :)

[brendankenny]

LGTM!

[brendankenny]

over to @patrickhulce if you want a last look

[patrickhulce]

LGTM thanks @CodeDem!! 🎉

[CodeDem]

Thank you @patrickhulce and @brendankenny for helping through my first contribution, yaay :)