Navigation Menu

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-grid] Intrinsic contributions for items spanning flex tracks are not web compatible #4783

Closed
Loirooriol opened this issue Feb 17, 2020 · 9 comments
Labels
Closed Accepted by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-grid-1 Tested Memory aid - issue has WPT tests

Comments

@Loirooriol
Copy link
Contributor

Loirooriol commented Feb 17, 2020

The behavior used to be:

  • If an item with span 1 is in an intrinsic flex track, its intrinsic contribution is taken into account.
    This is done in §11.5.2 simultaneously with contributions of items with span 1 in intrinsic non-flex tracks.
  • If an item with span>1 includes a flex track, its intrinsic contribution is ignored.

The spec was later changed (#2177 and follow-ups) to make it more consistent:

  • Intrinsic contributions of items spanning some intrinsic track are always taken into account.
  • For items with span>=1 that include some intrinsic flex track, this is done grouping all such items together (not by span). And it happens in §11.5.4, after distributing contributions of items with span>1 that don't include any intrinsic flex track.

Firefox hasn't implemented the new behavior (bug 1530097). I did it in Chromium (bug 935102), but it doesn't seem web compatible (bug 1051039 and multiple duplicates), so I had to revert. There are basically 2 problems.

Problem 1

It seems that it's kinda popular to divide the grid into 12 equal columns, and then add a class to the items specifying how many columns should be spanned.

.grid { display: grid; grid-template-columns: repeat(12, 1fr) }
.col-8 { grid-column: span 8; background: green }
.col-4 { grid-column: span 4; background: red }
.single-line { white-space: nowrap; overflow: hidden; text-overflow: ellipsis }
<div class="grid">
  <div class="col-8">
    <div class="single-line">Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nulla sed erat in ante posuere ornare vel vel mi. Curabitur et mi ac ex suscipit ultrices. Fusce non enim rhoncus, rhoncus sapien ut, ullamcorper mauris. Etiam nec lectus id nulla sagittis vehicula sed vitae orci. Praesent iaculis sodales ipsum. Nam placerat, quam ut facilisis fermentum, augue nibh condimentum nunc, in mattis turpis magna at risus. In ante nunc, sagittis ac vestibulum dapibus, laoreet in lectus.</div>
  </div>
  <div class="col-4">Foo</div>
</div>

Used to look like

problem1-before

But gotcha, 1fr means means minmax(auto, 1fr), so the columns are intrinsic! If the contents of some column are big, they will make the column big too.

problem1-after

Note that the spec change was totally reasonable, it just made the testcase behave like

.grid { display: grid; grid-template-columns: 8fr 4fr }
.col-8, .col-4 { grid-column: span 1 }

Authors should be using minmax(0, 1fr) instead, or min-width: 0. But they are using 1fr because it's simpler and used to work as they wanted.

Problem 2

This affects authors who are aware that 1fr is intrinsic and may "explode" the grid. They didn't want this, so they sized the flex columns with minmax(0, 1fr). However, they also have non-flex columns, and items spanning both kinds of columns:

.grid { display: grid; grid-template-columns: auto minmax(0, 1fr) }
.item1 { background: cyan }
.item2 { background: green }
.item3 { grid-column: span 2; background-color: red }
<div class="grid">
  <div class="item1">Foo</div>
  <div class="item2">Bar</div>
  <div class="item3">Lorem ipsum dolor sit amet, consectetur adipiscing elit.
</div>

The auto column used to be sized tight for item1, and the flex column took the remaining space:

problem2-before

However, with the change, the intrinsic contribution of item3 is no longer ignored. And since the flex column is not intrinsic, the whole contribution is distributed into the 1st column:

problem2-b

So using minmax(0, 1fr) seems a mistake, authors should have used 1fr so that the column remains intrinsic, but then use min-width: 0 to set the minimum contribution of item2 and item3 to 0.

According to bug 1051039#c21, minmax(0, 1fr) got popular from https://css-tricks.com/preventing-a-grid-blowout/, https://stackoverflow.com/q/52861086, "numerous other questions and their answers on Stackoverflow", and "even Rachel Andrews recommends to use minmax(0, Nfr)".

Possible solution

It would be a pity to go back to ignoring intrinsic contributions of items spanning multiple tracks with at least 1 flexible. But there could be a less drastic solution, hopefully still web compatible.

I think we could say that the automatic minimum size should be 0 if the item spans multiple tracks with at least 1 flexible. Assuming that width/height behaves as auto, then this will set the minimum contribution to 0. So, the intrinsic contributions will be ignored by default as they used to, but they will be respected

  • If you size the grid item explicitly, e.g. set width: 100px or min-width: 100px
  • If you size the column with a non-auto intrinsic minimum, e.g. minmax(min-content, 1fr).
  • When sizing the grid under a min-content or max-content constraint (since then an auto min track sizing function uses the limited min-/max-content contribution instead of the minimum contribution).

Cc @fantasai, @tabatkins, @MatsPalmgren, @rachelandrew, @mrego

@Loirooriol
Copy link
Contributor Author

Loirooriol commented Feb 18, 2020

I spoke too fast, that alone won't solve problem 2 because auto as a max track sizing function means max-content. So even if we make the minimum contribution be 0, we would distribute the max-content contribution into the growth limit. And §11.6. Maximize Tracks happens before §11.7. Expand Flexible Tracks, so we would get the same result.

Maybe the change from #3705 should be reverted, i.e. if an item spans some flex track, only distribute its intrinsic contribution among flex tracks. And if no flex track is intrinsic, so be it, no contribution will be distributed. This used to be a bit problematic because could mean that the automatic minimum size could force an item to become bigger than its grid area. But this should be fine if the automatic minimum size becomes 0 when the item spans multiple tracks with at least 1 flexible.

@fantasai
Copy link
Collaborator

@Loirooriol So to summarize, you're suggesting to

  • zero out the automatic minimum size of any item of span > 1 that crosses a flexible track (but not those of span = 1)
  • revert 90511c6 so that flexible tracks are treated the same in intrinsic size distribution, regardless of whether they have a fixed or intrinsic min size

?

@Loirooriol
Copy link
Contributor Author

Yes, exactly.

@franktopel

This comment has been minimized.

@Loirooriol
Copy link
Contributor Author

@astearns I'd want to be there when this is discussed, so if it's on 2020-07-30, it should be during the 2nd half. I don't have conflicts the other days.

@fantasai
Copy link
Collaborator

@Loirooriol Actually, I think you wanted only the first part of 90511c6 reverted, right? So that we skip items that cross any type of flexible track during the regular intrinsic sizing phase, but then consider them in the next phase but still only if they have an intrinsic base size*. Right? Because considering a minmax(10px, 1fr) track during intrinsic sizing doesn't make sense, right? Just doing this much would make your "Problem 2" render as expected, as far as we can tell.

Relatedly, are we doing something sensible when the grid is sized under a min-content/max-content constraint for these cases? I think we fail to do something sensible under min-content constraints... maybe that's a separate issue, though.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Intrinsic contributions for items spanning flex tracks are not web compatible, and agreed to the following:

  • RESOLVED: The auto minimum size is zero for grid items that span multiple tracks when at least one of those tracks is flexible.
  • RESOLVED: When destributing the intrinsic contribution of a grid item, if it spans flexible tracks, excess size is distributed to those tracks and not to fixed/auto sized tracks.
The full IRC log of that discussion <AmeliaBR> Topic: Intrinsic contributions for items spanning flex tracks are not web compatible
<AmeliaBR> github: https://github.com//issues/4783#issuecomment-587749012
<AmeliaBR> oriol: This is a web incompatibility issue.
<AmeliaBR> ... In the last published version of spec, we weren't taking into account intrinsic contributions of items that span multiple tracks, if one of those tracks was flexible.
<fantasai> https://github.com//issues/4783
<AmeliaBR> ... This doesn't doesn't make much sense. So we changed in E.D., but when I tried to implement the change I had to revert because it wasn't web compatible.
<AmeliaBR> ... My proposal is to alter the draft to keep as much of the new behavior as possible, but still be web compatible.
<AmeliaBR> ... But I found 2 different problems. There's a common pattern of 12 columns sized with 1fr. The intention is that all columns equal sized & not overflow. But problem is that 1fr by default has `auto` min size, which takes intrinsic size into count.
<AmeliaBR> ... right now, you don't see the auto min because most items span multi-columns. If we changed that so the intrinsic has an effect, you could make columns uneven or even overflow container.
<AmeliaBR> ... Now, this is partly an author error. You'd get the overflow if you had items that didn't span multiple columns. Could use minmax to make it work, but since 1fr was working, authors didn't do that.
<AmeliaBR> ... So suggestion is that the auto min width of elements is zero if they span multiple tracks, at least one track is flexible.
<AmeliaBR> ... Then in the algorithm, we'd change the auto min width, but the rest would work out based on that.
<AmeliaBR> ... But if someone explicitly said `min-content` for minimum width, that would work correctly.
<fantasai> +1 to Oriol's proposal
<AmeliaBR> ... I think this would handle most web compat issues.
<AmeliaBR> ... There's an example with code & images in the issue.
<AmeliaBR> jensimmons: So, right now this is about making things easier for authors? They *can* do what they want either way, but this is about making the default better?
<AmeliaBR> oriol: Right now in browsers, you can't use min-content or max-content for these items. It always behaves as zero. So the proposal is to support that. It's what the current draft was trying to do, but the change addresses the compat. Going back to always being zero isn't very useful.
<AmeliaBR> fantasai: FWIW, tab & I support the proposal.
<AmeliaBR> oriol: Should I cover the other issue, or resolve on this.
<AmeliaBR> PROPOSAL: The auto minimum size is zero for grid items that span multiple tracks when at least one of those tracks is flexible.
<AmeliaBR> astearns: Are there any concerns about having a switch based on spanning multiple tracks.
<AmeliaBR> fantasai: I think that's better than the alternative.
<AmeliaBR> ... The published draft already has a switch based on spanning multiple tracks. It means we don't consider intrinsic sizes at all. The existing change to the ED got rid of that, but it's not compatible by default.
<AmeliaBR> ... Oriol's proposal is that by default you get the current behavior, but if you specify something explicit that is used instead.
<fremy> fwiw, now that understand the difference between current and proposed, I support this change
<AmeliaBR> ... So yes, we have an inconsistency about whether the intrinsic size is considered, based on spanning multiple tracks. But we're shifting the inconsistency from the grid algorithm to the definition of auto min size.
<AmeliaBR> Rossen: But we're still having an inconsistency in auto min size. The default behavior isn't do the right thing.
<AmeliaBR> fantasai: Yes, but it's do what authors currently expect because of current implementations.
<AmeliaBR> Rossen: Is compat that bad?
<AmeliaBR> fantasai: Yes.
<AmeliaBR> Rossen: OK, so I guess this is least worst solution.
<AmeliaBR> RESOLVED: The auto minimum size is zero for grid items that span multiple tracks when at least one of those tracks is flexible.
<AmeliaBR> oriol: OK, the second problem affected authors who knew about the problem of using auto minimum in a flexible track, so they used an explicit minimum of zero, which then causes a separate problem.
<AmeliaBR> ... If you have 2 columns, one sized with minmax, other with 1fr. One item in auto column only, the other item crosses both.
<AmeliaBR> ... Author expects the flex item to fit the item that's only in that column, but we resolved in 3075 that *all* the flexible tracks, if they don't have an intrinsic minimum, it would be spread across all the tracks that an item spans.
<AmeliaBR> ... Otherwise, a minimum size on a particular item could cause it to overflow.
<AmeliaBR> ... But generally the element spanning both columns is the larger one, so this can result in stretching the column & therefore the other item too much.
<AmeliaBR> ... This could have been fixed with the content keywords, but since setting a minimum of zero was formerly a good way to do this, that's what they used.
<AmeliaBR> ... Proposal is to modify resolution from 3075. If an item spans flexible tracks, when running track-sizing algorithm, the intrinsic contribution will only be distributed among the *flexible* tracks it spans, not the non-flexible ones.
<AmeliaBR> ... That would solve this use case. There wouldn't be too much contribution going to the auto column, all the extra would go to the flexible one.
<AmeliaBR> ... It would still cover the use case we were trying to solve in the previous issue.
<fantasai> s/It/The resolution we just took/
<AmeliaBR> astearns: We never resolved that other issue.
<AmeliaBR> fantasai: But the resolution we just had would address it, with this one modification.
<AmeliaBR> fantasai: I think there are some subtleties to define in what we mean by flexible tracks taking up this extra size. E.g., depends on whether it has a fixed or intrinsic minimum size, and whether we're sizing the grid under a min size constraint.
<AmeliaBR> oriol: Should we do this at all if sizing under a min/max content constraint?
<AmeliaBR> fantasai: In that situation, we always consider the contents. That's expected. But I think we still have some errors there were we consider the min or the max but not both. So we do need to think about those details.
<fantasai> https://drafts.csswg.org/css-grid-1/#algo-flex-tracks
<fantasai> s/the min or the max but not both/the max, but not the min/
<AmeliaBR> fremy: My impression of this topic is that we had something widely implemented that we didn't think was well designed, but now we're making piecemeal changes to try to fix compatibility. But I'm afraid we're going to get inconsistent implementation where some browsers make some of these changes but not others. Reminds me of CSS tables, in a bad way. We're going to need careful tests to make sure we have consistency.
<AmeliaBR> TabAtkins: Elika & I have been adding tests as we go, so we should catch that quickly.
<fantasai> s/as we go/against the changes section/
<AmeliaBR> fantasai: And we can add tests for these changes in next couple weeks.
<AmeliaBR> astearns: So, you have no concerns about these particular changes, just about following up with tests?
<AmeliaBR> fremy: Yes.
<AmeliaBR> astearns: fantasai, the other issue you raised about the algorithm not handling min/ max content, that's a separate issue?
<AmeliaBR> fantasai: yes, we can still go ahead with this.
<AmeliaBR> oriol: PROPOSAL: When destributing the intrinsic contribution of a grid item, if it spans flexible tracks, excess size is distributed to those items and not to fixed/auto sized tracks.
<AmeliaBR> Rossen_: {Gives an example}
<AmeliaBR> oriol: No. If there are fixed size tracks, that size is subtracted before distributing extra to the flexible tracks.
<AmeliaBR> Rossen_: OK, then I agree. Need to make that clear.
<AmeliaBR> RESOLVED: When destributing the intrinsic contribution of a grid item, if it spans flexible tracks, excess size is distributed to those tracks and not to fixed/auto sized tracks.
<AmeliaBR> astearns: ... and add tests

tabatkins added a commit that referenced this issue Jul 28, 2020
…bution of a spanning item if it crosses a flexible track. #4783.
fantasai added a commit that referenced this issue Jul 28, 2020
…ks do not contribute to inflexible intrinsically-sized tracks. #4783"

This reverts commit 90511c6.
fantasai added a commit to fantasai/wpt that referenced this issue Jul 31, 2020
fantasai added a commit to fantasai/wpt that referenced this issue Jul 31, 2020
@fantasai
Copy link
Collaborator

Closing out r=Oriol via IRC

@fantasai fantasai added the Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. label Jul 31, 2020
fantasai added a commit to web-platform-tests/wpt that referenced this issue Jul 31, 2020
fantasai added a commit to fantasai/wpt that referenced this issue Jul 31, 2020
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 8, 2020
…swg-drafts#4783 and also fix some errors., a=testonly

Automatic update from web-platform-tests
[css-grid-1] Update test to match w3c/csswg-drafts#4783 and also fix some errors. (#24822)

--

wpt-commits: 107694f0fe4e0b0dd429b26e022b3602656f713a
wpt-pr: 24822
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 15, 2020
…swg-drafts#4783 and also fix some errors., a=testonly

Automatic update from web-platform-tests
[css-grid-1] Update test to match w3c/csswg-drafts#4783 and also fix some errors. (#24822)

--

wpt-commits: 107694f0fe4e0b0dd429b26e022b3602656f713a
wpt-pr: 24822
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Aug 16, 2020
…swg-drafts#4783 and also fix some errors., a=testonly

Automatic update from web-platform-tests
[css-grid-1] Update test to match w3c/csswg-drafts#4783 and also fix some errors. (#24822)

--

wpt-commits: 107694f0fe4e0b0dd429b26e022b3602656f713a
wpt-pr: 24822

UltraBlame original commit: c698da2dea06be2fa0ecb83a5e0e6cfd971fb7ea
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Aug 16, 2020
…swg-drafts#4783 and also fix some errors., a=testonly

Automatic update from web-platform-tests
[css-grid-1] Update test to match w3c/csswg-drafts#4783 and also fix some errors. (#24822)

--

wpt-commits: 107694f0fe4e0b0dd429b26e022b3602656f713a
wpt-pr: 24822

UltraBlame original commit: b5c5c9f7060f9f8e3daeb0a2d2b504fa4d9e82f6
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Aug 16, 2020
…swg-drafts#4783 and also fix some errors., a=testonly

Automatic update from web-platform-tests
[css-grid-1] Update test to match w3c/csswg-drafts#4783 and also fix some errors. (#24822)

--

wpt-commits: 107694f0fe4e0b0dd429b26e022b3602656f713a
wpt-pr: 24822

UltraBlame original commit: c698da2dea06be2fa0ecb83a5e0e6cfd971fb7ea
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Aug 16, 2020
…swg-drafts#4783 and also fix some errors., a=testonly

Automatic update from web-platform-tests
[css-grid-1] Update test to match w3c/csswg-drafts#4783 and also fix some errors. (#24822)

--

wpt-commits: 107694f0fe4e0b0dd429b26e022b3602656f713a
wpt-pr: 24822

UltraBlame original commit: b5c5c9f7060f9f8e3daeb0a2d2b504fa4d9e82f6
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Aug 16, 2020
…swg-drafts#4783 and also fix some errors., a=testonly

Automatic update from web-platform-tests
[css-grid-1] Update test to match w3c/csswg-drafts#4783 and also fix some errors. (#24822)

--

wpt-commits: 107694f0fe4e0b0dd429b26e022b3602656f713a
wpt-pr: 24822

UltraBlame original commit: c698da2dea06be2fa0ecb83a5e0e6cfd971fb7ea
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Aug 16, 2020
…swg-drafts#4783 and also fix some errors., a=testonly

Automatic update from web-platform-tests
[css-grid-1] Update test to match w3c/csswg-drafts#4783 and also fix some errors. (#24822)

--

wpt-commits: 107694f0fe4e0b0dd429b26e022b3602656f713a
wpt-pr: 24822

UltraBlame original commit: b5c5c9f7060f9f8e3daeb0a2d2b504fa4d9e82f6
sidvishnoi pushed a commit to sidvishnoi/gecko-webmonetization that referenced this issue Sep 23, 2020
…swg-drafts#4783 and also fix some errors., a=testonly

Automatic update from web-platform-tests
[css-grid-1] Update test to match w3c/csswg-drafts#4783 and also fix some errors. (#24822)

--

wpt-commits: 107694f0fe4e0b0dd429b26e022b3602656f713a
wpt-pr: 24822
ambroff pushed a commit to ambroff/gecko that referenced this issue Nov 4, 2020
…swg-drafts#4783 and also fix some errors., a=testonly

Automatic update from web-platform-tests
[css-grid-1] Update test to match w3c/csswg-drafts#4783 and also fix some errors. (#24822)

--

wpt-commits: 107694f0fe4e0b0dd429b26e022b3602656f713a
wpt-pr: 24822
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 29, 2021
This test was checking that grid items not spanning any flex track with
an intrinsic min track sizing function would distribute its intrinsic
contribution in $11.5.3. That was according to the resolution from
w3c/csswg-drafts#3705

However, that wasn't web compatible, so it was later amended in
w3c/csswg-drafts#4783

Now, the intrinsic contributions of items spanning flex tracks must be
distributed in $11.5.4, even if none of the flex tracks is intrinsic.

This change makes the test pass in GridNG.

Bug: 1045599
Change-Id: Ibfb818dcc12a185e30d2824e9ca534069beb48ec
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 29, 2021
This test was checking that grid items not spanning any flex track with
an intrinsic min track sizing function would distribute its intrinsic
contribution in $11.5.3. That was according to the resolution from
w3c/csswg-drafts#3705

However, that wasn't web compatible, so it was later amended in
w3c/csswg-drafts#4783

Now, the intrinsic contributions of items spanning flex tracks must be
distributed in $11.5.4, even if none of the flex tracks is intrinsic.

This change makes the test pass in GridNG.

Bug: 1045599
Change-Id: Ibfb818dcc12a185e30d2824e9ca534069beb48ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2791426
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Commit-Queue: Oriol Brufau <obrufau@igalia.com>
Cr-Commit-Position: refs/heads/master@{#867283}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 29, 2021
This test was checking that grid items not spanning any flex track with
an intrinsic min track sizing function would distribute its intrinsic
contribution in $11.5.3. That was according to the resolution from
w3c/csswg-drafts#3705

However, that wasn't web compatible, so it was later amended in
w3c/csswg-drafts#4783

Now, the intrinsic contributions of items spanning flex tracks must be
distributed in $11.5.4, even if none of the flex tracks is intrinsic.

This change makes the test pass in GridNG.

Bug: 1045599
Change-Id: Ibfb818dcc12a185e30d2824e9ca534069beb48ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2791426
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Commit-Queue: Oriol Brufau <obrufau@igalia.com>
Cr-Commit-Position: refs/heads/master@{#867283}
pull bot pushed a commit to FreddyZeng/chromium that referenced this issue Mar 30, 2021
This test was checking that grid items not spanning any flex track with
an intrinsic min track sizing function would distribute its intrinsic
contribution in $11.5.3. That was according to the resolution from
w3c/csswg-drafts#3705

However, that wasn't web compatible, so it was later amended in
w3c/csswg-drafts#4783

Now, the intrinsic contributions of items spanning flex tracks must be
distributed in $11.5.4, even if none of the flex tracks is intrinsic.

This change makes the test pass in GridNG.

Bug: 1045599
Change-Id: Ibfb818dcc12a185e30d2824e9ca534069beb48ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2791426
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Commit-Queue: Oriol Brufau <obrufau@igalia.com>
Cr-Commit-Position: refs/heads/master@{#867283}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 2, 2021
…nsic-sizes-002.html, a=testonly

Automatic update from web-platform-tests
[css-grid] Fix WPT grid-flex-track-intrinsic-sizes-002.html

This test was checking that grid items not spanning any flex track with
an intrinsic min track sizing function would distribute its intrinsic
contribution in $11.5.3. That was according to the resolution from
w3c/csswg-drafts#3705

However, that wasn't web compatible, so it was later amended in
w3c/csswg-drafts#4783

Now, the intrinsic contributions of items spanning flex tracks must be
distributed in $11.5.4, even if none of the flex tracks is intrinsic.

This change makes the test pass in GridNG.

Bug: 1045599
Change-Id: Ibfb818dcc12a185e30d2824e9ca534069beb48ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2791426
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Commit-Queue: Oriol Brufau <obrufau@igalia.com>
Cr-Commit-Position: refs/heads/master@{#867283}

--

wpt-commits: 4fb0b5be59092eecdb9ef0cd03180f2f9a423f1d
wpt-pr: 28291
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 2, 2021
…nsic-sizes-002.html, a=testonly

Automatic update from web-platform-tests
[css-grid] Fix WPT grid-flex-track-intrinsic-sizes-002.html

This test was checking that grid items not spanning any flex track with
an intrinsic min track sizing function would distribute its intrinsic
contribution in $11.5.3. That was according to the resolution from
w3c/csswg-drafts#3705

However, that wasn't web compatible, so it was later amended in
w3c/csswg-drafts#4783

Now, the intrinsic contributions of items spanning flex tracks must be
distributed in $11.5.4, even if none of the flex tracks is intrinsic.

This change makes the test pass in GridNG.

Bug: 1045599
Change-Id: Ibfb818dcc12a185e30d2824e9ca534069beb48ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2791426
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Commit-Queue: Oriol Brufau <obrufau@igalia.com>
Cr-Commit-Position: refs/heads/master@{#867283}

--

wpt-commits: 4fb0b5be59092eecdb9ef0cd03180f2f9a423f1d
wpt-pr: 28291
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 7, 2021
…nsic-sizes-002.html, a=testonly

Automatic update from web-platform-tests
[css-grid] Fix WPT grid-flex-track-intrinsic-sizes-002.html

This test was checking that grid items not spanning any flex track with
an intrinsic min track sizing function would distribute its intrinsic
contribution in $11.5.3. That was according to the resolution from
w3c/csswg-drafts#3705

However, that wasn't web compatible, so it was later amended in
w3c/csswg-drafts#4783

Now, the intrinsic contributions of items spanning flex tracks must be
distributed in $11.5.4, even if none of the flex tracks is intrinsic.

This change makes the test pass in GridNG.

Bug: 1045599
Change-Id: Ibfb818dcc12a185e30d2824e9ca534069beb48ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2791426
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Commit-Queue: Oriol Brufau <obrufau@igalia.com>
Cr-Commit-Position: refs/heads/master@{#867283}

--

wpt-commits: 4fb0b5be59092eecdb9ef0cd03180f2f9a423f1d
wpt-pr: 28291
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 8, 2021
…nsic-sizes-002.html, a=testonly

Automatic update from web-platform-tests
[css-grid] Fix WPT grid-flex-track-intrinsic-sizes-002.html

This test was checking that grid items not spanning any flex track with
an intrinsic min track sizing function would distribute its intrinsic
contribution in $11.5.3. That was according to the resolution from
w3c/csswg-drafts#3705

However, that wasn't web compatible, so it was later amended in
w3c/csswg-drafts#4783

Now, the intrinsic contributions of items spanning flex tracks must be
distributed in $11.5.4, even if none of the flex tracks is intrinsic.

This change makes the test pass in GridNG.

Bug: 1045599
Change-Id: Ibfb818dcc12a185e30d2824e9ca534069beb48ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2791426
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Commit-Queue: Oriol Brufau <obrufau@igalia.com>
Cr-Commit-Position: refs/heads/master@{#867283}

--

wpt-commits: 4fb0b5be59092eecdb9ef0cd03180f2f9a423f1d
wpt-pr: 28291
webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Oct 2, 2021
https://bugs.webkit.org/show_bug.cgi?id=230252

Reviewed by Sergio Villar Senin.

LayoutTests/imported/w3c:

Three tests are passing and the test expectation files are updated.

* web-platform-tests/css/css-grid/layout-algorithm/grid-flex-track-intrinsic-sizes-001-expected.txt:
* web-platform-tests/css/css-grid/layout-algorithm/grid-flex-track-intrinsic-sizes-002-expected.txt:
* web-platform-tests/css/css-grid/layout-algorithm/grid-flex-track-intrinsic-sizes-003-expected.txt:

Source/WebCore:

CSS spec has added one more step at https://drafts.csswg.org/css-grid/#algo-spanning-flex-items
for resolving intrinsic track sizes. Basically, increasing sizes to accommodate spanning items
crossing flexible tracks instead of ignoring their contents completely.

This patch is an import of obrufau's work in chromium. Initial CL is at
https://chromium-review.googlesource.com/c/chromium/src/+/1492337/16..20.
The performance issue caused by the patch were fixed at https://bugs.webkit.org/show_bug.cgi?id=207852
for webkit and https://bugs.chromium.org/p/chromium/issues/detail?id=1021929 for chromium.
This change also addresses web compat issue raised at w3c/csswg-drafts#4783.

* rendering/GridTrackSizingAlgorithm.cpp:
(WebCore::GridTrackSizingAlgorithm::initialGrowthLimit const):
(WebCore::GridTrackSizingAlgorithm::spanningItemCrossesFlexibleSizedTracks const):
(WebCore::GridTrackSizingAlgorithm::increaseSizesToAccommodateSpanningItems):
(WebCore::getSizeDistributionWeight):
(WebCore::distributeItemIncurredIncreaseToTrack):
(WebCore::distributeItemIncurredIncreases):
(WebCore::GridTrackSizingAlgorithm::distributeSpaceToTracks const):
(WebCore::GridTrackSizingAlgorithmStrategy::minSizeForChild const):
(WebCore::GridTrackSizingAlgorithm::resolveIntrinsicTrackSizes):
* rendering/GridTrackSizingAlgorithm.h:
(WebCore::GridTrackSizingAlgorithmStrategy::distributeSpaceToTracks const):


Canonical link: https://commits.webkit.org/242426@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@283439 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Cwiiis pushed a commit to Cwiiis/webkit-deprecated that referenced this issue Oct 4, 2021
https://bugs.webkit.org/show_bug.cgi?id=230252

Reviewed by Sergio Villar Senin.

LayoutTests/imported/w3c:

Three tests are passing and the test expectation files are updated.

* web-platform-tests/css/css-grid/layout-algorithm/grid-flex-track-intrinsic-sizes-001-expected.txt:
* web-platform-tests/css/css-grid/layout-algorithm/grid-flex-track-intrinsic-sizes-002-expected.txt:
* web-platform-tests/css/css-grid/layout-algorithm/grid-flex-track-intrinsic-sizes-003-expected.txt:

Source/WebCore:

CSS spec has added one more step at https://drafts.csswg.org/css-grid/#algo-spanning-flex-items
for resolving intrinsic track sizes. Basically, increasing sizes to accommodate spanning items
crossing flexible tracks instead of ignoring their contents completely.

This patch is an import of obrufau's work in chromium. Initial CL is at
https://chromium-review.googlesource.com/c/chromium/src/+/1492337/16..20.
The performance issue caused by the patch were fixed at https://bugs.webkit.org/show_bug.cgi?id=207852
for webkit and https://bugs.chromium.org/p/chromium/issues/detail?id=1021929 for chromium.
This change also addresses web compat issue raised at w3c/csswg-drafts#4783.

* rendering/GridTrackSizingAlgorithm.cpp:
(WebCore::GridTrackSizingAlgorithm::initialGrowthLimit const):
(WebCore::GridTrackSizingAlgorithm::spanningItemCrossesFlexibleSizedTracks const):
(WebCore::GridTrackSizingAlgorithm::increaseSizesToAccommodateSpanningItems):
(WebCore::getSizeDistributionWeight):
(WebCore::distributeItemIncurredIncreaseToTrack):
(WebCore::distributeItemIncurredIncreases):
(WebCore::GridTrackSizingAlgorithm::distributeSpaceToTracks const):
(WebCore::GridTrackSizingAlgorithmStrategy::minSizeForChild const):
(WebCore::GridTrackSizingAlgorithm::resolveIntrinsicTrackSizes):
* rendering/GridTrackSizingAlgorithm.h:
(WebCore::GridTrackSizingAlgorithmStrategy::distributeSpaceToTracks const):


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@283439 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@Loirooriol
Copy link
Contributor Author

This has been implemented as part of GridNG, shipped in Chromium 93, with only one complaint, so it seems web compatible now. Recently landed in WebKit too https://webkit.org/b/230252

In WPT this is covered by these tests:

@Loirooriol Loirooriol added Tested Memory aid - issue has WPT tests and removed Needs Testcase (WPT) labels Oct 7, 2021
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
This test was checking that grid items not spanning any flex track with
an intrinsic min track sizing function would distribute its intrinsic
contribution in $11.5.3. That was according to the resolution from
w3c/csswg-drafts#3705

However, that wasn't web compatible, so it was later amended in
w3c/csswg-drafts#4783

Now, the intrinsic contributions of items spanning flex tracks must be
distributed in $11.5.4, even if none of the flex tracks is intrinsic.

This change makes the test pass in GridNG.

Bug: 1045599
Change-Id: Ibfb818dcc12a185e30d2824e9ca534069beb48ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2791426
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Commit-Queue: Oriol Brufau <obrufau@igalia.com>
Cr-Commit-Position: refs/heads/master@{#867283}
GitOrigin-RevId: 762ea1b8feb239644f4c1073af42fc639959a6f6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed Accepted by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-grid-1 Tested Memory aid - issue has WPT tests
Projects
None yet
Development

No branches or pull requests

5 participants