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

Image natural dimensions strict to the precision of 4th decimal place #11505

Closed
zenofewords opened this issue Oct 1, 2020 · 11 comments · Fixed by #11518
Closed

Image natural dimensions strict to the precision of 4th decimal place #11505

zenofewords opened this issue Oct 1, 2020 · 11 comments · Fixed by #11518
Assignees

Comments

@zenofewords
Copy link

Hello, I noticed a "best practices" issue while testing some image srcsets. Here's a similar issue from 2018 (#5356) where the ratio check was loosened.

What is the current behavior?

At first, it looked like an image size issue, but using object-fit: cover; fixed it. So I checked the ratio and found that there are small differences between what's being displayed and the natural dimensions of the image.

Screenshot 2020-10-01 at 14 31 54

1st image displayed and expected vs displayed ratio is:
0.9473 (360x380, 1080x1140) vs 0.9478 (800x844)
2nd image displayed and expected vs displayed ratio is:
1.2631 (360x285, 1080x855) vs 1.2618 (800x634)

What is the expected behavior?

Unless I'm misunderstanding the "natural dimensions" audit, a check which is a little bit less strict.

Environment Information

  • Affected Channels: DevTools
  • Lighthouse version: 6.0.0
  • Chrome version: Version 85.0.4183.121 (Official Build) (64-bit)
  • Node.js version:
  • Operating System: macOS Catalina
@patrickhulce
Copy link
Collaborator

Thanks for filing @zenofewords! #5356 is talking about a different audit. This audit's ratio is referring to the device pixel ratio NOT the aspect ratio of the image. It checks to make sure srcset/picture is being used to serve a large enough image with DPR taken into account.

That being said, your images should actually pass in the latest version of Lighthouse (try Chrome Canary) which lowered this expectation to a max of 2x DPR (rather than the 3x shown in the screenshot).

@zenofewords
Copy link
Author

Thank you for the explanation and heads-up about the upcoming change, @patrickhulce! object-fit: cover; making the audit pass threw me off, still unclear why it would help in this case?

@patrickhulce
Copy link
Collaborator

patrickhulce commented Oct 1, 2020

object-fit: cover; making the audit pass threw me off, still unclear why it would help in this case?

It doesn't, that's again a different audit (the one that #5356 is talking about) Oh, I see you said object-fit: cover fixed it in your case. I have no clue, it shouldn't :) Do you have public URLs for these two pages you're talking about by chance?

@patrickhulce
Copy link
Collaborator

patrickhulce commented Oct 1, 2020

Ah, OK I have an idea how it could be related. If the image's container dimensions are much larger such that using object-fit: cover would make the image larger then it might pass the threshold for being large enough to be passed by this audit.

@zenofewords
Copy link
Author

Unfortunately no URLs, still local, but you figured it out.

The images are set to 100% width and auto height, looks like it fooled the audit. Alright, it's good to know I won't need to use cover where it's not required. Thanks again and sorry for the false alarm :)

@zenofewords
Copy link
Author

zenofewords commented Oct 1, 2020

Got back on my dev machine and tried setting both width and height to 400px. object-fit: cover; still makes the audit pass.

Also checked out canary (lighthouse 6.3.0), the audit still expects 3x, and cover makes it pass. I can put up a public minimal example tomorrow, if you think it would be of use.

@patrickhulce
Copy link
Collaborator

the audit still expects 3x

The audit lists a higher DPR than it expects in 6.3.0, but internally should accept anything that's at least 2x. Do you have an example where 2x doesn't work?

I can put up a public minimal example tomorrow, if you think it would be of use.

That would be awesome :)

@zenofewords
Copy link
Author

Here's the repo and the netlify URL.

I've added 2 sets of 2 images (red/blue and green/yellow). The only difference between the two is that that green/yellow also have object-fit: cover.

Red/blue fail the audit, while the green/yellow pair passes, both on latest chrome and canary. Adding object-fit: cover to red/green will make the audit pass.

I'd expect Displays images with incorrect aspect ratio to pass when using cover, but not the Displays images with inappropriate size.

Failing audit screenshots (canary is in dark mode):
fail without object fit cover
fail without object fit cover (canary)

Audit passed after adding object-fit: cover to every image:
pass with object fit cover

@patrickhulce
Copy link
Collaborator

patrickhulce commented Oct 2, 2020

Thanks so much for the detailed repro! I can see exactly what you're talking about in DevTools.

Oddly though, the CLI sees the expected behavior and is not caught by this. We'll have to dig into why DevTools is ever flagging this red image since it should be big enough to pass our threshold 🤔

My initial hunch is that DevTools screen emulation is noticeably different than when run from the CLI and that's somehow involved. Yes that's it DevTools uses DPR=3 for Moto G4 instead of the Lighthouse default.
image

To correct my earlier statement though, apparently we do in fact explicitly ignore images with object-fit afterall 😆 (I'm guessing because if it's object-fit: contain then the image can actually be significantly smaller than the container area and still be OK. I would expect us to fix this as part of the object-fit rework in #11207 .

@patrickhulce
Copy link
Collaborator

I'm going to let this issue track updating quantizeDPR to never use anything more than 2 and the expectations accordingly, which should also fix the DevTools problem you're seeing.

@patrickhulce patrickhulce self-assigned this Oct 2, 2020
@zenofewords
Copy link
Author

Thank you for the quick feedback!

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

Successfully merging a pull request may close this issue.

3 participants