Bug 227573 – [CSS-grid] Ignore the aspect-ratio of a replaced element if stretch alignments are applied to both axes

• Component: CSS
• Status: RESOLVED FIXED

Affected tests -

imported/w3c/web-platform-tests/css/css-grid/alignment/replaced-alignment-with-aspect-ratio-004.html
imported/w3c/web-platform-tests/css/css-grid/alignment/replaced-alignment-with-aspect-ratio-005.html

Created attachment 432671 [details]
Patch

[Oriol Brufau]

Informal LGTM.

[Javier Fernandez]

Comment on attachment 432671 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        This CL only fixed the case when both axes have stretch alignments applied and there is no auto margin(s)

Perhaps it'd be better to add our own description of the change.

> Source/WebCore/rendering/RenderBox.cpp:2635
> +    } else if (shouldComputeLogicalWidthFromAspectRatio() && style().logicalWidth().isAuto() && (!isGridItem() || !hasStretchedLogicalWidth() || !hasStretchedLogicalHeight())) {

I'm not sure this is the right approach. I know in Chromium there are these clauses as well to added the case of grid items with stretched size in any size to the ones that should ignore aspect ratio. However, in Chromium we also tried to do it in shouldComputeLogicalWidthFromAspectRatio, which is called in other functions of this class and seems a more appropriated candidate for this logic. Had you consider that approach already  ?

Created attachment 432925 [details]
Patch

Created attachment 432930 [details]
Patch

[Javier Fernandez]

Comment on attachment 432930 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:2609
> +    treatAsReplaced = treatAsReplaced && (!isGridItem() || (!hasStretchedLogicalWidth() && !shouldComputeLogicalWidthFromAspectRatio()));

I still have doubts about this change; sorry, because I suggested this approach in my last review, but I think we need to study it a bit more before we can land it. 

First of all, aren't hasStretchedLogicalWidth and shouldComputeLogicalWidthFromAspectRatio clauses redundant ? I mean, inside the second one, we are already checking out whether we are using 'stretch' as the value for the justify-self property.

As a matter of fact, the current implementation of shouldComputeLogicalWidthFromAspectRatio regarding how we are checking the justify-self property for 'stretch' is not totally correct. We should also consider the case of justify-items: stretch | normal in the containing block, when this RenderBox instance's  justify-self property has 'auto' for its value. This is precisely why hasStretchedLogicalWidth() uses style.resolvedJustifySelf to determine whether the size is being stretched or not. 

Finally, and this is why I asked sorry in the first place, I think we are mixing now the concepts of 'treatAsReplaced' vs 'compute-size-from-aspect-ratio'. I assumed in my last review that they were equivalent, but looking a the code (some lines after this) it seems we have different branches for these 2 concepts.

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

I think we should use resolveAlingSelf instead (as I commented before)

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

I think we should use resolveJustifySelf instead.

> Source/WebCore/rendering/RenderGrid.cpp:1886
> +        if (isHorizontalWritingMode() == child.isHorizontalWritingMode() && child.style().justifySelf().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/80311333>

Created attachment 433129 [details]
Patch

Created attachment 433576 [details]
Patch

[Javier Fernandez]

Comment on attachment 433576 [details]
Patch

r=me

[EWS]

Committed r280022 (239764@main): <https://commits.webkit.org/239764@main>

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