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] Implied Minimum Size of Grid Items #283

Closed
mrego opened this issue Jul 7, 2016 · 42 comments
Closed

[css-grid] Implied Minimum Size of Grid Items #283

mrego opened this issue Jul 7, 2016 · 42 comments

Comments

@mrego
Copy link
Member

mrego commented Jul 7, 2016

As we're approaching the CR stage I was checking again the "Implied Minimum Size of Grid Items" section of the spec.

It's kind of weird that the spec points now again to the text in Flexbox. It was like that originally then it had its own Grid text and now it points again to Flexbox. The problem I found with this is that I'm not 100% sure about some stuff, e.g. the Flexbox spec talks about main-axis, I'm assuming it applies in both axis on grid items. Also when I try to compare with Flexbox, I usually forget about flex-shrink: 1; which is the default in Flebox and creates different results that what I see on Grid.

After trying to understand it properly I was reviewing the W3C tests I created some time ago and updated them. It'd be really nice if someone could verify that my tests are right as some fail on Chromium and another on Firefox.

Please @fantasai and/or @tabatkins could you review the tests (they're just 9 simple tests where the expected result is just a 100x100 green box). Probably @MatsPalmgren would be interested in taking a look too.

Test Chromium Firefox
css-grid-1/grid-items/grid-minimum-size-grid-items-001.xht 👍
css-grid-1/grid-items/grid-minimum-size-grid-items-002.xht 👍
css-grid-1/grid-items/grid-minimum-size-grid-items-003.xht 👍 👍
css-grid-1/grid-items/grid-minimum-size-grid-items-004.xht 👍 👍
css-grid-1/grid-items/grid-minimum-size-grid-items-005.xht 👍
css-grid-1/grid-items/grid-minimum-size-grid-items-006.xht
css-grid-1/grid-items/grid-minimum-size-grid-items-007.xht 👍
css-grid-1/grid-items/grid-minimum-size-grid-items-008.xht 👍 👍
css-grid-1/grid-items/grid-minimum-size-grid-items-009.xht

The status on Chormium and Firefox right now is not really important, as I'm not 100% sure if the tests are right. Thanks! 😉

@fantasai
Copy link
Collaborator

So... as I was editing this to clarify that it applies to both axes (as you mention), I realized that the reason it applies to one axis and not the other in Flexbox is because of the flexibility of the items. I'm wondering if that's something we need to take into account here?

@mrego
Copy link
Member Author

mrego commented Jul 14, 2016

I don't know what's the goal in Grid Layout, but it's supposed that both directions on the grid are symmetric (at least in most situations if not all).
However it's true that Flexbox only do that in one axis (quick example comparing flexbox and grid).

@fantasai
Copy link
Collaborator

I think one of the goals is "do the most expected thing, given Flexbox and author expectations", and of course "be symmetric across axes". The min-width/height: auto behavior was added in part for handling auto-minimum-sized tracks--to make their content-based minimum more configurable. We could, as an alternative to having it always apply, only apply the automatic minimum on items in such tracks.

@fantasai
Copy link
Collaborator

Thinking about this some more, at the very least, the grid's maximum track-sizing function should clamp the implied minimum the same way a specified size on the item would. I'm pretty sure that's what authors would expect.

@mrego
Copy link
Member Author

mrego commented Jul 18, 2016

About author expectations I don't really know what they would be expecting in this situation. Just to make it clear, I'm attaching an example of a 20px x 20px Flexbox and Grid to show the difference between both. Flexbox applies min-size: auto; only in 1 axis and Grid where it applies in both axis.

min-size: auto applying in 1 (flexbox) or both axis (grid)

About your comment:

The min-width/height: auto behavior was added in part for handling auto-minimum-sized tracks--to make their content-based minimum more configurable.

I'm not sure if I'm getting it or not. Do you mean that when you use min-content for a track size, it doesn't use the min-size but it uses the content; and if you use auto for the track size, it checks the min-size and uses it (and when min-size is auto the behavior is equivalent to min-content)?

@fantasai
Copy link
Collaborator

Yes, that's exactly what I meant. That is the reason we changed 'auto' to be something distinct from minmax(min-content, max-content).

@fantasai
Copy link
Collaborator

Proposed wording (append to first paragraph of Implied Minimum Size of Grid Items):

If the grid item is placed in a grid track with a fixed maximum sizing function
(and does not otherwise span a track with a non-fixed maximum sizing function),
its automatic minimum size in that dimension
is further clamped to less than or equal to the size necessary
to fit its margin box within the track (flooring at zero).

@mrego
Copy link
Member Author

mrego commented Jul 18, 2016

Thinking about this some more, at the very least, the grid's maximum track-sizing function should clamp the implied minimum the same way a specified size on the item would. I'm pretty sure that's what authors would expect.

I always thought that the idea of min-size: auto was to avoid shrink-to-fit by default.
At least in Flexbox it avoids that the item shrink-to-fit in the flex container, when the flex container has a fixed width (like in the previous example). Maybe there were other reasons to add it in Flexbox that I'm missing.
Are we sure we want something different in Grid Layout? So the items will shrink-to-fit in their grid area.

@fantasai
Copy link
Collaborator

fantasai commented Jul 18, 2016

Hm, I think “shrink-to-fit” as it's traditionally used is not what you meant. :) But yes, the purpose of min-size: auto was to prevent flex items from shrinking below their min-content size when flexing into a small flex container. We are preserving this behavior for flexible things, i.e. those with a flexible max-track-size.

The track sizing functions are at a sort of intermediary phase between the grid container and the grid item; the flex layout analogy would be flex lines, which don't have any sizing associated with them. Explicitly specifying a fixed size on the tracks is in some way similar to specifying that size on the grid container and in some ways similar to specifying that size on the grid item. But for the purpose of safety -- the reason we have min-size: auto -- it is more similar to specifying that size on the grid item. Because in flex container, we invoke this safety when you have multiple items that, added up up, may or may not overflow the container. But in grid layout, you are fairly sure to target just the one item (in that axis): the author is not going to get any unexpected stacking effects.

(I don't know if that made much sense as an explanation. I'll try again if it didn't.)

@tabatkins
Copy link
Member

In

If the grid item is placed in a grid track with a fixed maximum sizing function
(and does not otherwise span a track with a non-fixed maximum sizing function),
its automatic minimum size in that dimension
is further clamped to less than or equal to the size necessary
to fit its margin box within the track (flooring at zero).

I think the first part is a little awkward. Maybe

If the grid item spans only tracks with fixed maximum sizing functions in a given axis,
...

@fantasai
Copy link
Collaborator

However, if the grid item spans only grid tracks
that have a fixed maximum sizing function,
its automatic minimum size in that dimension
is further clamped to less than or equal to the size necessary
to fit its margin box within the track (flooring at zero).

@mrego
Copy link
Member Author

mrego commented Jul 19, 2016

Mmmm, I'm still not convinced about this. Probably, we should ask for authors feedback on the topic.

Note that the example when you have a fixed track size was discussed before on www-style (Nov 2014) and the implementations got updated accordingly.

@astearns astearns removed the Agenda+ label Jul 26, 2016
@fantasai
Copy link
Collaborator

fantasai commented Aug 9, 2016

I... am having a hard time following that thread. There seems to be vast amounts of confusion over z-index, some additional arguments over TR vs ED, and then some discussion about stretch with the key text

Ah, yes, indeed. 'stretch' will not shrink it below its 'auto' width.
The 10px column merely provides the containing block width, and layout then ends up making it min-content.
(That is, min-content doesn't come automatically; it's just that the rendering which falls out should be the same as if you'd explicitly requesting min-content width.)

The first statement is true: whatever width auto decides, stretch can only increase it. But we are here discussing auto for min-width, which is not the same as auto for width.

The second clause “The 10px column merely provides the containing block width” is also true.

Then we have “and layout then ends up making it min-content” ... at that point the spec put min-content minimums on all auto-sized tracks, but this track is not auto-sized. And there was no implied minimum size on items themselves; min-width: auto still computed to zero. (Auto minimums were added to Grid in https://lists.w3.org/Archives/Public/www-style/2015Jan/0166.html which was January 2015.) So I am not sure where this assertion is coming from.

Agree it would be good to have author feedback; my feeling though is that fixed track sizes are thought of similarly to fixed sizes on the item itself, and it's good for the automatic minimum to not overrule that. (Auto minimums are supposed to be minimally invasive. ;)

The interesting question then becomes, what does auto mean for width/height themselves on a grid item? It Doesn't appear that this is defined. The only definition I'm seeing is https://drafts.csswg.org/css-align/#justify-self-property which only defines it for stretch.

@fantasai
Copy link
Collaborator

fantasai commented Aug 9, 2016

Nevermind, found it in https://www.w3.org/TR/css-grid-1/#grid-items That section needs more subheadings, I think. >_<

@fantasai
Copy link
Collaborator

fantasai commented Aug 9, 2016

Filed #394 on sizing grid items. There's still some spec issues there afaict.

@fantasai
Copy link
Collaborator

Ok, I went over this issue with the Microsoft folks, and my conclusion is that we need to make sure grid items are sized the same (in both axes) as flex items are in the cross axis. See #394 for further discussion.

Concretely, this means applying the CSS2.1 non-replaced block width formula to items when they are stretched, and fit-content sizing for other alignment values, and it means that the implied minimum can't take effect on fixed tracks. Note that on auto-sized tracks, whether the implied minimum is used solely for track sizing or also for sizing the item itself is indistinguishable.

Additionally, the major consideration for adding implied minimum sizes to the main axis of flex items was for the viewer's benefit when the screen was made too small. Flex items do not shrink past their minimum main size--but importantly, they also do not overlap each other within a line, because their positions are determined by previous content, not by the position/size of their container (the line). Grid items, if they were floored at their content-size, do not shift in such a manner, and they would overlap each other within the row/column. Forcing a minimum on the item doesn't really help the user so much in this case.

@mrego
Copy link
Member Author

mrego commented Sep 8, 2016

Let's see if I'm understanding the last comment. 😄

Coming back to one of the examples discussed, a grid with a fixed track of 20x20:

  <div style="display: grid; grid: 20px / 20px; width: 20px; height: 20px; border: thick solid; font: 50px/1 Monospace;">
    <div style="background: magenta;">item</div>
  </div>

It seems to me that the behavior we're looking for is that the item is sized to 20x20, and the rest of the text overflows. So it could be something like in the following picture:
Expected output for previous example: a 20x20 magenta block with a black border

And that's the default behavior as align|justify-items is stretch by default.

However, if align|justify-items is start the expected behavior would be like the next picture:
Expected output with align|justify-items: a magenta block filling the whole text (bigger than 20x20)

If I'm getting it right then we might need to update the spec in a few places:

  • 6.2. Grid Item Sizing

    Note: The auto value of min-width and min-height affects track sizing
    in the relevant axis similar to the flexbox behavior on flex items in the main axis.

    Shouldn't say in the "cross axis"?

  • 6.6. Implied Minimum Size of Grid Items

    (The effect is analogous to the automatic minimum size imposed on flex items in their main axis.)

    Again "cross axis"?

    However, if the grid item spans only grid tracks that have a fixed max track sizing function,
    its automatic minimum size in that dimension is further clamped to less than or equal to
    the size necessary to fit its margin box within the resulting grid area (flooring at zero).

    Does this apply in both stretch and start case?

@fantasai
Copy link
Collaborator

It seems to me that the behavior we're looking for is that the item is sized to 20x20, and the rest of the text overflows.
And that's the default behavior as align|justify-items is stretch by default.
However, if align|justify-items is start the expected behavior would be like the next picture

Yes, exactly.

Shouldn't say in the "cross axis"?

The implied minimum doesn't take effect in the cross axis at all for flex items. This is because flex lines are always auto-sized anyway. (Except for single-line flex containers... but in that case the items would overflow the flex container in more or less the same way.)

Does this apply in both stretch and start case?

Definitely applies for stretch. For the start case, afaict it makes no difference. Let me know if that assumption is incorrect. :)

@mrego
Copy link
Member Author

mrego commented Sep 16, 2016

The implied minimum doesn't take effect in the cross axis at all for flex items. This is because flex lines are always auto-sized anyway. (Except for single-line flex containers... but in that case the items would overflow the flex container in more or less the same way.)

Mmmm, but in a previous comment #283 (comment) you said:

my conclusion is that we need to make sure grid items are sized the same (in both axes) as flex items are in the cross axis

So using a flexbox as example:

  <div style="display: flex; width: 20px; height: 20px; border: thick solid; font: 50px/1 Monospace;">
    <div style="background: magenta;">item</div>
  </div>

The output is the following:
screenshot from 2016-09-16 10-02-20

  • In the main axis (width), the "min-width: auto" applies, so the size of the item is bigger than the 20px of the flex container.
  • In the cross axis (height), the implied minimum size doesn't have any effect, so the height of the item is reduced to 20px.

In the previous comment you confimred that the behavior for grids in the similar example should be a 20x20 item. That's why I was understanding it was behaving the same than in the cross axis of flexboxes.

Also note than if in the flexbox we use align-items: flex-start;, the height of the item will be bigger than the 20px. Again behaving exactly the same than the expected behavior we've just discussed for grid.

So probably I'm missing something but I'm not understanding the whole thing yet, sorry. 😔

@mrego
Copy link
Member Author

mrego commented Jan 10, 2017

It makes sense to me and it seems it matches current Chromium behavior.

I guess that the track sizes for the example are also A) 40px, B) 40px and C) 250px.

And if we add one more case to the example:
D) With minmax(auto, 0px) or minmax(auto, 500px) (it doesn't matter the number of pixels we set as maximum) the size of the item and the track will be 250px too.

MXEBot pushed a commit to mirror/chromium that referenced this issue Feb 2, 2017
This patch imports the CSS Grid Layout test suite from csswg-test repository.

Currently we're passing most of the but:
* grid-layout-properties.html: An old known bug (crbug.com/511177).
* grid-support-grid-template-areas-001.xht &
  grid-inline-support-grid-template-areas-001.xht: Because of an issue
  with testharness.js (crbug.com/687492).

Additionally I'm skipping the tests related to "Implied Minimum Size
of Grid Items" (https://drafts.csswg.org/css-grid/#min-size-auto),
because the test suite needs to be updated after the CSS WG resolves
the following issue: w3c/csswg-drafts#283
See crbug.com/666940 for more details.

BUG=687494

Review-Url: https://codereview.chromium.org/2670473003
Cr-Commit-Position: refs/heads/master@{#447562}
mrego added a commit to mrego/csswg-test that referenced this issue Feb 14, 2017
Once that issue w3c/csswg-drafts#283 has been solved, this patch
is updating the current tests related to "Implied Minimum Size
of Grid Items" section:
https://drafts.csswg.org/css-grid/#min-size-auto
mrego added a commit to mrego/csswg-test that referenced this issue Feb 14, 2017
This adds more coverage for that spec section:
https://drafts.csswg.org/css-grid/#min-size-auto

The examples in the tests were discussed in w3c/csswg-drafts#283.
@mrego
Copy link
Member Author

mrego commented Feb 15, 2017

Thanks for resolving this issue, we've been working on adding tests for this feature to the csswg-test repository (18 tests total):

It'd be nice if you could take a look to them and verify that they're right or not.

gsnedders pushed a commit to jgraham/css-test-built that referenced this issue Mar 2, 2017
Once that issue w3c/csswg-drafts#283 has been solved, this patch
is updating the current tests related to "Implied Minimum Size
of Grid Items" section:
https://drafts.csswg.org/css-grid/#min-size-auto

Build from revision 9ec6c7ab79af2eb6c3fe7d6165b1fffd81c81b30
gsnedders pushed a commit to jgraham/css-test-built that referenced this issue Mar 2, 2017
This adds more coverage for that spec section:
https://drafts.csswg.org/css-grid/#min-size-auto

The examples in the tests were discussed in w3c/csswg-drafts#283.

Build from revision 283588f54a347226b0d163d9c7ef7e0c5874af48
hubot pushed a commit to WebKit/WebKit-http that referenced this issue Mar 6, 2017
https://bugs.webkit.org/show_bug.cgi?id=163200

Reviewed by Darin Adler.

Source/WebCore:

After some discussions the CSS WG agreed that stretch should not only
grow items, but also shrink them to fit its grid area.
That way the "min-width|height: auto" is somehow ignored for grid items.
More info at: w3c/csswg-drafts#283

The good part is that this allows us to remove some ugly code we've
in RenderBox that was only used by Grid Layout.

For images, we'll be stretching on both axis right now, so the aspect
ratio won't be preserved. The default behavior might change in those
cases, but that should be implemented in a different patch.

* rendering/RenderBox.cpp:
(WebCore::RenderBox::computeLogicalWidthInRegion):
(WebCore::RenderBox::computeLogicalHeight):

LayoutTests:

The tests have been updated according to the new expected behavior.

* fast/css-grid-layout/grid-container-percentage-columns.html:
* fast/css-grid-layout/min-width-height-auto-and-margins.html:
* fast/css-grid-layout/min-width-height-auto.html:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@213449 268f45cc-cd09-0410-ab3c-d52691b4dbfc
bertogg pushed a commit to Igalia/webkit that referenced this issue Mar 14, 2017
…t its grid area

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

Reviewed by Darin Adler.

Source/WebCore:

After some discussions the CSS WG agreed that stretch should not only
grow items, but also shrink them to fit its grid area.
That way the "min-width|height: auto" is somehow ignored for grid items.
More info at: w3c/csswg-drafts#283

The good part is that this allows us to remove some ugly code we've
in RenderBox that was only used by Grid Layout.

For images, we'll be stretching on both axis right now, so the aspect
ratio won't be preserved. The default behavior might change in those
cases, but that should be implemented in a different patch.

* rendering/RenderBox.cpp:
(WebCore::RenderBox::computeLogicalWidthInRegion):
(WebCore::RenderBox::computeLogicalHeight):

LayoutTests:

The tests have been updated according to the new expected behavior.

* fast/css-grid-layout/grid-container-percentage-columns.html:
* fast/css-grid-layout/min-width-height-auto-and-margins.html:
* fast/css-grid-layout/min-width-height-auto.html:

git-svn-id: http://svn.webkit.org/repository/webkit/releases/WebKitGTK/webkit-2.16@213816 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@fantasai fantasai added Needs Review of Test Case(s) Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. labels Oct 17, 2017
@mrego
Copy link
Member Author

mrego commented Nov 20, 2017

D) With minmax(auto, 0px) or minmax(auto, 500px) (it doesn't matter the number of pixels we set as maximum) the size of the item and the track will be 250px too.

I believe this comment is wrong, and thus the following test is wrong too http://w3c-test.org/css/css-grid/grid-items/grid-minimum-size-grid-items-017.html
At least Firefox and Edge show a red rectangle there (in the current version as of 20th Nov 2017). Both Blink and WebKit show a green one, because I implemented both wrong following the assumption that the previous comment is right.

Basically this is wrong because the automatic minimum size has to be clamped as the max track sizing function is fixed. So in the case of minmax(auto, 0px) the automatic minimum size of the item gets clamped to 0px instead of 250px.

The Chromium bug is reported and will be fixed in the coming days: https://bugs.chromium.org/p/chromium/issues/detail?id=786971

@fantasai fantasai added this to the css-grid-1 WD 2016-05-19+ milestone Jan 23, 2019
JonWBedard pushed a commit to WebKit/WebKit that referenced this issue Nov 29, 2022
…t its grid area

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

Reviewed by Darin Adler.

Source/WebCore:

After some discussions the CSS WG agreed that stretch should not only
grow items, but also shrink them to fit its grid area.
That way the "min-width|height: auto" is somehow ignored for grid items.
More info at: w3c/csswg-drafts#283

The good part is that this allows us to remove some ugly code we've
in RenderBox that was only used by Grid Layout.

For images, we'll be stretching on both axis right now, so the aspect
ratio won't be preserved. The default behavior might change in those
cases, but that should be implemented in a different patch.

* rendering/RenderBox.cpp:
(WebCore::RenderBox::computeLogicalWidthInRegion):
(WebCore::RenderBox::computeLogicalHeight):

LayoutTests:

The tests have been updated according to the new expected behavior.

* fast/css-grid-layout/grid-container-percentage-columns.html:
* fast/css-grid-layout/min-width-height-auto-and-margins.html:
* fast/css-grid-layout/min-width-height-auto.html:
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

7 participants