Bug 209650 - [css-grid] WPT Tests css/css-grid/grid-items/grid-item-percentage-sizes-*.html fail
Summary: [css-grid] WPT Tests css/css-grid/grid-items/grid-item-percentage-sizes-*.htm...
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Keywords: InRadar
Depends on:
Reported: 2020-03-27 08:24 PDT by Carlos Alberto Lopez Perez
Modified: 2021-04-22 16:08 PDT (History)
13 users (show)

See Also:

Patch (5.05 KB, patch)
2021-04-22 02:23 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (4.67 KB, patch)
2021-04-22 06:13 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 Carlos Alberto Lopez Perez 2020-03-27 08:24:34 PDT
The following ref-test failures happen on WebKit for the WPT tests:


Check here the diff of the images:

^ click on the test name, then on compare on either safari or webkitgtk and a new window will appear
Comment 1 Carlos Alberto Lopez Perez 2020-03-30 11:09:58 PDT
This 3 tests were added to WPT from Mozilla bug https://bugzil.la/1526567
See also the Chromium https://crbug.com/890278 which is also related to this issue.

As fas as I can see there are several bugs contributing to the failures on this tests.

For example, on css/css-grid/grid-items/grid-item-percentage-sizes-001.html we have this:

Row1 : .hl  .item { writing-mode: horizontal-tb; direction:ltr; }
Row2 : .vrl .item { writing-mode: vertical-rl; direction:ltr; }
Row3 : .vrr .item { writing-mode: vertical-rl; direction:rtl; }
Row4 : .vll .item { writing-mode: vertical-lr; direction:ltr; }
Row5 : .vlr .item { writing-mode: vertical-lr; direction:rtl; }
Row6 : .sll .item { writing-mode: sideways-lr; direction:ltr; }
Row7 : .slr .item { writing-mode: sideways-lr; direction:rtl; }

There are 7 rows with diferent writing-mode. And each row has 7 columns.

Each one of this 7 columns tests different combinations of min-height/min-width, max-height/max-width and width/height.

Here is the first row (extracted from the test): https://codepen.io/cl0p3z/pen/abOXjRz
On Chrome and Firefox the last 3 blue squares have a small size, but not on WebKit.

It looks something perhaps related to min-width/min-height, it seems on WebKit that has preference over width/height?

On top of that, on the rows with vertical writing mode there are more failures, so I guess perhaps some of those are related to bug 180633 ?
Comment 2 Oriol Brufau 2020-03-30 12:21:12 PDT
In Chromium, grid-item-percentage-sizes-001.html and grid-item-percentage-sizes-002.html were fixed in https://crbug.com/931474.

In WebKit, the corresponding issue is bug 195967, which I should fix since I already have a patch. The problem was that the tests improved but still failed due to unrelated issues.

Regarding grid-item-percentage-sizes-003.html, in Chromium it used to pass in LayoutNG but not legacy. But later it was fixed in legacy as a side effect of https://crrev.com/c/1796323
Comment 3 Oriol Brufau 2020-05-27 14:34:43 PDT
These tests have improved. I think the only remaining issue that still makes them fail is not related to grid:

    .grid {
      height: 50px;
      width: 50px;
      min-width: min-content;
      background: green;
    .item {
      writing-mode: vertical-lr;
      width: 100%;
    .content {
      width: 100px;
    There should be a green rectangle, 50px tall, 100px wide.
    <div class="grid">
      <div class="item">
        <div class="content"></div>

WebKit produces a 50x50 square instead of a rectangle.
Comment 4 Manuel Rego Casasnovas 2020-05-28 14:24:57 PDT
Thanks for the analysis Oriol.

As you said that's not related to grid layout, so you might want to report a different bug for that, and link it from TestExpectations. And then close this one.
Comment 5 Oriol Brufau 2020-05-28 16:44:21 PDT
Actually in Firefox that's also a square, so now I'm less sure that it's supposed to be a rectangle. I'm not an expert in orthogonal flows.
Comment 6 Manuel Rego Casasnovas 2020-05-28 22:33:29 PDT
I'm not sure but it looks like a bug to me, maybe we could fill a spec issue asking for clarification or fill a bug in Firefox and see what they think about it.

In all browsers "item" has a 100px width. So I guess "min-width: min-content;" on the parent should use that width, I don't see any good reason not to do that. But I might be missing something.
Comment 7 zsun 2021-04-22 02:23:55 PDT
Created attachment 426792 [details]
Comment 8 Sergio Villar Senin 2021-04-22 04:06:41 PDT
Comment on attachment 426792 [details]

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

Looking good. I've some improvements in mind though

> Source/WebCore/ChangeLog:9
> +        percentage height against. Overridden containing block size should aslo be chosen in favor


> Source/WebCore/ChangeLog:15
> +        Three tests that failed are now passed.

Nit: passed->passing

> Source/WebCore/rendering/RenderBox.cpp:3111
> +    const RenderBlock* const realContainingBlock = cb;

Blink uses overriding containing block sizes to stablish fences between LayoutNG and legacy code but that is not the case of WebKit. AFAIK only flex and grid set the overriding content logical sizes and in both cases cb would be equal to realContainingBlock because we wouldn't iterate the loop bellow that code as skipContainingBlockForPercentHeightCalculation() would return false independently on whether the item is orthogonal or quirks mode. That's why I think that we don't need the realContaininBlock variable and can directly use cb.

> Source/WebCore/rendering/RenderBox.cpp:3124

We could perhaps define at this point a variable like:

bool isOrthogonal = isHorizontal != cb->isHorizontalWritingMode();

and use it in the following checks (either isOrthogonal or !isOrthogonal depending on the case).

> LayoutTests/ChangeLog:8
> +        * TestExpectations:

Nit: just mention here that grid-item-percentage-sizes-00{1-3}.html are unskipped because they're passing now.
Comment 9 zsun 2021-04-22 06:13:02 PDT
Created attachment 426804 [details]
Comment 10 Sergio Villar Senin 2021-04-22 10:08:47 PDT
Comment on attachment 426804 [details]

Comment 11 EWS 2021-04-22 10:12:32 PDT
Committed r276445 (236905@main): <https://commits.webkit.org/236905@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426804 [details].
Comment 12 Ling Ho 2021-04-22 16:08:27 PDT