[css-grid] WPT Tests css/css-grid/grid-items/grid-item-percentage-sizes-*.html fail

[Carlos Alberto Lopez Perez]

The following ref-test failures happen on WebKit for the WPT tests:


css/css-grid/grid-items/grid-item-percentage-sizes-001.html
css/css-grid/grid-items/grid-item-percentage-sizes-002.html
css/css-grid/grid-items/grid-item-percentage-sizes-003.html

Check here the diff of the images:

https://wpt.fyi/results/css/css-grid/grid-items?label=master&product=chrome%5Bexperimental%5D&product=edge%5Bexperimental%5D&product=firefox%5Bexperimental%5D&product=safari%5Bexperimental%5D&product=webkitgtk%5Bexperimental%5D&aligned&q=css%2Fcss-grid%2Fgrid-items%2Fgrid-item-percentage-sizes
^ click on the test name, then on compare on either safari or webkitgtk and a new window will appear

[Carlos Alberto Lopez Perez]

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 ?

[Oriol Brufau]

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

[Oriol Brufau]

These tests have improved. I think the only remaining issue that still makes them fail is not related to grid:

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

WebKit produces a 50x50 square instead of a rectangle.

[Manuel Rego Casasnovas]

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.

[Oriol Brufau]

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.

[Manuel Rego Casasnovas]

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.

Created attachment 426792 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 426792 [details]
Patch

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

Nits:
overriden->overriding
aslo->also

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

Created attachment 426804 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 426804 [details]
Patch

Super!

[EWS]

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

[Ling Ho]

<rdar://77026533>