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] Growth limits not increased with min-content contributions of spanning items #4790

Closed
Loirooriol opened this issue Feb 18, 2020 · 6 comments

Comments

@Loirooriol
Copy link
Contributor

Loirooriol commented Feb 18, 2020

If you have a non-spanning item, https://drafts.csswg.org/css-grid/#algo-single-span-items

  • For min-content maximums:
    If the track has a min-content max track sizing function, set its growth limit to the maximum of the items’ min-content contributions.

But for spanning items, https://drafts.csswg.org/css-grid/#algo-spanning-items

  • For intrinsic maximums: Next increase the growth limit of tracks with an intrinsic max track sizing function by distributing extra space as needed to account for these items' minimum contributions. [...]

Why do we distribute the min-content contribution in the former case, but only the minimum contribution in the latter?

.grid { display: grid }
.grid.one { grid-template-columns: minmax(0, min-content) }
.grid.two { grid-template-columns: repeat(2, minmax(0, min-content)) }
.grid > .item { min-width: 0; background: cyan }
.grid.two > .item { grid-column: span 2 }
<div class="grid one">
  <div class="item">Foo</div>
</div>
<div class="grid two">
  <div class="item">Foo</div>
</div>

looks like

Thus violating

Note: This step is a simplification of the steps below for handling spanning items, and should yield the same behavior as running those instructions on items with a span of 1.

@Loirooriol
Copy link
Contributor Author

Note that intrinsic max track sizing functions can only be min-content or max-content (auto is treated as max-content). So we should just distribute the min-content contribution instead of the minimum contribution.

@fantasai
Copy link
Collaborator

This was an error introduced in f48098c and it needs to be reverted.

@Loirooriol
Copy link
Contributor Author

Even if this was an unintentional error, it has been implemented by Chromium, WebKit, Firefox, and old Edge.
So I hope reverting to the sane behavior will still be web compatible.

@fantasai
Copy link
Collaborator

fantasai commented Jul 27, 2020

@Loirooriol You're right. Here's a live testcase. The specified behavior is absurd, though. I'm surprised no one else noticed it's wrong. :(

Agenda+ to confirm the correction.

@css-meeting-bot
Copy link
Member

css-meeting-bot commented Jul 27, 2020

The CSS Working Group just discussed Growth limits not increased with min-content contributions of spanning items.

RESOLVED: Accept the edits made to the ED re minimum vs min-content (and add WPT tests)

The full IRC log of that discussion <AmeliaBR> Topic: Growth limits not increased with min-content contributions of spanning items
<AmeliaBR> github: https://github.com//issues/4790#issuecomment-664068575
<fantasai> https://drafts.csswg.org/css-grid-1/#algo-content
<AmeliaBR> fantasai: This will make more sense if I pull up the relevant section of the spec. ↑
<AmeliaBR> ... There are several sections in how we handle intrinsic sizes. Minimums first, then maximums. For minimums, there's auto and minimum/maximum. For maximums, there's no auto.
<AmeliaBR> ... When we edited the spec recently to add new terminology, we accidentally said “minimum” when we meant “min-content”. But when we went to fix that, discovered someone had implemented as written, so need to confirm the reversion.
<AmeliaBR> ... The current text is rather non-sensical. It's very rare/weird that min-content would be specified as the maximum value of your minmax range. We should fix it.
<AmeliaBR> astearns: Anyone have actual web compat information measured? Or should we change & see if we get negative feedback.
<AmeliaBR> oriol: I do know there is no WPT for the bad behavior, although there is a Chromium internal test.
<fremy> sounds unexpected to me
<AmeliaBR> fantasai: I can't see any reason why anyone would write this code, with min-content in the max side of a minmax function.
<AmeliaBR> fremy: I don't expect people would do that. We should be able to fix this.
<AmeliaBR> PROPOSAL: Accept the edits made to the ED re minimum vs min-content (and add WPT tests)

@Loirooriol
Copy link
Contributor Author

This seems covered by WPT css-grid/layout-algorithm/grid-intrinsic-track-sizes-001.html

pull bot pushed a commit to Yannic/chromium that referenced this issue Sep 2, 2020
By mistake the specification used to say that, for items spanning
multiple tracks, the growth limits of the tracks with an intrinsic max
track sizing function should grow to accommodate the minimum
contribution of the item.

But this was a mistake, because an intrinsic max track sizing function
can only be min-content or max-content. So instead of distributing the
minimum contribution, it should be the min-content contribution.

The spec has been fixed and there is a CSSWG resolution in
w3c/csswg-drafts#4790

This patch fixes the problem by reverting r424527. The change is likely
web compatible, since it only affects a rare edge case with 'minmax()'
where the min sizing function is 'auto' or a fixed value smaller than
the min-content contribution, the max sizing function is 'min-content',
and an item whose minimum contribution is forced to be different than
the min-content contribution, and spans multiple tracks.

Bug: 1122084

TEST=external/wpt/css/css-grid/layout-algorithm/grid-intrinsic-track-sizes-001.html
TEST=fast/css-grid-layout/grid-intrinsic-maximums.html

Change-Id: I1efd6e48b55fc71f37f8303c731bfbf601ca4c70
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2377499
Reviewed-by: Manuel Rego <rego@igalia.com>
Commit-Queue: Oriol Brufau <obrufau@igalia.com>
Cr-Commit-Position: refs/heads/master@{#803871}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 11, 2020
…resolution. r=mats

By mistake the specification used to say that, for items spanning
multiple tracks, the growth limits of the tracks with an intrinsic max
track sizing function should grow to accommodate the minimum
contribution of the item.

But this was a mistake, because an intrinsic max track sizing function
can only be min-content or max-content. So instead of distributing the
minimum contribution, it should be the min-content contribution.

The spec has been fixed and there is a CSSWG resolution in
w3c/csswg-drafts#4790

This patch fixes the problem by reverting 2b923d48ea7e. The change is
likely web compatible, since it only affects a rare edge case with
'minmax()' where the min sizing function is 'auto' or a fixed value
smaller than the min-content contribution, the max sizing function is
'min-content', and an item whose minimum contribution is forced to be
smaller than the min-content contribution, and spans multiple tracks.

Differential Revision: https://phabricator.services.mozilla.com/D89145
seinlin pushed a commit to kaiostech/gecko-b2g that referenced this issue Sep 14, 2020
…resolution. r=mats

By mistake the specification used to say that, for items spanning
multiple tracks, the growth limits of the tracks with an intrinsic max
track sizing function should grow to accommodate the minimum
contribution of the item.

But this was a mistake, because an intrinsic max track sizing function
can only be min-content or max-content. So instead of distributing the
minimum contribution, it should be the min-content contribution.

The spec has been fixed and there is a CSSWG resolution in
w3c/csswg-drafts#4790

This patch fixes the problem by reverting 2b923d48ea7e. The change is
likely web compatible, since it only affects a rare edge case with
'minmax()' where the min sizing function is 'auto' or a fixed value
smaller than the min-content contribution, the max sizing function is
'min-content', and an item whose minimum contribution is forced to be
smaller than the min-content contribution, and spans multiple tracks.

Differential Revision: https://phabricator.services.mozilla.com/D89145
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Sep 16, 2020
…resolution. r=mats

By mistake the specification used to say that, for items spanning
multiple tracks, the growth limits of the tracks with an intrinsic max
track sizing function should grow to accommodate the minimum
contribution of the item.

But this was a mistake, because an intrinsic max track sizing function
can only be min-content or max-content. So instead of distributing the
minimum contribution, it should be the min-content contribution.

The spec has been fixed and there is a CSSWG resolution in
w3c/csswg-drafts#4790

This patch fixes the problem by reverting 2b923d48ea7e. The change is
likely web compatible, since it only affects a rare edge case with
'minmax()' where the min sizing function is 'auto' or a fixed value
smaller than the min-content contribution, the max sizing function is
'min-content', and an item whose minimum contribution is forced to be
smaller than the min-content contribution, and spans multiple tracks.

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

UltraBlame original commit: c8322fb7a54aed137bb6db2f5a62087113dd721c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Sep 16, 2020
…resolution. r=mats

By mistake the specification used to say that, for items spanning
multiple tracks, the growth limits of the tracks with an intrinsic max
track sizing function should grow to accommodate the minimum
contribution of the item.

But this was a mistake, because an intrinsic max track sizing function
can only be min-content or max-content. So instead of distributing the
minimum contribution, it should be the min-content contribution.

The spec has been fixed and there is a CSSWG resolution in
w3c/csswg-drafts#4790

This patch fixes the problem by reverting 2b923d48ea7e. The change is
likely web compatible, since it only affects a rare edge case with
'minmax()' where the min sizing function is 'auto' or a fixed value
smaller than the min-content contribution, the max sizing function is
'min-content', and an item whose minimum contribution is forced to be
smaller than the min-content contribution, and spans multiple tracks.

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

UltraBlame original commit: c8322fb7a54aed137bb6db2f5a62087113dd721c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Sep 16, 2020
…resolution. r=mats

By mistake the specification used to say that, for items spanning
multiple tracks, the growth limits of the tracks with an intrinsic max
track sizing function should grow to accommodate the minimum
contribution of the item.

But this was a mistake, because an intrinsic max track sizing function
can only be min-content or max-content. So instead of distributing the
minimum contribution, it should be the min-content contribution.

The spec has been fixed and there is a CSSWG resolution in
w3c/csswg-drafts#4790

This patch fixes the problem by reverting 2b923d48ea7e. The change is
likely web compatible, since it only affects a rare edge case with
'minmax()' where the min sizing function is 'auto' or a fixed value
smaller than the min-content contribution, the max sizing function is
'min-content', and an item whose minimum contribution is forced to be
smaller than the min-content contribution, and spans multiple tracks.

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

UltraBlame original commit: c8322fb7a54aed137bb6db2f5a62087113dd721c
ambroff pushed a commit to ambroff/gecko that referenced this issue Nov 4, 2020
…resolution. r=mats

By mistake the specification used to say that, for items spanning
multiple tracks, the growth limits of the tracks with an intrinsic max
track sizing function should grow to accommodate the minimum
contribution of the item.

But this was a mistake, because an intrinsic max track sizing function
can only be min-content or max-content. So instead of distributing the
minimum contribution, it should be the min-content contribution.

The spec has been fixed and there is a CSSWG resolution in
w3c/csswg-drafts#4790

This patch fixes the problem by reverting 2b923d48ea7e. The change is
likely web compatible, since it only affects a rare edge case with
'minmax()' where the min sizing function is 'auto' or a fixed value
smaller than the min-content contribution, the max sizing function is
'min-content', and an item whose minimum contribution is forced to be
smaller than the min-content contribution, and spans multiple tracks.

Differential Revision: https://phabricator.services.mozilla.com/D89145
ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
https://bugs.webkit.org/show_bug.cgi?id=216142

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Import WPT test.

* web-platform-tests/css/css-grid/layout-algorithm/grid-intrinsic-track-sizes-001-expected.txt: Added.
* web-platform-tests/css/css-grid/layout-algorithm/grid-intrinsic-track-sizes-001.html: Added.
* web-platform-tests/css/css-grid/layout-algorithm/w3c-import.log:

Source/WebCore:

By mistake the specification used to say that, for items spanning
multiple tracks, the growth limits of the tracks with an intrinsic max
track sizing function should grow to accommodate the minimum
contribution of the item.

But this was a mistake, because an intrinsic max track sizing function
can only be min-content or max-content. So instead of distributing the
minimum contribution, it should be the min-content contribution.

The spec has been fixed and there is a CSSWG resolution in
w3c/csswg-drafts#4790

This patch fixes the problem by reverting r207290. The change is likely
web compatible, since it only affects a rare edge case with 'minmax()'
where the min sizing function is 'auto' or a fixed value smaller than
the min-content contribution, the max sizing function is 'min-content',
and an item whose minimum contribution is forced to be different than
the min-content contribution, and spans multiple tracks.

This is a port of https://crrev.com/803871 from Chromium.

Tests: fast/css-grid-layout/grid-intrinsic-maximums.html
       imported/w3c/web-platform-tests/css/css-grid/layout-algorithm/grid-intrinsic-track-sizes-001.html

* rendering/GridTrackSizingAlgorithm.cpp:
(WebCore::GridTrackSizingAlgorithm::itemSizeForTrackSizeComputationPhase const):

LayoutTests:

Update test expectations. Some are wrong due to bug 216144.

* fast/css-grid-layout/grid-intrinsic-maximums-expected.html:


Canonical link: https://commits.webkit.org/229044@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@266675 268f45cc-cd09-0410-ab3c-d52691b4dbfc
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
By mistake the specification used to say that, for items spanning
multiple tracks, the growth limits of the tracks with an intrinsic max
track sizing function should grow to accommodate the minimum
contribution of the item.

But this was a mistake, because an intrinsic max track sizing function
can only be min-content or max-content. So instead of distributing the
minimum contribution, it should be the min-content contribution.

The spec has been fixed and there is a CSSWG resolution in
w3c/csswg-drafts#4790

This patch fixes the problem by reverting r424527. The change is likely
web compatible, since it only affects a rare edge case with 'minmax()'
where the min sizing function is 'auto' or a fixed value smaller than
the min-content contribution, the max sizing function is 'min-content',
and an item whose minimum contribution is forced to be different than
the min-content contribution, and spans multiple tracks.

Bug: 1122084

TEST=external/wpt/css/css-grid/layout-algorithm/grid-intrinsic-track-sizes-001.html
TEST=fast/css-grid-layout/grid-intrinsic-maximums.html

Change-Id: I1efd6e48b55fc71f37f8303c731bfbf601ca4c70
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2377499
Reviewed-by: Manuel Rego <rego@igalia.com>
Commit-Queue: Oriol Brufau <obrufau@igalia.com>
Cr-Commit-Position: refs/heads/master@{#803871}
GitOrigin-RevId: 1d01cbaf3860d44dc1128dc0337258f63024c4d4
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

4 participants