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-flexbox] content size suggestion of image flex items #6693

Open
davidsgrogan opened this issue Sep 28, 2021 · 8 comments
Open

[css-flexbox] content size suggestion of image flex items #6693

davidsgrogan opened this issue Sep 28, 2021 · 8 comments

Comments

@davidsgrogan
Copy link
Member

davidsgrogan commented Sep 28, 2021

This issue is about the height of the flex item in this case:

<div style="display: flex; flex-direction: column; width:100px; height: 0px;">
  <img src="300x150.png" style="height: 100px;">
</div>

Gecko and Blink both give the flex item height 100px. I think the current specs dictate height 50px.

The item has 0 available height and flex-shrink: 1. So the min-height of the item will be the item's final size.

I think all of us Gecko and Blink layout engineers agree so far. But we've had disagreement in something about what follows, so I am numbering steps for reference.

  1. The min-height is min(specified size suggestion, content size suggestion). specified size suggestion is clearly 100px, so min-height is min(100px, content size suggestion).

  2. Then, content size suggestion is just the min-content height.

  3. min-content height is defined to be the height of the image in this case:

<div style="height: 0px">
  <img src="300x150.png" style="width: 100px; float: left; min-height: 0px; max-height: none;">
</div>
  1. width:100px in step 3 is from flex 9.8.3 saying the "automatic preferred outer cross size of any stretched flex items is the flex container’s inner cross size". The flex container's inner cross size in the original example is 100px. The flex item in the original example has an "automatic preferred outer cross size".

  2. Both engines give that image height 50px. (width:100px in a 300/150 aspect ratio gives height 50px)

  3. So the min-content size of the image in the original example is 50px. So the final height of the flex item in the original example is min(100px, 50px) which is 50px. Both engines are wrong to give height 100px.

spec authors @tabatkins @fantasai: Is 50px wrong? Which step?

aforementioned layout engineers @aethanyc @dholbert @bfgeek @cbiesinger: Is 50px wrong? Which step?

@davidsgrogan davidsgrogan changed the title [css-flexbox] content size suggestion of natural aspect ratio flex items [css-flexbox] content size suggestion of image flex items Sep 28, 2021
@aethanyc
Copy link

I think the issue to discuss is in step 3 & 4. That is, when computing content size suggestion, should we transfer the stretched flex item's cross-size 100px to the main-axis through preferred aspect-ratio? If yes, then the content size suggestion is 50px, which happens to be its transferred size suggestion in this example. If no, then the content size suggestion is the image's natural size 150px as in this case.

<div style="height: 0px">
  <img src="300x150.png" style="float: left; min-height: 0px; max-height: none;">
</div>

@bfgeek
Copy link

bfgeek commented Sep 30, 2021

I'm pretty sure we want to consider the stretch constraint. The way I view the stretch constraint is that it should have all the same effect as width: stretch and/or (mostly) the same effect as width: 100% (and tweaking the testcase we do agree here).

We also agree if we use our prefixed stretch keywords, e.g. -webkit-fill-available / -moz-available:
https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=9678

If not we should clearly describe when stretch doesn't apply.

davidsgrogan referenced this issue in web-platform-tests/wpt Oct 6, 2021
… element.

If the flex item is a non-replaced element and its min-width/min-height
is 'auto', the spec has changed so that it has no transferred size
suggestion now. https://drafts.csswg.org/css-flexbox-1/#min-size-auto

This patch also updates WPT tests to fix
#27878

Differential Revision: https://phabricator.services.mozilla.com/D112830

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1703304
gecko-commit: fb14e6d29a44a79efc296cc727f92aa58ea4dacc
gecko-reviewers: dholbert
@davidsgrogan
Copy link
Member Author

Just for ease of tracking, our related bug reports:
https://crbug.com/1254988
https://crbug.com/1246126

@davidsgrogan
Copy link
Member Author

Agenda+ to hopefully discuss on the 9am Pacific Oct 13 call.

@dholbert
Copy link
Member

I think I agree with the logic in the initial comment here.

I think this line of reasoning would apply to grid item sizing as well, correct? e.g. for this fiddle which I think is the grid version of @davidsgrogan's initial testcase here.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed content size suggestion of large flex items, and agreed to the following:

  • RESOLVED: no substantive change to spec, but clarify the text to avoid confusion
The full IRC log of that discussion <fantasai> Topic: content size suggestion of large flex items
<fantasai> github: https://github.com//issues/6693
<dgrogan> https://jsfiddle.net/dgrogan/ug9rsf2a/
<fantasai> dgrogan: We recently rewrote some sizing code. Blink and Gecko agree, but counter to spec
<fantasai> dgrogan: I you have an image item that is stretched in the cross axis, do you reflect that stretched size through aspect ratio for min-content sizing
<fantasai> dgrogan: neither engine does so, but spec seems to say to do so
<fantasai> dgrogan: currently just arguing over what the spec says
<fantasai> dholbert: I think it makes sense. It also applies to grid
<fantasai> dholbert: whatever sizing ends up, make sure to be similar between flexbox and grid
<fantasai> dgrogan: Not familiar with grid, but iirc grid uses different minimum calculation
<fantasai> dgrogan: so outcome is maybe not the same
<fantasai> iank_: Talked about a very similar case in grid last month
<fantasai> iank_: Basically, there's a bunch of testcases in grid that test this, looking at one axis looking at other for min-size
<astearns> ack fantasai
<TabAtkins> Yes, cross sizing shoudl transfer to main axis. Unless there's a wrinkle I'm missing, I agree with a 50px height, maintaining the aspect ratio.
<florian> fantasai: from what I recall, the intention of the spec is that the size would transfer
<astearns> +1 to maintaining the aspect ratio
<dgrogan> q+
<astearns> ack dgrogan
<fantasai> fantasai: ...
<florian> fantasai: if you don't do that, you're going to end up distorting the aspec ratio
<fantasai> dgrogan: I think we agree that's the behavior we want, though engines don't have it today
<fantasai> dgrogan: This was a long confusing thread on interpreting the spec, would be good to make it clearer
<TYLin> q+
<fantasai> fantasai: I think that's something Tab and I would have to figure out offline :)
<florian> fantasai: tab and I will have to take time and do it offline
<astearns> ack TYLin
<fantasai> astearns: if anyone has any suggestions?
<TabAtkins> yup, don't want to commit to a particular way to write it until i have time to think about it ^_^
<fantasai> TYLin: Note that transfer size and ? are ???
<fantasai> TYLin: and ?? affects content-size suggestion
<fantasai> astearns: is the resolution for this issue no change to spec other than clarification?
<fantasai> dgrogan: I think that's accurate
<fantasai> iank_: Blink will make this change and update the testcases that are wrong
<fantasai> s/.../if you imagine a large image in a short row flex container, you wouldn't want its natural width to be the minimum, you'd want the width reduced in proportion to how the height is reduced by the cross size/
<TYLin> I think the transfer size suggestion and content size suggestion can be merged into one concept, and explicit saying that stretched cross-size should be used in content size suggestion.
<fantasai> RESOLVED: no substantive change to spec, but clarify the text to avoid confusion
<gtalbot> [crikets chirping sound]

@davidsgrogan
Copy link
Member Author

The WPT PR discussion that spawned this thread (with our initial attempts figuring out what the spec dictates) is web-platform-tests/wpt#30900

@davidsgrogan
Copy link
Member Author

I liked @aethanyc's suggestion on IRC about how to clarify this:

I think the transfer size suggestion and content size suggestion can be merged into one concept, and explicit saying that stretched cross-size should be used in content size suggestion.

I share @aethanyc's interpretation that transferred size suggestion is redundant with content size suggestion as they are defined today.

This redundancy contributed to all of our confusion ("if the content size suggestion is 50px then transferred size suggestion is redundant... but why would transferred size suggestion exist if it were redundant? are we sure content size suggestion is 50px?" -- i.e. the canon against surplusage led us astray)

If transferred size suggestion is not a no-op (meaning @aethanyc and I are mistaken) then it would be helpful for us implementers to have the spec include an example in which it makes a difference.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 13, 2021
Content size suggestion is just min-content size. The min-content size
of a replaced element is supposed to account for preferred size in the
opposite axis. We were not doing that for content size suggestion when
the opposize axis preferred size came from a flexbox's fixed cross size.

In the example below, Blink now gives the image content size suggestion
100px because 100 = 300*150/50. Before this patch Blink gave it a
content size suggestion equal to its natural width 300px.

<div style="display: flex; height: 50px;">
  <img src="support/300x150-green.png">
</div>

Firefox matches Blink's behavior before this patch. We may have compat
issues.

See w3c/csswg-drafts#6693 for discussion of
this new behavior.

Bug: 1246126
Change-Id: I1d42c7151b9a0da7e001ca89ea1d7fb9187ae235
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 13, 2021
Content size suggestion is just min-content size. The min-content size
of a replaced element is supposed to account for preferred size in the
opposite axis. We were not doing that for content size suggestion when
the opposize axis preferred size came from a flexbox's fixed cross size.

In the example below, Blink now gives the image content size suggestion
100px because 100 = 300*150/50. Before this patch Blink gave it a
content size suggestion equal to its natural width 300px.

<div style="display: flex; height: 50px;">
  <img src="support/300x150-green.png">
</div>

Firefox matches Blink's behavior before this patch. We may have compat
issues.

See w3c/csswg-drafts#6693 for discussion of
this new behavior.

Bug: 1246126
Change-Id: I1d42c7151b9a0da7e001ca89ea1d7fb9187ae235
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 13, 2021
Content size suggestion is just min-content size. The min-content size
of a replaced element is supposed to account for preferred size in the
opposite axis. We were not doing that for content size suggestion when
the opposize axis preferred size came from a flexbox's fixed cross size.

In the example below, Blink now gives the image content size suggestion
100px because 100 = 300*150/50. Before this patch Blink gave it a
content size suggestion equal to its natural width 300px.

<div style="display: flex; height: 50px;">
  <img src="support/300x150-green.png">
</div>

Firefox matches Blink's behavior before this patch. We may have compat
issues.

See w3c/csswg-drafts#6693 for discussion of
this new behavior.

Bug: 1246126
Change-Id: I1d42c7151b9a0da7e001ca89ea1d7fb9187ae235
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 13, 2021
Content size suggestion is just min-content size. The min-content size
of a replaced element is supposed to account for preferred size in the
opposite axis. We were not doing that for content size suggestion when
the opposize axis preferred size came from a flexbox's fixed cross size.

In the example below, Blink now gives the image content size suggestion
100px because 100 = 300*150/50. Before this patch Blink gave it a
content size suggestion equal to its natural width 300px.

<div style="display: flex; height: 50px;">
  <img src="support/300x150-green.png">
</div>

Firefox matches Blink's behavior before this patch. We may have compat
issues.

See w3c/csswg-drafts#6693 for discussion of
this new behavior.

Bug: 1246126
Change-Id: I1d42c7151b9a0da7e001ca89ea1d7fb9187ae235
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3173922
Commit-Queue: David Grogan <[email protected]>
Reviewed-by: Ian Kilpatrick <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931259}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 13, 2021
Content size suggestion is just min-content size. The min-content size
of a replaced element is supposed to account for preferred size in the
opposite axis. We were not doing that for content size suggestion when
the opposize axis preferred size came from a flexbox's fixed cross size.

In the example below, Blink now gives the image content size suggestion
100px because 100 = 300*150/50. Before this patch Blink gave it a
content size suggestion equal to its natural width 300px.

<div style="display: flex; height: 50px;">
  <img src="support/300x150-green.png">
</div>

Firefox matches Blink's behavior before this patch. We may have compat
issues.

See w3c/csswg-drafts#6693 for discussion of
this new behavior.

Bug: 1246126
Change-Id: I1d42c7151b9a0da7e001ca89ea1d7fb9187ae235
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3173922
Commit-Queue: David Grogan <[email protected]>
Reviewed-by: Ian Kilpatrick <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931259}
pull bot pushed a commit to FreddyZeng/chromium that referenced this issue Oct 14, 2021
Content size suggestion is just min-content size. The min-content size
of a replaced element is supposed to account for preferred size in the
opposite axis. We were not doing that for content size suggestion when
the opposize axis preferred size came from a flexbox's fixed cross size.

In the example below, Blink now gives the image content size suggestion
100px because 100 = 300*150/50. Before this patch Blink gave it a
content size suggestion equal to its natural width 300px.

<div style="display: flex; height: 50px;">
  <img src="support/300x150-green.png">
</div>

Firefox matches Blink's behavior before this patch. We may have compat
issues.

See w3c/csswg-drafts#6693 for discussion of
this new behavior.

Bug: 1246126
Change-Id: I1d42c7151b9a0da7e001ca89ea1d7fb9187ae235
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3173922
Commit-Queue: David Grogan <[email protected]>
Reviewed-by: Ian Kilpatrick <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931259}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 15, 2021
… items' content size suggestion, a=testonly

Automatic update from web-platform-tests
[css-flex] Obey stretch size in replaced items' content size suggestion

Content size suggestion is just min-content size. The min-content size
of a replaced element is supposed to account for preferred size in the
opposite axis. We were not doing that for content size suggestion when
the opposize axis preferred size came from a flexbox's fixed cross size.

In the example below, Blink now gives the image content size suggestion
100px because 100 = 300*150/50. Before this patch Blink gave it a
content size suggestion equal to its natural width 300px.

<div style="display: flex; height: 50px;">
  <img src="support/300x150-green.png">
</div>

Firefox matches Blink's behavior before this patch. We may have compat
issues.

See w3c/csswg-drafts#6693 for discussion of
this new behavior.

Bug: 1246126
Change-Id: I1d42c7151b9a0da7e001ca89ea1d7fb9187ae235
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3173922
Commit-Queue: David Grogan <[email protected]>
Reviewed-by: Ian Kilpatrick <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931259}

--

wpt-commits: 2c83d2e8d545706b565dc6e3dcf77daa77eb5e96
wpt-pr: 30900
aosmond pushed a commit to aosmond/gecko that referenced this issue Oct 16, 2021
… items' content size suggestion, a=testonly

Automatic update from web-platform-tests
[css-flex] Obey stretch size in replaced items' content size suggestion

Content size suggestion is just min-content size. The min-content size
of a replaced element is supposed to account for preferred size in the
opposite axis. We were not doing that for content size suggestion when
the opposize axis preferred size came from a flexbox's fixed cross size.

In the example below, Blink now gives the image content size suggestion
100px because 100 = 300*150/50. Before this patch Blink gave it a
content size suggestion equal to its natural width 300px.

<div style="display: flex; height: 50px;">
  <img src="support/300x150-green.png">
</div>

Firefox matches Blink's behavior before this patch. We may have compat
issues.

See w3c/csswg-drafts#6693 for discussion of
this new behavior.

Bug: 1246126
Change-Id: I1d42c7151b9a0da7e001ca89ea1d7fb9187ae235
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3173922
Commit-Queue: David Grogan <[email protected]>
Reviewed-by: Ian Kilpatrick <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931259}

--

wpt-commits: 2c83d2e8d545706b565dc6e3dcf77daa77eb5e96
wpt-pr: 30900
Gabisampaio pushed a commit to Gabisampaio/wpt that referenced this issue Nov 18, 2021
Content size suggestion is just min-content size. The min-content size
of a replaced element is supposed to account for preferred size in the
opposite axis. We were not doing that for content size suggestion when
the opposize axis preferred size came from a flexbox's fixed cross size.

In the example below, Blink now gives the image content size suggestion
100px because 100 = 300*150/50. Before this patch Blink gave it a
content size suggestion equal to its natural width 300px.

<div style="display: flex; height: 50px;">
  <img src="support/300x150-green.png">
</div>

Firefox matches Blink's behavior before this patch. We may have compat
issues.

See w3c/csswg-drafts#6693 for discussion of
this new behavior.

Bug: 1246126
Change-Id: I1d42c7151b9a0da7e001ca89ea1d7fb9187ae235
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3173922
Commit-Queue: David Grogan <[email protected]>
Reviewed-by: Ian Kilpatrick <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931259}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
Content size suggestion is just min-content size. The min-content size
of a replaced element is supposed to account for preferred size in the
opposite axis. We were not doing that for content size suggestion when
the opposize axis preferred size came from a flexbox's fixed cross size.

In the example below, Blink now gives the image content size suggestion
100px because 100 = 300*150/50. Before this patch Blink gave it a
content size suggestion equal to its natural width 300px.

<div style="display: flex; height: 50px;">
  <img src="support/300x150-green.png">
</div>

Firefox matches Blink's behavior before this patch. We may have compat
issues.

See w3c/csswg-drafts#6693 for discussion of
this new behavior.

Bug: 1246126
Change-Id: I1d42c7151b9a0da7e001ca89ea1d7fb9187ae235
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3173922
Commit-Queue: David Grogan <[email protected]>
Reviewed-by: Ian Kilpatrick <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931259}
NOKEYCHECK=True
GitOrigin-RevId: c28d8dc4b5a05537258252facd533e56bf45ada5
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