Bug 227573 - [CSS-grid] Ignore the aspect-ratio of a replaced element if stretch alignments are applied to both axes
Summary: [CSS-grid] Ignore the aspect-ratio of a replaced element if stretch alignment...
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zsun
Keywords: InRadar
Depends on:
Reported: 2021-07-01 01:16 PDT by zsun
Modified: 2021-07-19 01:39 PDT (History)
12 users (show)

See Also:

Patch (6.27 KB, patch)
2021-07-01 02:39 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (10.31 KB, patch)
2021-07-06 07:25 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (10.58 KB, patch)
2021-07-06 07:54 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (11.42 KB, patch)
2021-07-08 06:23 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (5.96 KB, patch)
2021-07-15 05:28 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 zsun 2021-07-01 01:16:19 PDT
Affected tests -

Comment 1 zsun 2021-07-01 02:39:35 PDT
Created attachment 432671 [details]
Comment 2 Oriol Brufau 2021-07-01 07:02:43 PDT
Informal LGTM.
Comment 3 Javier Fernandez 2021-07-05 06:42:39 PDT
Comment on attachment 432671 [details]

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  ?
Comment 4 zsun 2021-07-06 07:25:12 PDT
Created attachment 432925 [details]
Comment 5 zsun 2021-07-06 07:54:32 PDT
Created attachment 432930 [details]
Comment 6 Javier Fernandez 2021-07-07 15:31:13 PDT
Comment on attachment 432930 [details]

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) {


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

Comment 7 Radar WebKit Bug Importer 2021-07-08 01:17:17 PDT
Comment 8 zsun 2021-07-08 06:23:07 PDT
Created attachment 433129 [details]
Comment 9 zsun 2021-07-15 05:28:17 PDT
Created attachment 433576 [details]
Comment 10 Javier Fernandez 2021-07-17 02:57:46 PDT
Comment on attachment 433576 [details]

Comment 11 EWS 2021-07-19 01:39:08 PDT
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].