Bug 191627 - [css-grid] Incorrect track sizing when using relative sized items in 'auto' column tracks
Summary: [css-grid] Incorrect track sizing when using relative sized items in 'auto' c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 221439
  Show dependency treegraph
 
Reported: 2018-11-14 02:08 PST by Javier Fernandez
Modified: 2021-04-13 13:52 PDT (History)
16 users (show)

See Also:


Attachments
Test case to reproduce the issue (873 bytes, text/html)
2018-11-14 02:08 PST, Javier Fernandez
no flags Details
Expected result (690 bytes, image/png)
2018-11-14 03:06 PST, Javier Fernandez
no flags Details
Actual result (912 bytes, image/png)
2018-11-14 03:06 PST, Javier Fernandez
no flags Details
Patch (13.52 KB, patch)
2021-02-24 12:23 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (18.50 KB, patch)
2021-02-26 06:54 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (18.66 KB, patch)
2021-02-26 07:12 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (18.60 KB, patch)
2021-02-26 09:25 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (19.43 KB, patch)
2021-03-12 02:06 PST, zsun
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (19.45 KB, patch)
2021-03-16 12:39 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (19.55 KB, patch)
2021-03-17 06:55 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (19.81 KB, patch)
2021-03-22 04:36 PDT, zsun
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Javier Fernandez 2018-11-14 02:08:28 PST
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)
Comment 1 Javier Fernandez 2018-11-14 03:02:56 PST
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)
Comment 2 Javier Fernandez 2018-11-14 03:06:00 PST
Created attachment 354788 [details]
Expected result
Comment 3 Javier Fernandez 2018-11-14 03:06:27 PST
Created attachment 354789 [details]
Actual result
Comment 4 Javier Fernandez 2021-02-23 15:13:49 PST
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.
Comment 5 zsun 2021-02-24 12:23:34 PST
Created attachment 421440 [details]
Patch
Comment 6 Javier Fernandez 2021-02-24 14:02:18 PST
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.
Comment 7 zsun 2021-02-26 06:54:22 PST
Created attachment 421638 [details]
Patch
Comment 8 Javier Fernandez 2021-02-26 06:59:10 PST
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.
Comment 9 zsun 2021-02-26 07:12:20 PST
Created attachment 421640 [details]
Patch
Comment 10 zsun 2021-02-26 07:12:48 PST
(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!
Comment 11 Sergio Villar Senin 2021-02-26 08:13:37 PST
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"
Comment 12 zsun 2021-02-26 09:25:06 PST
Created attachment 421659 [details]
Patch
Comment 13 Javier Fernandez 2021-02-26 09:26:14 PST
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)
Comment 14 zsun 2021-02-26 11:44:37 PST
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?
Comment 15 zsun 2021-03-08 12:35:18 PST
(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?
Comment 16 zsun 2021-03-12 02:06:21 PST
Created attachment 423024 [details]
Patch
Comment 17 zsun 2021-03-16 12:39:04 PDT
Created attachment 423380 [details]
Patch
Comment 18 Javier Fernandez 2021-03-17 06:38:53 PDT
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.
Comment 19 zsun 2021-03-17 06:55:51 PDT
Created attachment 423484 [details]
Patch
Comment 20 zsun 2021-03-17 06:59:15 PDT
(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!
Comment 21 Javier Fernandez 2021-03-17 15:54:32 PDT
Comment on attachment 423484 [details]
Patch

r=me
Comment 22 EWS 2021-03-17 15:59:14 PDT
Committed r274596: <https://commits.webkit.org/r274596>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423484 [details].
Comment 23 Radar WebKit Bug Importer 2021-03-17 16:01:09 PDT
<rdar://problem/75548318>
Comment 24 zalan 2021-03-19 15:14:57 PDT
This broke google.com see https://trac.webkit.org/changeset/274741/webkit
Comment 25 zalan 2021-03-19 15:14:58 PDT
Committed r274741 (235558@main): <https://commits.webkit.org/235558@main>
Comment 26 zalan 2021-03-19 15:15:44 PDT
r274741
Comment 27 zsun 2021-03-22 04:36:09 PDT
Created attachment 423870 [details]
Patch
Comment 28 Javier Fernandez 2021-04-13 13:27:23 PDT
Comment on attachment 423870 [details]
Patch

r=me
Comment 29 EWS 2021-04-13 13:52:10 PDT
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].