Bug 163200 – [css-grid] New expected behavior for stretch

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Manuel Rego Casasnovas]

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

[Manuel Rego Casasnovas]

Created attachment 291076 [details]
Patch

[Sergio Villar Senin]

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?

[Darin Adler]

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.

[Manuel Rego Casasnovas]

(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

[Manuel Rego Casasnovas]

Created attachment 303508 [details]
Rebased patch

[Manuel Rego Casasnovas]

(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?

[Sergio Villar Senin]

(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.

[Manuel Rego Casasnovas]

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.

[WebKit Commit Bot]

Comment on attachment 303508 [details]
Rebased patch

Clearing flags on attachment: 303508

Committed r213449: <http://trac.webkit.org/changeset/213449>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.