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

[css-images-4] Should contain-intrinsic-size affect object-fit? #10116

Open
eeeps opened this issue Mar 21, 2024 · 14 comments
Open

[css-images-4] Should contain-intrinsic-size affect object-fit? #10116

eeeps opened this issue Mar 21, 2024 · 14 comments

Comments

@eeeps
Copy link
Contributor

eeeps commented Mar 21, 2024

This question came up during sizes=auto spec/implementation work.

Here's a test page: https://codepen.io/eeeps/pen/mdgWNJo?editors=1101

WebKit thinks that it should, Chromium thinks that it should, and Firefox refuses to do any kind of object-fitting on size-contained elements.

Notably, @mirisuzanne thinks that it shouldn't, because contain-intrinsic-size doesn't actually change the natural dimensions of replaced elements (Image.naturalWidth/naturalHeight are unaffected), but simply tells the layout algorithm to proceed as if those were the natural dimensions.

I agree with her, and think UAs should use the actual natural dimensions that you get from Image.naturalWidth/naturalHeight, and not the contain-intrinsic-size dimensions, when painting object-fit objects into contain:sized elements. I think we need some more spec language somewhere here, although admittedly my understanding of some of the concepts here is provisional at best.

@mirisuzanne
Copy link
Contributor

The reason for object-fit and related properties is to help align and size the image content inside a box that has different dimensions, or a different aspect ratio - because of some other specified styles. When we start applying those box layout dimensions to the image content, we lose the purpose of the feature.

@tabatkins
Copy link
Member

Agreed, 'object-fit' 'itself clearly requires working on the actual image dimensions, or else it's worthless. The c-i-s dimensions are just to force layout stability. They only have an effect on the image's dimensions as a side-effect, since images default to stretching into their layout dimensions.

I'd have to poke around to figure out what, if anything, needs fixing in the specs; the language around intrinsic/natural dimensions is a bit wonky and there probably needs to be more "layers" of terminology here.

@Loirooriol
Copy link
Contributor

Some previous discussion in #6257

@mirisuzanne
Copy link
Contributor

We could close one of the issues, and add agenda+ to the other? It seems like there's consensus this is broken. And it seems like it should be clarified somewhere.

@vmpstr
Copy link
Member

vmpstr commented Mar 22, 2024

Is this a consequence of contain-intrinsic-size or size containment? Size containment says that

Replaced elements must be treated as having an natural width and height of 0 and no natural aspect ratio.

contain-intrinsic-size definition says:

These properties allow elements with size containment to specify an explicit intrinsic inner size, causing the box to size as if its in-flow content totals to a width and height matching the specified explicit intrinsic inner size (rather than sizing as if it were empty).

I couldn't find the reference for contain-intrinsic-size and natural size interactions.

In particular, in the codepen example (on Chromium), I can't get a different effect if I change contain-intrinsic-size to something like 800px 400px. It always remains a square.

@eeeps
Copy link
Contributor Author

eeeps commented Mar 22, 2024

@vmpstr Try refreshing? This has been flaky for me before. Not sure if that's CodePen's fault, or Chromiums. Anyways here's proof that it does sometimes have an effect: https://o.img.rodeo/c2axgu9ms1l3ojujbjjn.png https://codepen.io/eeeps/pen/abxWVmN?editors=1101

@Loirooriol
Copy link
Contributor

Loirooriol commented Mar 22, 2024

It always remains a square

Open devtools and set display: none, then unset it.

Is this a consequence of contain-intrinsic-size or size containment?

Size containment, which may be affected by contain-intrinsic-size.

I couldn't find the reference for contain-intrinsic-size and natural size interactions.

See the resolution in #7519 (comment).

But as I argued in #6257 (comment), this is just for sizing the <img>. Once we know its size, the spec says to lay out normally. So object-fit should work. And since the example uses width and height, contain-intrinsic-size should have no effect at all.

@vmpstr
Copy link
Member

vmpstr commented Mar 23, 2024

Got it working, thanks all!

And yes, it makes sense that contain-intrinsic-size affects the img element size not the interaction with object-fit

eeeps added a commit to eeeps/wpt that referenced this issue Apr 3, 2024
[css-images] [css-contain]

`object-fit` should use the "real" natural aspect ratio, and `contain: size` should not ruin this

w3c/csswg-drafts#10116
w3c/csswg-drafts#6257
@eeeps
Copy link
Contributor Author

eeeps commented Apr 3, 2024

First stab at a Web Platform Test https://github.com/eeeps/wpt/tree/object-fit-contain-size

@mirisuzanne
Copy link
Contributor

I think the desired resolution here would be what @bfgeek proposed in #6257 (comment) - though it's not clear to me which spec (if any) needs to be clarified. Maybe we're just confirming this, and adding the WPT to prove it? :)

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-images-4] Should `contain-intrinsic-size` affect `object-fit`?, and agreed to the following:

  • RESOLVED: `contain` removes the natural aspect ratio / width / height only for the purposes of sizing and layout of the box (and object-fit is therefore not affected)
The full IRC log of that discussion <TabAtkins1> argh, be there one sec
<fantasai> EricP: object-fit uses the natural size, but there are two natural aspect ratios we care about
<fantasai> EricP: there's the actual natural aspect ratio of the object, and the one we use in layout (affected by contain-intrinsic-size)
<fantasai> EricP: wanted to do auto sizes for ? and were using contain-intrinsic-size
<fantasai> EricP: If you don't do that, you will load every resource in srcset, very bad
<fantasai> EricP: When shipped in Chrome Beta, a number of sites were also using sizes=auto and those images became distorted
<fantasai> EricP: We thought maybe set contain-intrinsic-size in UA stylesheet
<fantasai> EricP: But realized maybe we need two concepts, the intrinsic size for layout vs the intrinsic size for drawing
<vmpstr> q+
<miriam> q+
<TabAtkins> +1 to being explicit in both sites
<astearns> ack vmpstr
<fantasai> EricP: object-fit is looking at the actual intrinsic size, contain is for layout only
<fantasai> vmpstr: Is object-fit the only property that should be affected by actual natural size?
<fantasai> miriam: I'm thinking in the reverse way, the containment features should only remove the natural sizing for layout purposes
<schenney> q+
<fantasai> miriam: the answer is, anything not about laying out the box should respect the natural size
<fantasai> +1
<florian> +1 to miriam
<astearns> ack miriam
<schenney> q-
<fantasai> miriam: This also solves some other problems we've had in the past, where no way to remove the aspect ratio of the image and give it a new one
<fantasai> -> https://www.w3.org/TR/css-sizing-4/#aspect-ratio
<fantasai> miriam: hard to have the image contribute some dimensions without contributing its aspect ratio or nothing
<fantasai> miriam: we discussed awhile back, and ian made a proposal similar here, basically that containment should only contain for purposes of layout, nothing else
<astearns> ack fantasai
<dholbert> scribe+ dandclark
<dandclark> fantasai: When we the aspect ratio prop we added the concept of preferred aspect ratio, which is different from natural aspect ratio. Can we hook into that concept? Agree contain shouldn't hook into natural aspect ratio.
<eeeps> q+
<astearns> ack eeeps
<dandclark> eeeps: I do think we need aspect ratio, and width, and height. Ratio alone isn't enough for modes like object fit scale down
<fantasai> astearns: so can we update the proposed resolution to use natural vs preferred aspect ratio terms?
<dandclark> fantasai: Let's specify what we're trying to get here, and Tab and I can deal with the vocab aspects of it. We're trying to say that contain removes natural aspect ratio for purposes of [missed]. Tab and I can figure out what that means in specs.
<fantasai> s/[missed]/sizing/
<fantasai> PROPOSED: `contain` removes the natural aspect ratio / width / height only for the purposes of sizing and layout
<fantasai> dholbert: maybe clarify that it doesn't impact how object-fit works
<dandclark> fantasai: For sizing and layout of the box, and i.e. object fit is not affected
<fantasai> PROPOSED: `contain` removes the natural aspect ratio / width / height only for the purposes of sizing and layout of the box (and object-fit is therefore not affected)
<fantasai> astearns: Sounds like we're in agreement, might need some wordsmithing to put in the specs
<fantasai> astearns: any objections?
<fantasai> RESOLVED: `contain` removes the natural aspect ratio / width / height only for the purposes of sizing and layout of the box (and object-fit is therefore not affected)
<fantasai> astearns: Any other notes for the minutes?
<fantasai> miriam: notes in the linked issues

eeeps added a commit to eeeps/wpt that referenced this issue Apr 4, 2024
[css-images] [css-contain]

`object-fit` should use the "real" natural aspect ratio, and `contain: size` should not ruin this

w3c/csswg-drafts#10116
w3c/csswg-drafts#6257
eeeps added a commit to eeeps/wpt that referenced this issue Apr 4, 2024
[css-images] [css-contain]

`object-fit` should use the "real" natural aspect ratio, and `contain: size` should not ruin this

w3c/csswg-drafts#10116
w3c/csswg-drafts#6257
tcaptan-cr pushed a commit to web-platform-tests/wpt that referenced this issue Apr 10, 2024
* Test interaction between object-fit and contain:size

[css-images] [css-contain]

`object-fit` should use the "real" natural aspect ratio, and `contain: size` should not ruin this

w3c/csswg-drafts#10116
w3c/csswg-drafts#6257

* Change image-rendering: crisp-edges to pixelated

Chromium doesn’t support `crisp-edges`

* Remove whitespace to make linter happy
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 15, 2024
The resolution of issue 10116 [1]
"Should contain-intrinsic-size affect object-fit?" was that:
"`contain` removes the natural aspect ratio / width / height only for
the purposes of sizing and layout of the box (and object-fit is
therefore not affected)".

Therefore `LayoutReplaced::IntrinsicSize`, used by
`LayoutReplaced::ComputeObjectFitAndPositionRect`, should return the
actual intrinsic size of a replaced element.

[1]
w3c/csswg-drafts#10116

Low-Coverage-Reason: TRIVIAL_CHANGE - no effective change

Bug: 332775468
Change-Id: I9cbdcb45a392145586a3ee7b3f06f46ce2301931
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 15, 2024
The resolution of issue 10116 [1]
"Should contain-intrinsic-size affect object-fit?" was that:
"`contain` removes the natural aspect ratio / width / height only for
the purposes of sizing and layout of the box (and object-fit is
therefore not affected)".

Therefore, `LayoutReplaced::ComputeObjectFitAndPositionRect` should
use the natural size rather than the overridden intrinsic size. After
analyzing all callsites of `LayoutReplaced::IntrinsicSize`, it was
found that they all should use the natural size rather than the
overridden intrinsic size, so this core function was updated.

[1]
w3c/csswg-drafts#10116

Low-Coverage-Reason: TRIVIAL_CHANGE - no effective change

Bug: 332775468
Change-Id: I9cbdcb45a392145586a3ee7b3f06f46ce2301931
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 15, 2024
The resolution of issue 10116 [1]
"Should contain-intrinsic-size affect object-fit?" was that:
"`contain` removes the natural aspect ratio / width / height only for
the purposes of sizing and layout of the box (and object-fit is
therefore not affected)".

Therefore, `LayoutReplaced::ComputeObjectFitAndPositionRect` should
use the natural size rather than the overridden intrinsic size. After
analyzing all callsites of `LayoutReplaced::IntrinsicSize`, it was
found that they all should use the natural size rather than the
overridden intrinsic size, so this core function was updated.

[1]
w3c/csswg-drafts#10116

Low-Coverage-Reason: TRIVIAL_CHANGE - no effective change

Bug: 332775468
Change-Id: I9cbdcb45a392145586a3ee7b3f06f46ce2301931
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 15, 2024
The resolution of issue 10116 [1]
"Should contain-intrinsic-size affect object-fit?" was that:
"`contain` removes the natural aspect ratio / width / height only for
the purposes of sizing and layout of the box (and object-fit is
therefore not affected)".

Therefore, `LayoutReplaced::ComputeObjectFitAndPositionRect` should
use the natural size rather than the overridden intrinsic size. After
analyzing all callsites of `LayoutReplaced::IntrinsicSize`, it was
found that they all should use the natural size rather than the
overridden intrinsic size, so this core function was updated.

[1]
w3c/csswg-drafts#10116

Low-Coverage-Reason: TRIVIAL_CHANGE - no effective change

Bug: 332775468
Change-Id: I9cbdcb45a392145586a3ee7b3f06f46ce2301931
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 16, 2024
The resolution of issue 10116 [1]
"Should contain-intrinsic-size affect object-fit?" was that:
"`contain` removes the natural aspect ratio / width / height only for
the purposes of sizing and layout of the box (and object-fit is
therefore not affected)".

Therefore, `LayoutReplaced::ComputeObjectFitAndPositionRect` should
use the natural size rather than the overridden intrinsic size. After
analyzing all callsites of `LayoutReplaced::IntrinsicSize`, it was
found that they all should use the natural size rather than the
overridden intrinsic size, so this core function was updated.

[1]
w3c/csswg-drafts#10116

Low-Coverage-Reason: TRIVIAL_CHANGE - no effective change

Bug: 332775468
Change-Id: I9cbdcb45a392145586a3ee7b3f06f46ce2301931
aarongable pushed a commit to chromium/chromium that referenced this issue Apr 16, 2024
The resolution of issue 10116 [1]
"Should contain-intrinsic-size affect object-fit?" was that:
"`contain` removes the natural aspect ratio / width / height only for
the purposes of sizing and layout of the box (and object-fit is
therefore not affected)".

Therefore, `LayoutReplaced::ComputeObjectFitAndPositionRect` should
use the natural size rather than the overridden intrinsic size. After
analyzing all callsites of `LayoutReplaced::IntrinsicSize`, it was
found that they all should use the natural size rather than the
overridden intrinsic size, so this core function was updated.

[1]
w3c/csswg-drafts#10116

Low-Coverage-Reason: TRIVIAL_CHANGE - no effective change

Bug: 332775468
Change-Id: I9cbdcb45a392145586a3ee7b3f06f46ce2301931
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5442533
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Traian Captan <tcaptan@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1287761}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 16, 2024
The resolution of issue 10116 [1]
"Should contain-intrinsic-size affect object-fit?" was that:
"`contain` removes the natural aspect ratio / width / height only for
the purposes of sizing and layout of the box (and object-fit is
therefore not affected)".

Therefore, `LayoutReplaced::ComputeObjectFitAndPositionRect` should
use the natural size rather than the overridden intrinsic size. After
analyzing all callsites of `LayoutReplaced::IntrinsicSize`, it was
found that they all should use the natural size rather than the
overridden intrinsic size, so this core function was updated.

[1]
w3c/csswg-drafts#10116

Low-Coverage-Reason: TRIVIAL_CHANGE - no effective change

Bug: 332775468
Change-Id: I9cbdcb45a392145586a3ee7b3f06f46ce2301931
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5442533
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Traian Captan <tcaptan@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1287761}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 16, 2024
The resolution of issue 10116 [1]
"Should contain-intrinsic-size affect object-fit?" was that:
"`contain` removes the natural aspect ratio / width / height only for
the purposes of sizing and layout of the box (and object-fit is
therefore not affected)".

Therefore, `LayoutReplaced::ComputeObjectFitAndPositionRect` should
use the natural size rather than the overridden intrinsic size. After
analyzing all callsites of `LayoutReplaced::IntrinsicSize`, it was
found that they all should use the natural size rather than the
overridden intrinsic size, so this core function was updated.

[1]
w3c/csswg-drafts#10116

Low-Coverage-Reason: TRIVIAL_CHANGE - no effective change

Bug: 332775468
Change-Id: I9cbdcb45a392145586a3ee7b3f06f46ce2301931
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5442533
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Traian Captan <tcaptan@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1287761}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 19, 2024
…d `contain: size`, a=testonly

Automatic update from web-platform-tests
Test interaction between `object-fit` and `contain: size` (#45563)

* Test interaction between object-fit and contain:size

[css-images] [css-contain]

`object-fit` should use the "real" natural aspect ratio, and `contain: size` should not ruin this

w3c/csswg-drafts#10116
w3c/csswg-drafts#6257

* Change image-rendering: crisp-edges to pixelated

Chromium doesn’t support `crisp-edges`

* Remove whitespace to make linter happy
--

wpt-commits: ae05e1ac2530c57f3e6205c50d6176b4875098ff
wpt-pr: 45563
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue Apr 22, 2024
…d `contain: size`, a=testonly

Automatic update from web-platform-tests
Test interaction between `object-fit` and `contain: size` (#45563)

* Test interaction between object-fit and contain:size

[css-images] [css-contain]

`object-fit` should use the "real" natural aspect ratio, and `contain: size` should not ruin this

w3c/csswg-drafts#10116
w3c/csswg-drafts#6257

* Change image-rendering: crisp-edges to pixelated

Chromium doesn’t support `crisp-edges`

* Remove whitespace to make linter happy
--

wpt-commits: ae05e1ac2530c57f3e6205c50d6176b4875098ff
wpt-pr: 45563
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 23, 2024
… aspect ratio, a=testonly

Automatic update from web-platform-tests
object-fit should use the "real" natural aspect ratio

The resolution of issue 10116 [1]
"Should contain-intrinsic-size affect object-fit?" was that:
"`contain` removes the natural aspect ratio / width / height only for
the purposes of sizing and layout of the box (and object-fit is
therefore not affected)".

Therefore, `LayoutReplaced::ComputeObjectFitAndPositionRect` should
use the natural size rather than the overridden intrinsic size. After
analyzing all callsites of `LayoutReplaced::IntrinsicSize`, it was
found that they all should use the natural size rather than the
overridden intrinsic size, so this core function was updated.

[1]
w3c/csswg-drafts#10116

Low-Coverage-Reason: TRIVIAL_CHANGE - no effective change

Bug: 332775468
Change-Id: I9cbdcb45a392145586a3ee7b3f06f46ce2301931
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5442533
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Traian Captan <tcaptan@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1287761}

--

wpt-commits: 30a7e26a85a65544133fd257e8af644f785e04f7
wpt-pr: 45723
vinnydiehl pushed a commit to vinnydiehl/mozilla-unified that referenced this issue Apr 24, 2024
… aspect ratio, a=testonly

Automatic update from web-platform-tests
object-fit should use the "real" natural aspect ratio

The resolution of issue 10116 [1]
"Should contain-intrinsic-size affect object-fit?" was that:
"`contain` removes the natural aspect ratio / width / height only for
the purposes of sizing and layout of the box (and object-fit is
therefore not affected)".

Therefore, `LayoutReplaced::ComputeObjectFitAndPositionRect` should
use the natural size rather than the overridden intrinsic size. After
analyzing all callsites of `LayoutReplaced::IntrinsicSize`, it was
found that they all should use the natural size rather than the
overridden intrinsic size, so this core function was updated.

[1]
w3c/csswg-drafts#10116

Low-Coverage-Reason: TRIVIAL_CHANGE - no effective change

Bug: 332775468
Change-Id: I9cbdcb45a392145586a3ee7b3f06f46ce2301931
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5442533
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Traian Captan <tcaptan@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1287761}

--

wpt-commits: 30a7e26a85a65544133fd257e8af644f785e04f7
wpt-pr: 45723
@eeeps
Copy link
Contributor Author

eeeps commented Jul 16, 2024

Ran into this today in Safari and submitted https://bugs.webkit.org/show_bug.cgi?id=276681

@eeeps
Copy link
Contributor Author

eeeps commented Jan 22, 2025

A patch was submitted to the WebKit bug today. https://bugs.webkit.org/show_bug.cgi?id=276681#c2

So perhaps soon we will have three implementations, tests, but no spec yet.

@Loirooriol
Copy link
Contributor

Even though clarifying the spec would be good, it already implies the resolved behavior, as I explained in #6257 (comment)

webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Jan 23, 2025
https://bugs.webkit.org/show_bug.cgi?id=276681
<rdar://problem/131866042>

Reviewed by Antti Koivisto and Tim Nguyen.

This change aligns WebKit with w3c/csswg-drafts#10116 resolution:

"'contain' removes the natural aspect ratio / width / height only for
the purposes of sizing and layout of the box (and object-fit is
therefore not affected)".

* LayoutTests/TestExpectations:
* Source/WebCore/rendering/RenderReplaced.cpp:
(WebCore::RenderReplaced::paint):

Canonical link: https://commits.webkit.org/289287@main
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