search

w3c / largest-contentful-paint

Specification for the LargestContentfulPaint API
https://w3c.github.io/largest-contentful-paint/
Other
88 stars 16 forks source link

Fix image sizing penalty to use CSS terms #99

Closed noamr closed 2 years ago

noamr commented 2 years ago

This takes CSS sizing and layout for backgrounds & regular images into account.

Note that because of CSS object positioning, there still might be scenario of a big image that's not intersecting with the viewport but is counted towards LCP.

Closes #98

Preview | Diff

npm1 commented 2 years ago

What is an implementation change resulting from this PR (which I think there are some?)? Do we have tests for such changes?

noamr commented 2 years ago

What is an implementation change resulting from this PR (which I think there are some?)? Do we have tests for such changes?

This is meant, for a start, as a discussion point for #98. If it seems like a good direction I'd be happy to cover this with some tests.

yoavweiss commented 2 years ago

^^ @sefeng211 @emilio

sefeng211 commented 2 years ago

I should say nothing in this PR and just defer to Emilio. (Sorry about push more works to your table)

npm1 commented 2 years ago

"Note that because of CSS object positioning, there still might be scenario of a big image that's not intersecting with the viewport but is counted towards LCP." seems like a problem. Besides this, what are some examples where this change improves the LCP size?

noamr commented 2 years ago

"Note that because of CSS object positioning, there still might be scenario of a big image that's not intersecting with the viewport but is counted towards LCP." seems like a problem. Besides this, what are some examples where this change improves the LCP size?

It's a problem in the sense that you might not actually intersect with the image, but its not directly affecting the image upscaling penalty.it can be handled separately.

This patch would make the upscaling penalty a lot more accurate in cases where css affects the image size and not just the width/height attributes, which is almost always - background-size, object-size, css width/height attributes, flex/grid etc.

You're right in the sense that this is doesn't solve all the issues of image content size.

noamr commented 2 years ago

"Note that because of CSS object positioning, there still might be scenario of a big image that's not intersecting with the viewport but is counted towards LCP." seems like a problem. Besides this, what are some examples where this change improves the LCP size?

It's a problem in the sense that you might not actually intersect with the image, but its not directly affecting the image upscaling penalty.it can be handled separately.

This patch would make the upscaling penalty a lot more accurate in cases where css affects the image size and not just the width/height attributes, which is almost always - background-size, object-size, css width/height attributes, flex/grid etc.

You're right in the sense that this is doesn't solve all the issues of image content size.

A concrete example:

In the current spec, a 1x1 image scaled to 1000px*1000px with CSS (e.g. by having width: 100vh in its style) might be eligible for LCP because its scaling is done via CSS and not via the width/height attribute. this allows devs to "game" LCP by scaling tiny images in this way.

yoavweiss commented 2 years ago

This change makes sense from my perspective. Can you accompany it with tests, to outline what implementations would need to change?

noamr commented 2 years ago

I've updated the patch to also account for object position and transforms. I think it's pretty rigorous now in terms of catching image edge cases. The idea is that it would always return only the intersecting visible rect of the image rather than the element, and divide that area by the image's upscaling factor. I'll start working on WPTs.

sefeng211 commented 2 years ago

The natural dimension can be 0 for svg images, what do we do with this case?

noamr commented 2 years ago

The natural dimension can be 0 for svg images, what do we do with this case?

0-size images should not contribute to LCP :) Added a line to explicitly say that and avoid the div-by-zero.

emilio commented 2 years ago

@noamr this is not about 0-sized images, but about SVG images that don't have an intrinsic size (SVG images can only have an intrinsic ratio for example).

noamr commented 2 years ago

@noamr this is not about 0-sized images, but about SVG images that don't have an intrinsic size (SVG images can only have an intrinsic ratio for example).

What are the current dimensions used to display these if they don't have width/height attributes?

emilio commented 2 years ago

Whatever CSS says. Also this is a place where browsers aren't quite interoperable. IIRC Blink falls back to the 300x150 size that iframes use, but Gecko doesn't.

noamr commented 2 years ago

Whatever CSS says. Also this is a place where browsers aren't quite interoperable. IIRC Blink falls back to the 300x150 size that iframes use, but Gecko doesn't.

Also Firefox falls back to 300x150 for <img src="something.svg" />

I think in the case of unspecified size SVG there shouldn't be an image penalty. Since in SVG upscaling is not a reduction in quality, we might want to exclude this penalty for SVG altogether.

If it's about gaming the metric, I don't think this penalty helps when it comes to SVG. (see https://github.com/w3c/largest-contentful-paint/issues/72).

@yoavweiss @npm1 ?

noamr commented 2 years ago

Whatever CSS says. Also this is a place where browsers aren't quite interoperable. IIRC Blink falls back to the 300x150 size that iframes use, but Gecko doesn't.

Also Firefox falls back to 300x150 for <img src="something.svg" />

I think in the case of unspecified size SVG there shouldn't be an image penalty. Since in SVG upscaling is not a reduction in quality, we might want to exclude this penalty for SVG altogether.

If it's about gaming the metric, I don't think this penalty helps when it comes to SVG. (see #72).

@yoavweiss @npm1 ?

Ping @yoavweiss