There were some discussion on GitHub about minimum size of grid items: https://github.com/w3c/csswg-drafts/issues/283 The spec has been modified so it now includes the following sentence (https://drafts.csswg.org/css-grid/#min-size-auto): "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)." This has changed quite a lot the default behavior, so if a grid cell is very small the item should be shrunk to fit into that cell. This has been already fixed on Blink: https://codereview.chromium.org/2398043002
Created attachment 291076 [details] Patch
Comment on attachment 291076 [details] Patch Removing that code looks awesome to me, but I wonder where the "if the grid item spans only grid tracks that have a fixed max track sizing function" restriction is considered now. Shouldn't we keep the current behavior for the other cases? Shouldn't we add new test cases for these two possible situations?
Comment on attachment 291076 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291076&action=review > Source/WebCore/rendering/RenderBox.cpp:2429 > if (treatAsReplaced) { > computedValues.m_extent = logicalWidthLength.value() + borderAndPaddingLogicalWidth(); > -#if ENABLE(CSS_GRID_LAYOUT) > - } else if (parent()->isRenderGrid() && style().logicalWidth().isAuto() && style().logicalMinWidth().isAuto() && style().overflowX() == OVISIBLE && containerWidthInInlineDirection < minPreferredLogicalWidth()) { > - // TODO (lajava) Move this logic to the LayoutGrid class. > - // Implied minimum size of Grid items. > - computedValues.m_extent = constrainLogicalWidthInRegionByMinMax(minPreferredLogicalWidth(), containerWidthInInlineDirection, cb); > -#endif > } else { WebKit coding style uses no braces for a one line if body. > Source/WebCore/rendering/RenderBox.cpp:2838 > )) { > - LayoutUnit contentHeight = overrideLogicalContentHeight(); > -#if ENABLE(CSS_GRID_LAYOUT) > - if (parent()->isRenderGrid() && style().logicalHeight().isAuto() && style().logicalMinHeight().isAuto() && style().overflowX() == OVISIBLE) { > - LayoutUnit intrinsicContentHeight = computedValues.m_extent - borderAndPaddingLogicalHeight(); > - if (auto minContentHeight = computeContentLogicalHeight(MinSize, Length(MinContent), intrinsicContentHeight)) > - contentHeight = std::max(contentHeight, constrainContentBoxLogicalHeightByMinMax(minContentHeight.value(), intrinsicContentHeight)); > - } > -#endif > - h = Length(contentHeight, Fixed); > + h = Length(overrideLogicalContentHeight(), Fixed); > } else if (treatAsReplaced) WebKit coding style uses no braces for a one line if body.
(In reply to comment #2) > Removing that code looks awesome to me, but I wonder where the "if the grid > item spans only grid tracks that have a fixed max track sizing function" > restriction is considered now. Shouldn't we keep the current behavior for > the other cases? Shouldn't we add new test cases for these two possible > situations? Yes it's true we're not checking those cases. However I'm not 100% sure what would be the expected behavior, let me clarify it with the CSS WG before moving this forward: https://github.com/w3c/csswg-drafts/issues/283#issuecomment-252584331
Created attachment 303508 [details] Rebased patch
(In reply to comment #4) > (In reply to comment #2) > > Removing that code looks awesome to me, but I wonder where the "if the grid > > item spans only grid tracks that have a fixed max track sizing function" > > restriction is considered now. Shouldn't we keep the current behavior for > > the other cases? Shouldn't we add new test cases for these two possible > > situations? > > Yes it's true we're not checking those cases. > > However I'm not 100% sure what would be the expected behavior, let me > clarify it with the CSS WG before moving this forward: > https://github.com/w3c/csswg-drafts/issues/283#issuecomment-252584331 The issues have been clarified, and the expected behavior matches this patch implementation. There are tests on the W3C test suite checking the cases explained before, and I'm working on getting it imported (see bug #169196). @svillar would you be fine with landing this as is? That would move us closer to Blink implementation, and we can always add more tests cases later if needed. What do you think?
(In reply to comment #6) > (In reply to comment #4) > > (In reply to comment #2) > > > Removing that code looks awesome to me, but I wonder where the "if the grid > > > item spans only grid tracks that have a fixed max track sizing function" > > > restriction is considered now. Shouldn't we keep the current behavior for > > > the other cases? Shouldn't we add new test cases for these two possible > > > situations? > > > > Yes it's true we're not checking those cases. > > > > However I'm not 100% sure what would be the expected behavior, let me > > clarify it with the CSS WG before moving this forward: > > https://github.com/w3c/csswg-drafts/issues/283#issuecomment-252584331 > > The issues have been clarified, and the expected behavior matches this patch > implementation. > > There are tests on the W3C test suite checking the cases explained before, > and I'm working on getting it imported (see bug #169196). > > @svillar would you be fine with landing this as is? > That would move us closer to Blink implementation, > and we can always add more tests cases later if needed. > What do you think? After carefully thinking about it, I think this patch addresses my concerns because it properly stretches items spanning tracks with fixed max track sizing functions. For the other cases (i.e. content sized functions) the tracks would be properly sized to the contents of the items, so they would never overflow. Anyway, it would be awesome to add more test cases for: a) items spanning only non fixed max track sizing functions b) items spanning none non fixed max track sizing functions c) items spanning both types of tracks and check how the same item with the same min-size behaves differently (actually cases b and c should implement the same behavior). I wouldn't block the landing of this patch due to the lack of those tests if you add then later.
Comment on attachment 303508 [details] Rebased patch (In reply to comment #7) > I wouldn't block the landing of this patch due to the lack of those tests if > you add then later. Ok, thanks for the new comment. Some of the cases you mentioned are already covered by the W3C Test Suite, and I'm planning to import it very soon. Then I could add there more tests if needed and get them imported.
Comment on attachment 303508 [details] Rebased patch Clearing flags on attachment: 303508 Committed r213449: <http://trac.webkit.org/changeset/213449>
All reviewed patches have been landed. Closing bug.