Bug 231802

Summary: [css-grid] update the content-sized grid width before laying out a grid item with block constraints and aspect-ratio
Product: WebKit Reporter: zsun
Component: CSSAssignee: zsun
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, clopez, esprehn+autocc, ews-watchlist, fred.wang, glenn, jfernandez, kondapallykalyan, pdr, rego, svillar, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch none

Description zsun 2021-10-15 02:53:38 PDT
We need to relayout grid items when the grid width changes due to the change on item size.
Comment 1 zsun 2021-10-15 03:33:12 PDT
Created attachment 441359 [details]
Patch
Comment 2 Sergio Villar Senin 2021-10-15 04:33:04 PDT
Comment on attachment 441359 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:974
> +            child->layoutIfNeeded();

We shouldn't layout the item twice. If you need to recompute the preferred width then just do it before the child->layoutIfNeeded() call above and do the layout just once.
Comment 3 zsun 2021-10-15 06:38:45 PDT
Recompute the preferred with before laying out a grid item with block constraints
Comment 4 zsun 2021-10-15 06:52:10 PDT
Created attachment 441373 [details]
Patch
Comment 5 Sergio Villar Senin 2021-10-15 07:13:11 PDT
Comment on attachment 441373 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:972
> +            updateLogicalWidth();

The part I don't understand is that you mention that you need to recompute the preferred width of the item but you call updateLogicalWidth() in the grid. This means that we're recomputing the width of the grid in the middle of the the layout of the items. That's problematic, note that some items might be laid out with the old grid width while some others will use the updated one.
Comment 6 Radar WebKit Bug Importer 2021-10-22 02:54:16 PDT
<rdar://problem/84542687>
Comment 7 Frédéric Wang (:fredw) 2021-10-28 07:00:19 PDT
Comment on attachment 441373 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        IOS platform. However, we noticed that they also fail for GTK at

iOS*

> Source/WebCore/rendering/RenderGrid.cpp:970
> +        // We need to recompute the preferred width for gird items with block constraints.

grid*
Comment 8 zsun 2021-11-05 08:10:11 PDT
Created attachment 443394 [details]
Patch
Comment 9 zsun 2021-11-05 08:51:29 PDT
Created attachment 443397 [details]
Patch
Comment 10 Javier Fernandez 2021-11-08 01:05:15 PST
Comment on attachment 443397 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:282
> +            updateLogicalWidth();

I don't understand why this solves the issue. I thought we needed to repeat the track sizing algorithm; don't we ?

Additionally, wouldn't we need to call setPreferredLogicalWidthsDirty for the grid container so that we run the track sizing algorithm again to adjust its intrinsic size ?
Comment 11 zsun 2021-11-08 02:41:08 PST
Created attachment 443532 [details]
Patch
Comment 12 zsun 2021-11-08 07:51:55 PST
Created attachment 443546 [details]
Patch
Comment 13 Javier Fernandez 2021-11-08 09:49:19 PST
Comment on attachment 443546 [details]
Patch

The change looks good, but I think we should add the new test that we discussed today; the one to verify that we update the tracks after applying the aspect ratio.
Comment 14 zsun 2021-11-08 11:42:18 PST
Created attachment 443579 [details]
Patch
Comment 15 EWS Watchlist 2021-11-08 11:43:04 PST
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
Comment 16 Javier Fernandez 2021-11-09 00:59:39 PST
Comment on attachment 443579 [details]
Patch

r=me

Please, ensure the new test is exported to the WPT repository.
Comment 17 EWS 2021-11-09 06:48:10 PST
Committed r285497 (244021@main): <https://commits.webkit.org/244021@main>

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