Bug 191627 – [css-grid] Incorrect track sizing when using relative sized items in 'auto' column tracks

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

[Javier Fernandez]

Created attachment 354786 [details]
Test case to reproduce the issue

Steps to reproduce the issue:

1- load the attached test case

What is the expected behavior?
The first column track size should be 50px, based on the fixed sized item placed in the first row (see expected.png)


What went wrong ?
The first column track size is 200px, based on the grid container width (100px) and the relative sized item placed in the second row (see actual.png)

[Javier Fernandez]

Some corrections about the expected size of the first column track:

What is the expected behavior?
The first column track size should be 62.5px, based on the fixed sized item placed in the first row (see expected.png)


What went wrong ?
The first column track size is 200px, based on the grid container width (100px) and the relative sized item placed in the second row (see actual.png)

[Javier Fernandez]

Created attachment 354788 [details]
Expected result

[Javier Fernandez]

Created attachment 354789 [details]
Actual result

[Javier Fernandez]

The root cause of this bug seems that we have an incorrect implementation of  RenderBox::containingBlockLogicalWidthForContent method for grid items (probably flex items too) with relative size.

The case is especially noticeable with orthogonal flows (vertical items), since the grid container's width is always definite (resolving the intrinsic width if needed), so that the grid item's logical height is based on the container's definite width (wrongly, as I'll explain now).

For grid items, the containing block shouldn't be the grid container, but the grid area it's located in. During the GridTrackSizingAlgorithm, we determine that this item has a relative size (percent in this case) and that the grid area is still indefinite (auto column track). Hence, we set nullopt for the grid area width and mark the item for layout. 

During the item's layout, we call this RenderBox::containingBlockLogicalWidthForContent to determine the available height. However, this method checks the containing block width's if the overridingContentLogicalWidth is nullopt, like in this case. This is incorrect for grid items; we should never use the grid container for resolving grid item's size.

Created attachment 421440 [details]
Patch

[Javier Fernandez]

Comment on attachment 421440 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421440&action=review

We don't need the new test grid-item-auto-track-and-relative-sized.html since that case is already defined in grid-self-baseline-not-applied-if-sizing-cyclic-dependency-001.html, which is now passing thanks to this change. 

Additionally, we shouldn't add an entry to the w3c-import.log because this test has not been imported from the WTP repo, given that it's a new test that doesn't exists in the mentioned repository.

If you want to add a new test, we can create a new one indeed; if it's a reference tests perhaps it can provide something that the testharness may miss. It's a bad idea to create a pixel test like this, though. If you want to proceed, you'd need to create a reference html tests that can be used to describe the expected result.

> Source/WebCore/rendering/RenderBox.cpp:2046
> +        return overridingContainingBlockContentLogicalWidth().valueOr(0_lu);

I'm not sure whether this function should have the responsibility of deciding what to do in case of a nullopt value. The RenderBox class is understanding a nullopt value from these functions as an indefinite height. As a matter of fact, this function is called by RenderBox::computePercentageLogicalHeight to compute the availableLogicalHeight, which is defined as Optional variable. I admit that returning an Optional value would complicate other callers of this method, so we should think carefully what's the best approach here.

On the other hand, I think we have the same problem with containingBlockLogicalHeightForContent() function. I think it make sense to apply this change to both functions.

Created attachment 421638 [details]
Patch

[Javier Fernandez]

Comment on attachment 421638 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421638&action=review

> Source/WebCore/rendering/RenderBox.cpp:2058
> +    // height value we get from overridingContainingBlockContentLogicalHeight() here, including WTPF::nullopt.

Nit: s/ WTPF::nullopt/WTF::nullopt
Nit: I think we could write the comment in a single line.

> LayoutTests/imported/w3c/ChangeLog:8
> +        * web-platform-tests/css/css-grid/alignment/grid-self-baseline-not-applied-if-sizing-cyclic-dependency-001-expected.txt:

Maybe we can comment that the root cause of the failures is what we've got in the FIXME comment.

Created attachment 421640 [details]
Patch

(In reply to Javier Fernandez from comment #8)
> Comment on attachment 421638 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421638&action=review
> 
> > Source/WebCore/rendering/RenderBox.cpp:2058
> > +    // height value we get from overridingContainingBlockContentLogicalHeight() here, including WTPF::nullopt.
> 
> Nit: s/ WTPF::nullopt/WTF::nullopt
> Nit: I think we could write the comment in a single line.
> 
> > LayoutTests/imported/w3c/ChangeLog:8
> > +        * web-platform-tests/css/css-grid/alignment/grid-self-baseline-not-applied-if-sizing-cyclic-dependency-001-expected.txt:
> 
> Maybe we can comment that the root cause of the failures is what we've got
> in the FIXME comment.

Updated. Thanks!

[Sergio Villar Senin]

Comment on attachment 421640 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421640&action=review

> Source/WebCore/ChangeLog:8
> +        For grid items, grid container is the grid area it's located in rather than the containing block.

I think this should be reworded because it looks like there is one thing that is the containing block and another which is the grid area. For grid items the containing block is the grid area.

> Source/WebCore/rendering/RenderBox.cpp:2046
> +        return overridingContainingBlockContentLogicalWidth().valueOr(LayoutUnit());

Hmm if hasOverridingContainingBlockContentLogicalWidth() is true then we know that it has a value, then why do we need the valueOr() ?

> Source/WebCore/rendering/RenderBox.cpp:2057
> +    // FIXME: For grid items, the grid container is the grid area it's located in. We need to return whatever height value we get from overridingContainingBlockContentLogicalHeight() here, including WTF::nullopt.

Nit, "the grid container is" -> "the containing block is"

Created attachment 421659 [details]
Patch

[Javier Fernandez]

Comment on attachment 421640 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421640&action=review

>> Source/WebCore/rendering/RenderBox.cpp:2046
>> +        return overridingContainingBlockContentLogicalWidth().valueOr(LayoutUnit());
> 
> Hmm if hasOverridingContainingBlockContentLogicalWidth() is true then we know that it has a value, then why do we need the valueOr() ?

No, the HasOverrideXXX function only tells whether a value has been set (which is always true for grid items), bit it might be nullopt (to specify indefinite size)

Yes, the value might not have been initialized/set. While running the test case here (grid-self-baseline-not-applied-if-sizing-cyclic-dependency-001.html), without the valueOr(), this test crashes and throws "STDERR: ASSERTION FAILED: initialized()".

Would the width be nullopt? I'm just wondering why in RenderGrid::placeItemsOnGrid(), it's initialized as LayoutUnit() rather than LayoutUnit(-1) like the height?

(In reply to zsun from comment #14)
> Yes, the value might not have been initialized/set. While running the test
> case here
> (grid-self-baseline-not-applied-if-sizing-cyclic-dependency-001.html),
> without the valueOr(), this test crashes and throws "STDERR: ASSERTION
> FAILED: initialized()".
> 
> Would the width be nullopt? I'm just wondering why in
> RenderGrid::placeItemsOnGrid(), it's initialized as LayoutUnit() rather than
> LayoutUnit(-1) like the height?

(In reply to Javier Fernandez from comment #13)
> Comment on attachment 421640 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421640&action=review
> 
> >> Source/WebCore/rendering/RenderBox.cpp:2046
> >> +        return overridingContainingBlockContentLogicalWidth().valueOr(LayoutUnit());
> > 
> > Hmm if hasOverridingContainingBlockContentLogicalWidth() is true then we know that it has a value, then why do we need the valueOr() ?
> 
> No, the HasOverrideXXX function only tells whether a value has been set
> (which is always true for grid items), bit it might be nullopt (to specify
> indefinite size)

Would it make sense to initiate it as 0 for width just as RenderGrid::placeItemsOnGrid() does?

Created attachment 423024 [details]
Patch

Created attachment 423380 [details]
Patch

[Javier Fernandez]

Comment on attachment 423380 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423380&action=review

Do we know the cause of the ios failures ?

> Source/WebCore/ChangeLog:8
> +        For grid items, containing block is the grea area. During the GridTrackSizingAlgorithm, we determine

s/grea/grid

> Source/WebCore/rendering/RenderBox.cpp:2058
> +        return overridingContainingBlockContentLogicalHeight().valueOr(0_lu);

I'm not sure we want to return 0 in case of nullopt, which would mean we have an indefinite grid area size. I'm fine with this change for now, but I'd like to add a FIXME comment to review this if necessary. The idea solution would be to use an optional value, but I admit it'd imply a bigger change, since this function is widely consumed in RenderBox layout logic.

Created attachment 423484 [details]
Patch

(In reply to Javier Fernandez from comment #18)
> Comment on attachment 423380 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423380&action=review
> 
> Do we know the cause of the ios failures ?

Not sure. This patch has improved the test result in ios but not fully fixed it.
The original test expected results for ios are different from the one for mac as well.
> 
> > Source/WebCore/ChangeLog:8
> > +        For grid items, containing block is the grea area. During the GridTrackSizingAlgorithm, we determine
> 
> s/grea/grid
> 
Updated.
> > Source/WebCore/rendering/RenderBox.cpp:2058
> > +        return overridingContainingBlockContentLogicalHeight().valueOr(0_lu);
> 
> I'm not sure we want to return 0 in case of nullopt, which would mean we
> have an indefinite grid area size. I'm fine with this change for now, but
> I'd like to add a FIXME comment to review this if necessary. The idea
> solution would be to use an optional value, but I admit it'd imply a bigger
> change, since this function is widely consumed in RenderBox layout logic.

Added a FIXME comment.

Thanks!

[Javier Fernandez]

Comment on attachment 423484 [details]
Patch

r=me

[EWS]

Committed r274596: <https://commits.webkit.org/r274596>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423484 [details].

[Radar WebKit Bug Importer]

<rdar://problem/75548318>

[zalan]

This broke google.com see https://trac.webkit.org/changeset/274741/webkit

[zalan]

Committed r274741 (235558@main): <https://commits.webkit.org/235558@main>

[zalan]

r274741

Created attachment 423870 [details]
Patch

[Javier Fernandez]

Comment on attachment 423870 [details]
Patch

r=me

[EWS]

Committed r275908 (236472@main): <https://commits.webkit.org/236472@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423870 [details].