Bug 225860 – Fix grid aspect-ratio tests

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Rob Buis]

Fix grid-aspect-ratio-018+19.html.

[Rob Buis]

Created attachment 428816 [details]
Patch

[Javier Fernandez]

Comment on attachment 428816 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:5163
> +    if (isGridItem() && style().justifySelf().position() == ItemPosition::Stretch)

I don't think this is correct. There are several other cases that we should consider to determine the grid item uses stretch alignment:

1- container: justify-items: stretch | normal; item: justify-self: auto | normal
2- container/item combination resolved to stretch, but width non-auto.

Additionally, should we consider here the item's writing-mode, so that we check align-self property instead ?

> Source/WebCore/rendering/RenderGrid.cpp:1870
> +        if (isHorizontalWritingMode() == child.isHorizontalWritingMode() && child.style().alignSelf().position() != ItemPosition::Stretch) {

ditto

> Source/WebCore/rendering/RenderGrid.cpp:1891
> +        } else if (child.style().alignSelf().position() != ItemPosition::Stretch) {

ditto

[Radar WebKit Bug Importer]

<rdar://problem/78390550>

[Rob Buis]

Created attachment 432866 [details]
Patch

[Rob Buis]

Created attachment 433747 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Rob Buis]

Created attachment 433777 [details]
Patch

[Rob Buis]

Created attachment 433786 [details]
Patch

[Rob Buis]

Created attachment 433791 [details]
Patch

[Javier Fernandez]

Comment on attachment 433791 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:2791
> +    auto itemPosition = stretchingMode == StretchingMode::Any ? containingBlock->selfAlignmentNormalBehavior(this) : ItemPosition::Normal;

Is this variable name accurate ? Shouldn't be something like "normal behavior" ?

> Source/WebCore/rendering/RenderGrid.cpp:1140
> +    // interested in explicit stretch, not normal behavior.

No need to wrap line in this comment.

> Source/WebCore/rendering/RenderGrid.cpp:1141
> +    bool hasExplicitInlineStretch = child.style().resolvedJustifySelf(&style(), ItemPosition::Normal).position() == ItemPosition::Stretch;

Now that we have this StretchingMode enumg, wouldn't be better to use it in the alignSelfForChild to determine the proper normal-behavior ?

> Source/WebCore/rendering/RenderGrid.cpp:1142
> +    bool hasExplicitBlockStretch = child.style().resolvedAlignSelf(&style(), ItemPosition::Normal).position() == ItemPosition::Stretch;

ditto

> Source/WebCore/rendering/RenderGrid.cpp:1144
> +        std::swap(hasExplicitInlineStretch, hasExplicitBlockStretch);

Perhaps we could avoid this swatch if we use the blockFlowIsColumnAxis before, so that inline is associated to justify or align, and viceversa.

> Source/WebCore/rendering/RenderGrid.cpp:1145
> +    if (hasExplicitInlineStretch && hasExplicitBlockStretch)

Couldn't these 3 clause be simplified like this ? 
return hasExplicitInlineStretch ? hasExplicitBlockStretc : !hasExplicitBlockStretch;

> Source/WebCore/rendering/RenderGrid.cpp:1887
> +        if (isHorizontalWritingMode() == child.isHorizontalWritingMode() && child.style().alignSelf().position() != ItemPosition::Stretch) {

Why we don't need to consider align-self: auto here ?

> Source/WebCore/rendering/RenderGrid.cpp:1891
> +        } else if (child.style().justifySelf().position() != ItemPosition::Stretch) {

ditto

> Source/WebCore/rendering/RenderGrid.cpp:1903
> +        if (isHorizontalWritingMode() == child.isHorizontalWritingMode() && child.style().justifySelf().position() != ItemPosition::Stretch) {

ditto

> Source/WebCore/rendering/RenderGrid.cpp:1908
> +        } else if (child.style().alignSelf().position() != ItemPosition::Stretch) {

ditto

[Rob Buis]

Created attachment 433855 [details]
Patch

[Rob Buis]

Comment on attachment 433791 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:2791
>> +    auto itemPosition = stretchingMode == StretchingMode::Any ? containingBlock->selfAlignmentNormalBehavior(this) : ItemPosition::Normal;
> 
> Is this variable name accurate ? Shouldn't be something like "normal behavior" ?

Yes, I had to improve it too in my chromium patch, done.

>> Source/WebCore/rendering/RenderGrid.cpp:1140
>> +    // interested in explicit stretch, not normal behavior.
> 
> No need to wrap line in this comment.

I think we can remove this comment, but point taken (this happens when applying chromium patches :))

>> Source/WebCore/rendering/RenderGrid.cpp:1141
>> +    bool hasExplicitInlineStretch = child.style().resolvedJustifySelf(&style(), ItemPosition::Normal).position() == ItemPosition::Stretch;
> 
> Now that we have this StretchingMode enumg, wouldn't be better to use it in the alignSelfForChild to determine the proper normal-behavior ?

Done.

>> Source/WebCore/rendering/RenderGrid.cpp:1142
>> +    bool hasExplicitBlockStretch = child.style().resolvedAlignSelf(&style(), ItemPosition::Normal).position() == ItemPosition::Stretch;
> 
> ditto

Done.

>> Source/WebCore/rendering/RenderGrid.cpp:1144
>> +        std::swap(hasExplicitInlineStretch, hasExplicitBlockStretch);
> 
> Perhaps we could avoid this swatch if we use the blockFlowIsColumnAxis before, so that inline is associated to justify or align, and viceversa.

I think row/columns and writing-mode needs more tests which could result in removing the swap, but I prefer to do that in a future patch.

>> Source/WebCore/rendering/RenderGrid.cpp:1145
>> +    if (hasExplicitInlineStretch && hasExplicitBlockStretch)
> 
> Couldn't these 3 clause be simplified like this ? 
> return hasExplicitInlineStretch ? hasExplicitBlockStretc : !hasExplicitBlockStretch;

Looks like it can be simplified way more, now that I wrote down the combinations.

>> Source/WebCore/rendering/RenderGrid.cpp:1887
>> +        if (isHorizontalWritingMode() == child.isHorizontalWritingMode() && child.style().alignSelf().position() != ItemPosition::Stretch) {
> 
> Why we don't need to consider align-self: auto here ?

I think this one boils down to more testing needed. I left a FIXME to indicate we should work on this more.

>> Source/WebCore/rendering/RenderGrid.cpp:1891
>> +        } else if (child.style().justifySelf().position() != ItemPosition::Stretch) {
> 
> ditto

Done.

>> Source/WebCore/rendering/RenderGrid.cpp:1903
>> +        if (isHorizontalWritingMode() == child.isHorizontalWritingMode() && child.style().justifySelf().position() != ItemPosition::Stretch) {
> 
> ditto

Done.

>> Source/WebCore/rendering/RenderGrid.cpp:1908
>> +        } else if (child.style().alignSelf().position() != ItemPosition::Stretch) {
> 
> ditto

Done.

[Javier Fernandez]

Comment on attachment 433855 [details]
Patch

r=me

[EWS]

Committed r280075 (239801@main): <https://commits.webkit.org/239801@main>

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

*** Bug 227293 has been marked as a duplicate of this bug. ***