Fix image sizing penalty to use CSS terms #99

noamr · 2022-04-26T14:14:35Z

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

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

npm1 · 2022-04-26T15:51:43Z

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

noamr · 2022-04-26T16:58:33Z

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 · 2022-04-27T12:57:19Z

^^ @sefeng211 @emilio

sefeng211 · 2022-04-27T15:05:25Z

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

index.bs

npm1 · 2022-04-28T15:32:13Z

"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 · 2022-04-28T16:16:49Z

"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 · 2022-04-28T16:48:40Z

"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 · 2022-04-29T10:44:46Z

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

noamr · 2022-05-02T13:35:32Z

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.

Tests w3c/largest-contentful-paint#99

sefeng211 · 2022-05-11T22:11:31Z

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

noamr · 2022-05-13T04:28:13Z

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 · 2022-05-13T08:44:28Z

@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 · 2022-05-13T08:47:18Z

@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 · 2022-05-13T08:56:36Z

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 · 2022-05-13T09:09:07Z

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 ?

noamr · 2022-07-17T06:49:30Z

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

npm1

I had looked at this before, but never actually posted the comments

npm1 · 2022-04-26T15:46:15Z

index.bs

-            1. Let |penaltyFactor| be <code>min(|displaySize|, |naturalSize|) / |displaySize|</code>.
-            1. Multiply |size| by |penaltyFactor|.
+        1. If |imageRequest| is not null, run the following steps to reduce the computed size of upscaled images:
+            1. Let (|naturalWidth|, |naturalHeight|) be |imageRequest|'s [=natural dimension=].


Should this be natural dimensions ie plural? Also this is clear but not technically correct as those are a triple and here you have two variables. I think you can use natural sizes?

It's an existing reference to CSS

index.bs

yoavweiss

LGTM

yoavweiss · 2022-07-19T08:56:48Z

index.bs

+            1. Let |concreteDimensions| be |imageRequest|'s [=concrete object size=] within |element|.
+            1. Let |visibleDimensions| be |concreteDimensions|, adjusted for positioning by 'object-position' or 'background-position' and |element|'s [=content box=].
+
+               Note: some of those algorithms are not rigorously defined in CSS. The expected result is to get the actual position and size of the image in |element| as a {{DOMRectReadOnly}}.


I agree with this note, and had the same thoughts while reading through this. Maybe we can file bugs against CSS?

w3c/csswg-drafts#7514

Fix image sizing penalty to use CSS terms

8d97bbf

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

noamr requested review from npm1 and yoavweiss April 26, 2022 14:16

emilio reviewed Apr 27, 2022

View reviewed changes

index.bs Show resolved Hide resolved

noamr added 3 commits May 2, 2022 16:26

Adjust for object position and transforms

b1555e4

Fix build

26e0572

Fix build

6fda7b7

noamr added a commit to web-platform-tests/wpt that referenced this pull request May 4, 2022

LCP: Test different scenarios of image upscaling

bf3b78c

Tests w3c/largest-contentful-paint#99

noamr mentioned this pull request May 4, 2022

LCP: Test different scenarios of image upscaling web-platform-tests/wpt#33930

Merged

Account for 0-image sizes

aa0e9bb

noamr mentioned this pull request May 13, 2022

Specify exclusions for opacity:0, display:none, etc. #79

Open

Fix algorithm

f082051

npm1 reviewed Jul 18, 2022

View reviewed changes

Fix natural dimension ref

fcfa5ea

yoavweiss approved these changes Jul 19, 2022

View reviewed changes

noamr merged commit 2da152d into main Jul 19, 2022

noamr deleted the image-sizing branch July 19, 2022 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix image sizing penalty to use CSS terms #99

Fix image sizing penalty to use CSS terms #99

noamr commented Apr 26, 2022 •

edited by pr-preview bot

npm1 commented Apr 26, 2022

noamr commented Apr 26, 2022 •

edited

yoavweiss commented Apr 27, 2022

sefeng211 commented Apr 27, 2022

npm1 commented Apr 28, 2022

noamr commented Apr 28, 2022 •

edited

noamr commented Apr 28, 2022

yoavweiss commented Apr 29, 2022

noamr commented May 2, 2022

sefeng211 commented May 11, 2022

noamr commented May 13, 2022

emilio commented May 13, 2022

noamr commented May 13, 2022

emilio commented May 13, 2022

noamr commented May 13, 2022

noamr commented Jul 17, 2022

npm1 left a comment

npm1 Apr 26, 2022

noamr Jul 19, 2022

noamr Jul 19, 2022

yoavweiss left a comment

yoavweiss Jul 19, 2022

noamr Jul 19, 2022

Fix image sizing penalty to use CSS terms #99

Fix image sizing penalty to use CSS terms #99

Conversation

noamr commented Apr 26, 2022 • edited by pr-preview bot

npm1 commented Apr 26, 2022

noamr commented Apr 26, 2022 • edited

yoavweiss commented Apr 27, 2022

sefeng211 commented Apr 27, 2022

npm1 commented Apr 28, 2022

noamr commented Apr 28, 2022 • edited

noamr commented Apr 28, 2022

yoavweiss commented Apr 29, 2022

noamr commented May 2, 2022

sefeng211 commented May 11, 2022

noamr commented May 13, 2022

emilio commented May 13, 2022

noamr commented May 13, 2022

emilio commented May 13, 2022

noamr commented May 13, 2022

noamr commented Jul 17, 2022

npm1 left a comment

Choose a reason for hiding this comment

npm1 Apr 26, 2022

Choose a reason for hiding this comment

noamr Jul 19, 2022

Choose a reason for hiding this comment

noamr Jul 19, 2022

Choose a reason for hiding this comment

yoavweiss left a comment

Choose a reason for hiding this comment

yoavweiss Jul 19, 2022

Choose a reason for hiding this comment

noamr Jul 19, 2022

Choose a reason for hiding this comment

noamr commented Apr 26, 2022 •

edited by pr-preview bot

noamr commented Apr 26, 2022 •

edited

noamr commented Apr 28, 2022 •

edited