-
Notifications
You must be signed in to change notification settings - Fork 707
[css-grid] Implied Minimum Size of Grid Items #283
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
Comments
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? |
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). |
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 |
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. |
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 About your comment:
I'm not sure if I'm getting it or not. Do you mean that when you use |
Yes, that's exactly what I meant. That is the reason we changed 'auto' to be something distinct from minmax(min-content, max-content). |
Proposed wording (append to first paragraph of Implied Minimum Size of Grid Items):
|
I always thought that the idea of |
Hm, I think “shrink-to-fit” as it's traditionally used is not what you meant. :) But yes, the purpose of 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 (I don't know if that made much sense as an explanation. I'll try again if it didn't.) |
In
I think the first part is a little awkward. Maybe
|
|
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 |
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
The first statement is true: whatever 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; 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 |
Nevermind, found it in https://www.w3.org/TR/css-grid-1/#grid-items That section needs more subheadings, I think. >_< |
Filed #394 on sizing grid items. There's still some spec issues there afaict. |
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. |
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: And that's the default behavior as However, if If I'm getting it right then we might need to update the spec in a few places:
|
Yes, exactly.
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.)
Definitely applies for |
Mmmm, but in a previous comment #283 (comment) you said:
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>
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 So probably I'm missing something but I'm not understanding the whole thing yet, sorry. 😔 |
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: |
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}
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
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.
Thanks for resolving this issue, we've been working on adding tests for this feature to the
It'd be nice if you could take a look to them and verify that they're right or not. |
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
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
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
…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
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 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 The Chromium bug is reported and will be fixed in the coming days: https://bugs.chromium.org/p/chromium/issues/detail?id=786971 |
…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:
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.
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! 😉
The text was updated successfully, but these errors were encountered: