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] Overflow with auto-repeat and minmax() #4043

Closed
mrego opened this issue Jun 19, 2019 · 11 comments
Closed

[css-grid] Overflow with auto-repeat and minmax() #4043

mrego opened this issue Jun 19, 2019 · 11 comments

Comments

@mrego
Copy link
Member

Imagine the following example:

<div style="display: grid; width: 200px; border: solid thick;
            grid-template-columns: repeat(auto-fill, minmax(200px, 100px));
            grid-auto-rows: 100px;">
  <div style="background: magenta;"></div>
  <div style="background: cyan;"></div>
  <div style="background: yellow;"></div>
</div>

All implementations (Chromium, Firefox, WebKit and Edge) render it the same way. The use the 2nd argument of minmax(200px, 100px) to calculate the number of repetitions,
so they consider there are 2 columns. Then we have overflow because columns are 200px width.

Screenshot of example rendering with 2 columns and overflow

I guess the reason for that is the following text in the spec:

if the grid container has a definite size or max size in the relevant axis, then the number of repetitions is the largest possible positive integer that does not cause the grid to overflow the content box of its grid container (treating each track as its max track sizing function if that is definite or as its minimum track sizing function otherwise, and taking gap into account)

The max track sizing function is:

If the track was sized with a minmax() function, this is the second argument to that function

It's true that minmax() definition already talks about the case were min is bigger than max:

If the max is less than the min, then the max will be floored by the min (essentially yielding minmax(min, min))

But it seems that none of the implementations catch that detail.

It seems we need to use the min in this case, so the grid of the example has only 1 column, unless there are some other weird implications from using the min in this case. So I guess that the spec text should be updated to be clearer about that and avoid similar misunderstandings in the future.

@MatsPalmgren
Copy link

I think key word in "If the max is less than the min, then the max will be floored by the min" is "will", meaning that some other spec text will say how/when that's applied. Indeed, this is what falls out from the "if the growth limit is less than the base size, increase the growth limit to match the base size" which is applied at various stages through the Track Sizing Algorithm. The auto-repeat spec text in your first quote discuss min/max track sizing functions separately without mentioning any flooring though, so I think current UAs are correct in not applying it in this case. However, I agree that it's a reasonable change to add an explicit flooring step there. I'd prefer if the spec continues to make that explicit in each case though, so that it's clear when in the algorithms it should be applied (we've agreed on that in a previous spec issue, IIRC). Otherwise, I fear that it's open to interpretation when to apply it and that might lead to different rendering results.

@Loirooriol
Copy link
Contributor

Loirooriol commented Jun 20, 2019

I think key word is "will", meaning that some other spec text will say how/when that's applied.

That's right, see #3567. The min and max cannot always be compared a priori, so each time we handle max<min needs to be explicitly defined.

I agree the proposal makes sense, i.e.

  • If both the min and max track sizing functions are definite, use the maximum of them
  • If only one track sizing function is definite, use that one

And I think there is the remaining case of both sizing functions being indefinite, like a grid container with width: max-content; max-width: 100px; grid-template-columns: repeat(auto-fill, 50%). This falls into this case because the max size is definite, but the preferred size is intrinsic, so the percentage track size is initially treated as auto, right?

@mrego
Copy link
Member Author

And I think there is the remaining case of both sizing functions being indefinite

The first sentence of the spec says:

if the grid container has a definite size or max size in the relevant axis

So if the grid container size is indefinite, then we don't calculate the number of repetitions, even if they have a fixed size like repeat(auto-fill, 100px).

@Loirooriol
Copy link
Contributor

The first sentence of the spec says:

if the grid container has a definite size or max size in the relevant axis

But max-width: 100px is definite so the condition holds, even if width is indefinite, doesn't it?

@fantasai
Copy link
Collaborator

OK, updated the spec to floor the max by the min. Agenda+ to confirm.

Split out the issue of resolving percentages in the indefinite-width, fixed-max-width case as #4480

@heycam
Copy link
Contributor

@MatsPalmgren does that text look good to you?

@MatsPalmgren
Copy link

Yup, the new text looks clear to me.

@cdoublev
Copy link
Collaborator

cdoublev commented Nov 15, 2019

I have tried to read all discussions here (BugZilla), here (Chromium), and in this issue, but please forgive me if I missed the answer to the following question: does this update will also resolve the following case, or should I fill a new ticket? This is a minimal reproduction of the CodePen used by Rachel Andrew to explain minmax() on its personal website. I think it's also used in the ticket from BugZilla.

What is the expected behavior?

Each grid item should have a minimum width of 200px.

What went wrong?

When resizing the window, the min value in minmax() might be ignored for "B" and/or "C", ie. grid items placed before an "overflowing" area (grid cell-s-) generated by a spanning grid item.

@Loirooriol
Copy link
Contributor

@cdoublev When the window is small enough, repeat(auto-fill, minmax(200px, 1fr)) will only produce a single explicit column, otherwise there would be overflow. Then, item D has a span of 3, so 2 implicit columns are added at the end. These implicit columns are not sized with minmax(200px, 1fr), they obey the grid-auto-column property instead, which defaults to auto. To achieve your desired behavior, use grid-auto-columns: minmax(200px, 1fr).

@cdoublev
Copy link
Collaborator

Thank you for this clarification. I would have thought that it makes more sense to create a new row and preserve the min value in minmax(), but I guess it would make an exception to the placement behavior.

Anyway, I'm not trying to achieve a particular behavior. I was just revisiting auto-fit/auto-fill behaviors, by reading Rachel's blog post, and I was "surprised" by the min value not being respected.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Overflow with auto-repeat and minmax(), and agreed to the following:

  • RESOLVED: Accept change in https://github.com/w3c/csswg-drafts/issues/4043
The full IRC log of that discussion <dael> Topic: Overflow with auto-repeat and minmax()
<dael> github: https://github.com//issues/4043
<dael> astearns: fantasai do you want this?
<dael> TabAtkins: I'm happy to do it
<dael> TabAtkins: We missed where if you do a minmax function and a repeat autofil or autofill we previously based it on max track sizing function. You had to give a real length. minmax(100px, 50px) we would tile columns based on filling 50px and then at layout they're all too big.
<dael> TabAtkins: We now floor the max with the min. Need review from impl if they want. Otherwise should be relatively rubber stamp.
<dael> astearns: This has had review. Anyone want time?
<dael> astearns: Objection to Accept change in https://github.com//issues/4043 ?
<dael> RESOLVED: Accept change in https://github.com//issues/4043

fantasai added a commit to web-platform-tests/wpt that referenced this issue Jul 6, 2020
fantasai added a commit to web-platform-tests/wpt that referenced this issue Jul 6, 2020
@fantasai fantasai added the Tested Memory aid - issue has WPT tests label Jul 6, 2020
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jul 10, 2020
…stonly

Automatic update from web-platform-tests
[css-grid] Test autorepeat with max,min. w3c/csswg-drafts#4043

--
[css-grid] Use background vs. background-color consistently. (editorial/lint)

--

wpt-commits: 9520e0643c5171ff97157469ec342636f0f1cf88, 7f77bdd0d1a3d7ae933026b74342ee0de1afed46
wpt-pr: 24470
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Jul 10, 2020
…stonly

Automatic update from web-platform-tests
[css-grid] Test autorepeat with max,min. w3c/csswg-drafts#4043

--
[css-grid] Use background vs. background-color consistently. (editorial/lint)

--

wpt-commits: 9520e0643c5171ff97157469ec342636f0f1cf88, 7f77bdd0d1a3d7ae933026b74342ee0de1afed46
wpt-pr: 24470
webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Sep 21, 2021
…l be floored by the min

https://bugs.webkit.org/show_bug.cgi?id=230481

Reviewed by Sergio Villar Senin.

LayoutTests/imported/w3c:

Updated test expectation files as all the tests are passing.

* web-platform-tests/css/css-grid/grid-definition/grid-auto-fill-columns-001-expected.txt:
* web-platform-tests/css/css-grid/grid-definition/grid-auto-fill-rows-001-expected.txt:
* web-platform-tests/css/css-grid/grid-definition/grid-auto-fit-columns-001-expected.txt:
* web-platform-tests/css/css-grid/grid-definition/grid-auto-fit-rows-001-expected.txt:

Source/WebCore:

As per discussions in w3c/csswg-drafts#4043, when the max is less than
the min in minmax()we need to floor the max track sizing function by the min track sizing function
when calculating the number of "auto-fit" or "auto-fill" repetitions. This change handles the
situations such as
  -  If both the min and max track sizing functions are definite, use the maximum of them
  -  If only one track sizing function is definite, use that one

* rendering/RenderGrid.cpp:
(WebCore::RenderGrid::computeAutoRepeatTracksCount const):


Canonical link: https://commits.webkit.org/241936@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@282804 268f45cc-cd09-0410-ab3c-d52691b4dbfc
bertogg pushed a commit to Igalia/webkit that referenced this issue Sep 22, 2021
…l be floored by the min

https://bugs.webkit.org/show_bug.cgi?id=230481

Reviewed by Sergio Villar Senin.

LayoutTests/imported/w3c:

Updated test expectation files as all the tests are passing.

* web-platform-tests/css/css-grid/grid-definition/grid-auto-fill-columns-001-expected.txt:
* web-platform-tests/css/css-grid/grid-definition/grid-auto-fill-rows-001-expected.txt:
* web-platform-tests/css/css-grid/grid-definition/grid-auto-fit-columns-001-expected.txt:
* web-platform-tests/css/css-grid/grid-definition/grid-auto-fit-rows-001-expected.txt:

Source/WebCore:

As per discussions in w3c/csswg-drafts#4043, when the max is less than
the min in minmax()we need to floor the max track sizing function by the min track sizing function
when calculating the number of "auto-fit" or "auto-fill" repetitions. This change handles the
situations such as
  -  If both the min and max track sizing functions are definite, use the maximum of them
  -  If only one track sizing function is definite, use that one

* rendering/RenderGrid.cpp:
(WebCore::RenderGrid::computeAutoRepeatTracksCount const):


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@282804 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants