Skip to content
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

Ignore incorrect aspect ratio if the image is 1x1 pixel #6290

Closed
SebastianEberlein opened this issue Oct 16, 2018 · 7 comments
Closed

Ignore incorrect aspect ratio if the image is 1x1 pixel #6290

SebastianEberlein opened this issue Oct 16, 2018 · 7 comments

Comments

@SebastianEberlein
Copy link

Feature request summary
Ignore incorrect aspect ratio if the image is 1x1 pixel.

What is the motivation or use case for changing this?
Lazyloading images. The image container has an intrinsic ratio to avoid layout shifting. The placeholder image is a 1x1 pixel transparent base64 gif as suggested here.

Lighthouse 3.2.0 displays the warning "Displays images with incorrect aspect ratio":

screenshot

This is technically correct, but should be ignored. A 1x1 pixel image will almost never match the natural aspect ratio. And it is unlikely that a 1x1 pixel image is used for anything other than a placeholder.

How is this beneficial to Lighthouse?
Seeing this warning, developers might look for a different solution, maybe a larger placeholder image with the correct aspect ratio. But this could increase the overall page size and thus degrade performance.

@SebastianEberlein SebastianEberlein changed the title Image aspect ratio test: Ignore 1x1 pixel lazyload placeholder image Ignore incorrect aspect ratio if the image is 1x1 pixel Oct 16, 2018
@patrickhulce
Copy link
Collaborator

Thanks for filing @SebastianEberlein! I'm not sure I follow why your placeholder image needs to be so big if you set the actual size to be 1x1 anyhow.

Maybe I'm misunderstanding but it seems like there's two possible paths with this neat trick you're using. Please correct me if I'm wrong :)

  1. You use a transparent 1x1 GIF as the source image and stretch it to fill the real aspect ratio of the image it's placeholding. This will fail the audit, but it's efficient, and it seems reasonable for us to ignore images with sufficiently small source dimensions in LH.
  2. You use a transparent GIF the same size as the image it's placeholding. This will waste bytes, but now there's no need to set the size of the placeholder and the audit will pass naturally.

IIUC, it seems like you're using a transparent GIF the same size as the image it's placeholding, but then forcing it to not take up its natural placeholder size.

With the current situation in the screenshot, there's no way of LH knowing that this seemingly regular sized image isn't being accidentally squished by some CSS rule which is one of the exact reasons the audit exists.

@wardpeet
Copy link
Collaborator

@patrickhulce I'm pretty sure they use this lazyloading technique
<img src="data:blankgifdatauri" width="376" height="187" data-src="realimage" /> this way the image will take up it's space and lazyloads the image when needed without getting a page to jump up and down.

Sadly like @patrickhulce described we can't track down that your case is used for lazyloading. There are other ways to lazyload and keep your space filled in like flexembed techniques so your audit will pass 😛

@patrickhulce
Copy link
Collaborator

patrickhulce commented Oct 16, 2018

Ooooooooh, wait I misread the screenshot as flipped 🤦‍♂️ 🤣

So so sorry @SebastianEberlein that makes perfect sense! You're already doing what I've described in bullet 1. Yeah we should ignore 1x1 source pixel images.

@patrickhulce
Copy link
Collaborator

For any potential contributors, these few lines are where we'd want to throw in a filter for the width/height being at least a few pixels (5px min maybe?)

images.filter(image => {
// - filter out images that don't have following properties:
// networkRecord, width, height, images that use `object-fit`: `cover` or `contain`
// - filter all svgs as they have no natural dimensions to audit
return image.networkRecord &&
image.networkRecord.mimeType !== 'image/svg+xml' &&
image.width &&
image.height &&
!image.usesObjectFit;
}).forEach(image => {

@CodeDem
Copy link
Contributor

CodeDem commented Oct 17, 2018

Hi @patrickhulce , I'll try to fix this bug as my first contribution to lighthouse. Thank for the above code

@midzer
Copy link
Contributor

midzer commented Oct 24, 2018

Good thing issue seems fixed :)
More alternatives than the 1x1 gif pixel approach for lazy loaded images are discussed on https://stackoverflow.com/questions/31337327/source-placeholder-for-lazy-loading-images/49452679

@brendankenny
Copy link
Member

ah, thanks @midzer, we missed closing this. Fixed in #6305

If you run into Lighthouse issues with those other methods, we should take a look to see if they're worth adding to the audit as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants