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] column wrap intrinsic size algorithm can make inline min-content > max-content #6777

Open
davidsgrogan opened this issue Oct 29, 2021 · 7 comments

Comments

@davidsgrogan
Copy link
Member

davidsgrogan commented Oct 29, 2021

https://drafts.csswg.org/css-flexbox/#intrinsic-cross-sizes

Flex's intrinsic cross size algorithm can lead to inline min-content > max-content for a column-wrap container.

In the example below, the two items are identical. They each have min/max-content contribution of 75px,100px.

To determine container's max-content size, we lay out each item with available width 100px[1]. The items' resulting heights are each 50px, so we can fit both of them in a single flex line of width 100px. So container's max-content size is 100px.

To determine container's min-content size, we lay out each item with available width 75px[1]. Laying out the first item with available width 75px gives the item a height of 100px. The container is 100px tall so this item fills an entire flex line. item2 is forced into a second flex line. The width of each flex line is 75px, so container's min-content size is 150px.

So min-content size 150px > max-content size 100px, which seems bad.

The problem is exacerbated if the container has column-gap > 0.

<div style="display: flex; flex-flow: column wrap; height: 100px">
  <div id=item1>
    <div style="display: inline-block; style="width: 75px; height: 50px;"></div>
    <div style="display: inline-block; style="width: 25px; height: 50px;"></div>
  </div>
  <div id=item2>
    <div style="display: inline-block; style="width: 75px; height: 50px;"></div>
    <div style="display: inline-block; style="width: 25px; height: 50px;"></div>
  </div>
</div>

Strawperson possible solutions:

  1. Give the items the same available inline space no matter if the flex container is being sized under a min-content or max-content constraint.
    • this would result in min-content = max-content for all column wrap containers
    • that seems suboptimal, but it's still an unequivocal improvement over what engines do today
  2. Floor max-content by min-content. I.e. final max-content = max(min-content, max-content)
    • seems kludgy

[1] From the spec link above:

if the flex container is flex-flow: column wrap;, then it’s sized by first finding the largest min-content/max-content cross-size contribution among the flex items (respectively), then using that size as the available space in the cross axis for each of the flex items during layout.

/cc @bfgeek

@bfgeek bfgeek added the css-flexbox-1 Current Work label Nov 4, 2021
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-flexbox] column wrap intrinsic size algorithm can make inline min-content > max-content.

The full IRC log of that discussion <emilio> topic: [css-flexbox] column wrap intrinsic size algorithm can make inline min-content > max-content
<emilio> github: https://github.com//issues/6777
<emilio> dgrogan: so... background is that all engines have shipped the same intrinsic sizing algo for years
<emilio> ... but the algorithm is bad
<emilio> ... fantasai specified a new one in 2015 but nobody implemented it
<emilio> ... authors really want it
<emilio> ... but there's a big compat risk here
<emilio> ... but we want to ship it along everyone else so that authors just fix sites once
<emilio> ... we started implementing it and found this issue where min can be > max
<emilio> ... details are in the issue, there's a few possible things we can do
<emilio> ... one is flooring max to min
<emilio> ... or in this case making min the max content
<Rossen_> q?
<emilio> fantasai: I'm really excited you're working on this
<emilio> dgrogan: yeah authors really want this and current algorithm is bad
<emilio> TabAtkins: I see the problem, not sure what the best solution is but that's wrong
<emilio> ... fantasai and me will review asap
<emilio> iank_: we might testing the new thing in our canary channels and be more concrete about the compat risks
<emilio> ... we might have to go back for the old thing if we get tons of reports
<emilio> ... we might also want to do this incrementally, first for single-items flexboxes and so on
<fantasai> s/single-items/single-row/

@davidsgrogan
Copy link
Member Author

Ian mentioned this in the call but it didn't make it to the minutes:

We don't want to start shipping the new algorithm, even piecemeal in Canary, until someone has checked our tests (which validates that we understood and implemented the spec correctly). Just spot-checking a few would helpful. https://github.com/web-platform-tests/wpt/blob/master/css/css-flexbox/intrinsic-size/row-005.html has 7 cases. The 6th is annotated with some intermediate calculations, if you want to start there.

@fantasai
Copy link
Collaborator

So the goal of the min-content and max-content sizes are to represent the smallest and largest sizes that the box and its contents could lay out into without triggering overflow or leaving excess space. And the intention is that the box, when set at that size, would lay out accordingly, with its contents in its fully compressed or fully unwound state, fitting its container exactly.

But for column wrap flex items, it doesn't quite work that way. If we figured out the right size for a min-content layout, for example, we'd size the flex container at that size... and then tell its items to lay out into their container. Which means they'd lay out against the available space we calculated for all the flex lines combined, rather than the min-content size of their own flex line. There's basically no good answer here.

I think the best (least nonsensical) thing we can do for the min-content size is to treat it as a single-column flex container, and just take the largest min-content contribution among the items. Which will result in overflow if the flex container has a height constraint, but at least there's some relationship between this size and fitting the contents.

For the max-content size, if we lay out each item at its max-content size, fill the columns, and then wrap a box around that, we can get a size that fits its contents exactly most of the time. The exception would be if we have percentage-sized items: they'll try to resolve against the width calculated for all the flex lines combined, and be too big. Maybe we can avoid that by suppressing percentage resolution (treating as auto) in these cases, i.e. when the percentage is cyclic, like we do for percentage heights in block layout.

Note that this is essentially just a direct translation of what we earlier determined was the desired behavior for multicol under these situations: https://github.com/w3c/csswg-drafts/blob/main/css-sizing-4/intrinsic-sizing-notes.bs#L124 - under a min-content constraint without a set number of columns, assume it's just one column; under a max-content constraint with restrained height, "just do layout" and return the result.

Next interesting question is what about row wrap flex containers. Do we need to do something special here as well?

Consider this case:

item { 
    grid-template-rows: minmax(10px, 100px);
}

<outer height="min-content">
  <flex container row wrap width="tiny">
    <item/>
    <item/>
  </flex>
</outer>

I think the two options we have here are:

  • Do the same as for column wrap containers: take the min-content size as if it were a single row; or
  • Always size wrappable flex lines under a max-content constraint, never passing in a cyclic height or a min-content constraint from their container.

The tricky part here is going to knowing when to break cyclic sizing.

Neither option is great, but they at least get a size that relates to the layout, rather than a size that's effectively random, being derived from a layout that we no longer have. (#1 gives you a 10px-tall flexbox, overflowing due to having two 10px-tall items; #2 gives you a 200px tall flexbox containing two 100px-tall items. The current spec gives you a 20px-tall flexbox, overflowing due to having two 20px-tall items, which is effectively random as far as the author is concerned.)

Thoughts?

@astearns astearns removed the Agenda+ label Mar 28, 2022
@davidsgrogan
Copy link
Member Author

davidsgrogan commented Apr 12, 2022

I think the best (least nonsensical) thing we can do for the min-content size is to treat it as a single-column flex container, and just take the largest min-content contribution among the items. Which will result in overflow if the flex container has a height constraint, but at least there's some relationship between this size and fitting the contents.

Intuitively, I'd guess that most column-wrap containers DO have a height constraint. Because otherwise, why would an author use a multiline container in the first place? Single-line seems more appropriate for a column container with an indefinite height. I suppose someone would use multiline on an indefinite height container If they don't want the flex line to stretch to the full width of the container, but that strikes me as niche.

All that said, I'm happy to apply the multicol precedent to column-wrap flex containers. Hopefully there won't be that many overflow problems in practice.

For the max-content size, if we lay out each item at its max-content size, fill the columns, and then wrap a box around that, we can get a size that fits its contents exactly most of the time. The exception would be if we have percentage-sized items: they'll try to resolve against the width calculated for all the flex lines combined, and be too big. Maybe we can avoid that by suppressing percentage resolution (treating as auto) in these cases, i.e. when the percentage is cyclic, like we do for percentage heights in block layout.

I suppose the competing principles are (1) we want to obey whatever authors specify (i.e. not treat percent widths as auto) but (2) we don't want the container's max-content width to result in overflow. I found it so frustrating to change properties and not see any subsequent change when I was a key-mashing web developer so I'd put (1) over (2) even though (1) results in some overflow. If the author doesn't want the overflow, they'll figure out to remove the specified percent width, which is what (2) would do for them and not give them a choice to opt out.

Next interesting question is what about row wrap flex containers. Do we need to do something special here as well?

Consider this case:

item { 
    grid-template-rows: minmax(10px, 100px);
}

<outer height="min-content">
  <flex container row wrap width="tiny">
    <item/>
    <item/>
  </flex>
</outer>

I think the two options we have here are:

  • Do the same as for column wrap containers: take the min-content size as if it were a single row; or
  • Always size wrappable flex lines under a max-content constraint, never passing in a cyclic height or a min-content constraint from their container.

The tricky part here is going to knowing when to break cyclic sizing.

Neither option is great, but they at least get a size that relates to the layout, rather than a size that's effectively random, being derived from a layout that we no longer have. (#1 gives you a 10px-tall flexbox, overflowing due to having two 10px-tall items; #2 gives you a 200px tall flexbox containing two 100px-tall items. The current spec gives you a 20px-tall flexbox, overflowing due to having two 20px-tall items, which is effectively random as far as the author is concerned.)

Agree. We aren't going to implement min/max-content block sizes anytime soon so I didn't consider this issue.

I don't see why we'd deviate from #1, if #1 is what we did for multicol and will do for column wrap inline sizes. Like, what is substantially different about the row-wrap container that we would do something different? If we go with #2, would whatever reasoning we use to choose #2 also apply to column-wrap flex containers discussed above? (Sorry, I know this isn't very helpful.)

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 9, 2022
min-content width algorithm is unchanged, which matches old behavior and
what was suggested on w3c/csswg-drafts#6777,
but I'm reserving the right to modify it to reduce the frequency of
overflow.

Bug: 240765
Change-Id: I91a6180f04e9514eec8d3a1ea2fdbda24b598096
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 9, 2022
min-content width algorithm is unchanged, which matches old behavior and
what was suggested on w3c/csswg-drafts#6777,
but I'm reserving the right to modify it to reduce the frequency of
overflow.

Bug: 240765
Change-Id: I91a6180f04e9514eec8d3a1ea2fdbda24b598096
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 9, 2022
min-content width algorithm is unchanged, which matches old behavior and
what was suggested on w3c/csswg-drafts#6777,
but I'm reserving the right to modify it to reduce the frequency of
overflow.

Bug: 240765
Change-Id: I91a6180f04e9514eec8d3a1ea2fdbda24b598096
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 10, 2022
min-content width algorithm is unchanged, which matches old behavior and
what was suggested on w3c/csswg-drafts#6777,
but I'm reserving the right to modify it to reduce the frequency of
overflow.

Bug: 240765
Change-Id: I91a6180f04e9514eec8d3a1ea2fdbda24b598096
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 10, 2022
min-content width algorithm is unchanged, which matches old behavior and
what was suggested on w3c/csswg-drafts#6777,
but I'm reserving the right to modify it to reduce the frequency of
overflow.

Bug: 240765
Change-Id: I91a6180f04e9514eec8d3a1ea2fdbda24b598096
aarongable pushed a commit to chromium/chromium that referenced this issue Aug 10, 2022
min-content width algorithm is unchanged, which matches old behavior and
what was suggested on w3c/csswg-drafts#6777,
but I'm reserving the right to modify it to reduce the frequency of
overflow.

Bug: 240765
Change-Id: I91a6180f04e9514eec8d3a1ea2fdbda24b598096
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3740999
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: David Grogan <dgrogan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1033350}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 10, 2022
min-content width algorithm is unchanged, which matches old behavior and
what was suggested on w3c/csswg-drafts#6777,
but I'm reserving the right to modify it to reduce the frequency of
overflow.

Bug: 240765
Change-Id: I91a6180f04e9514eec8d3a1ea2fdbda24b598096
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3740999
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: David Grogan <dgrogan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1033350}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 10, 2022
min-content width algorithm is unchanged, which matches old behavior and
what was suggested on w3c/csswg-drafts#6777,
but I'm reserving the right to modify it to reduce the frequency of
overflow.

Bug: 240765
Change-Id: I91a6180f04e9514eec8d3a1ea2fdbda24b598096
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3740999
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: David Grogan <dgrogan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1033350}
@fantasai
Copy link
Collaborator

So to summarize from the background logic in #6777 (comment) and @davidsgrogan's preferences expressed in #6777 (comment) we're looking at:

  • For min-content cross-axis size of a column wrap flex container, effectively assume it's one column wide and take the largest min-content size among all the items. If the flex container has a height constraint, this will result in overflow, but at least if you set overflow on it's large enough to fit any given column entirely within its scrollport.
  • For max-content cross-axis size of a column wrap flex container, lay out each item at its max-content size, fill the columns, and then wrap a box around that. This will yield a box sized to its contents exactly most of the time, the exception being if the author used percentage sizing (which gives us a cyclic percentage that, if we allow it to resolve, can result in overflow).

Agenda+ to ask the WG if this is the direction we want to go in.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 5, 2022
…hm for column wrap flexboxes, a=testonly

Automatic update from web-platform-tests
[css-flex] New max-content width algorithm for column wrap flexboxes

min-content width algorithm is unchanged, which matches old behavior and
what was suggested on w3c/csswg-drafts#6777,
but I'm reserving the right to modify it to reduce the frequency of
overflow.

Bug: 240765
Change-Id: I91a6180f04e9514eec8d3a1ea2fdbda24b598096
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3740999
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: David Grogan <dgrogan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1033350}

--

wpt-commits: c1783313cda795ef118c6974015ee47de58a0f80
wpt-pr: 35269
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed column wrap intrinsic size algorithm can make inline min-content > max-content, and agreed to the following:

  • RESOLVED: Accept the proposal in the issue
The full IRC log of that discussion <heycam> Topic: column wrap intrinsic size algorithm can make inline min-content > max-content
<heycam> github: https://github.com//issues/6777
<heycam> dgrogan: this one is similar to the last one
<fantasai> https://github.com//issues/6777
<fantasai> proposal: https://github.com//issues/6777#issuecomment-1220003107
<heycam> ... there have been spec proposal, but fantasai made a good proposal, implemented it not shipped, but it works well
<TabAtkins> summary at https://github.com//issues/6777#issuecomment-1220003107
<heycam> ... from calculating the min-content width of a column wrapped flexbox, instead of laying out all the items at their min-content width and then taking the result and calling that hte min-content width
<heycam> ... we'll just ignore wrapping
<heycam> ... so the min-content width of a column wrapped flexbox will be the same as a non-wrapped flexbox
<heycam> ... downside will be is there can be some overflow if the column wrapped flexbox has a definite height specified, such that the items need to wrap, but now they can't
<heycam> ... also point out this proposal is what all browsers implement today
<heycam> ... so it seems fine
<heycam> dholbert: seems fine to me
<heycam> fantasai: that's for min0content sizes
<heycam> ... guarantees even if you have overflow columns, the width of the box will be as wide as the widest column
<heycam> ... trickier is max-content size, where there's no real good answer
<heycam> ... proposal is to lay out each item at its max-content size, then try to build the columsn, then try to wrap a box around that set of columns
<heycam> ... mostly that should be a box that can size its contents exactly, but if the author used %age sizing there'd be cyclic effects
<heycam> s/min0content/min-content/
<heycam> dgrogan: what fantasai just proposed is already specced
<heycam> RESOLVED: Accept the proposal in the issue

tabatkins added a commit that referenced this issue Oct 3, 2022
…ti-line column flexboxes act like it's a single column. #6777
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
min-content width algorithm is unchanged, which matches old behavior and
what was suggested on w3c/csswg-drafts#6777,
but I'm reserving the right to modify it to reduce the frequency of
overflow.

Bug: 240765
Change-Id: I91a6180f04e9514eec8d3a1ea2fdbda24b598096
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3740999
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: David Grogan <dgrogan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1033350}
NOKEYCHECK=True
GitOrigin-RevId: 3447e6fff1473a2f7b66d1c70eef9d1a195e8de5
@gsnedders
Copy link
Member

Is there any reason why this hasn't been resolved? It appears the necessary edits were made in 2022 in 5630e7b.

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