Bug 209282 - [css-grid] Grid columns are wrong on successive layouts when align-items: baseline
Summary: [css-grid] Grid columns are wrong on successive layouts when align-items: bas...
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Javier Fernandez
URL:
Keywords: InRadar
Depends on: 212920
Blocks:
  Show dependency treegraph
 
Reported: 2020-03-19 08:58 PDT by Caleb Hearon
Modified: 2022-03-17 15:29 PDT (History)
15 users (show)

See Also:


Attachments
Safari grid as flex item with baseline can have wrong column widths (563 bytes, text/html)
2020-03-19 08:58 PDT, Caleb Hearon
no flags Details
Patch (32.21 KB, patch)
2020-05-27 05:12 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (32.19 KB, patch)
2020-05-27 06:33 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (7.91 KB, patch)
2020-05-29 02:27 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (8.00 KB, patch)
2020-05-29 05:06 PDT, Javier Fernandez
jfernandez: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caleb Hearon 2020-03-19 08:58:58 PDT
Created attachment 393981 [details]
Safari grid as flex item with baseline can have wrong column widths

TL;DR: https://jsfiddle.net/e6fxnoac/1/

I have finally been able to isolate this grid layout bug in WebKit that has now bitten us 3 different times in our web application. It goes back several Safari versions.

Firefox and Chrome do not have this problem so it's easy to forget and have websites look bad in Safari.

The bug is that the column widths are wrong (too narrow). What causes it:

1. The grid is a flex item
2. The outer flexbox flows in the block direction (flex-flow: column)
3. The grid specifies align-items: baseline;
4. The element is appended to the DOM in Javascript rather than loaded from the initial document

Could be related to Bug #199648
Comment 1 Javier Fernandez 2020-03-19 14:45:14 PDT
I'll take a look.
Comment 2 Radar WebKit Bug Importer 2020-03-19 17:47:53 PDT
<rdar://problem/60662717>
Comment 3 Javier Fernandez 2020-03-20 06:23:26 PDT
It seems the root cause of the issue is a call to WebCore::RenderGrid::computeIntrinsicLogicalWidths made by the Flexible Box when computing the crossAxisIntrinsicExtentForChild. 

In the Grid Layout intrinsic size computation logic, we need to perform a layout of a grid item that participates in baseline alignment before the grid track sizes are actually computed. This causes the item to compute incorrectly its intrinsic size. 

In Blink, we do this only for grid items that have a relative size, which will indeed depend on the grid area, eventual computed. We can follow a similar approach, and I'm already working on a patch; still trying to investigate a few layout tests failures, though.
Comment 4 Javier Fernandez 2020-05-27 03:28:20 PDT
I'm working on this bug now.
Comment 5 Javier Fernandez 2020-05-27 05:12:36 PDT
Created attachment 400323 [details]
Patch
Comment 6 Javier Fernandez 2020-05-27 06:33:02 PDT
Created attachment 400328 [details]
Patch
Comment 7 Javier Fernandez 2020-05-29 02:27:37 PDT
Created attachment 400561 [details]
Patch
Comment 8 Javier Fernandez 2020-05-29 05:06:44 PDT
Created attachment 400571 [details]
Patch
Comment 9 Manuel Rego Casasnovas 2020-06-02 04:44:54 PDT
Comment on attachment 400571 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        There are already Web Platform Tests covering this change, which a

Nit: I think "which" is not needed in this sentence.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1044
> +    // This code is executed only when computing the grid's intrinsic
> +    // width based on an orthogonal child. We rely on the pre-layout
> +    // performed in LayoutGrid::LayoutOrthogonalWritingModeRoots.

Nit: Comments don't need to be wrapped at 80 chars in WebKit.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1063
> +    return child.logicalHeight() + GridLayoutFunctions::marginLogicalSizeForChild(*renderGrid(), childBlockDirection, child);

So the minContentForChild() can be bigger than the maxContentForChild(). You're not adding m_algorithm.baselineOffsetForChild() here, is that right?
Comment 10 Javier Fernandez 2020-06-10 07:43:16 PDT
It seems that the issue described in this bug is not reproducible any more. 

The reason is that we landed some changes in #r262716 that affected how flexbox managed the flex item's preferred sizes and the shrink-to-fit logic. 

I really think the root cause of the bug is still in the grid layout code. The problem is that as part of the grid's intrinsic size computation logic, we perform a pre-layout of the grid items participating in baseline alignment. Additionally, the intrinsic size computation currently perform's layouts of the grid items.  

If a container asks for the max/min preferred width of a children that happens to be a grid element, and this grid element has been already laid our, we may end up with grid items with a wrong size, due to these layouts mentioned before.

I'm ok with either keeping the bug and adding new test cases to reproduce the issue if we find them, or close it and file a new one whenever the issue appears again.
Comment 11 Oriol Brufau 2020-06-10 08:05:07 PDT
(In reply to Javier Fernandez from comment #10)
> If a container asks for the max/min preferred width of a children that
> happens to be a grid element, and this grid element has been already laid
> our, we may end up with grid items with a wrong size, due to these layouts
> mentioned before.

Maybe you could add a unit test which manually asks for the max/min preferred width, just like flexbox used to do.

Even if the bug is not noticeable right now, it seems to me that fixing it can avoid problems in the future.
Comment 12 Matt Woodrow 2022-03-13 18:50:48 PDT
It looks like bug 232140 added a new path so that this reproduces with nested grids.

This got reported again as bug 237732 and bug 237710.
Comment 13 Matt Woodrow 2022-03-13 19:00:21 PDT
Comment on attachment 400571 [details]
Patch

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

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1059
> +    // performed in LayoutGrid::LayoutOrthogonalWritingModeRoots.

The constraint here appears to be that 'IndefiniteSizeStrategy + ForColumns' is unique to intrinsic size calculations, and thus we shouldn't be mutating 'child' (unless the RenderGrid needsLayout, in which case we can, since it'll get fixed again later).

That seems really hard to keep track of through the sizing algorithm code, and ensure we don't accidentally break this again in the future.

Could we just return cached values from RenderGrid::computeIntrinsicLogicalWidths when !needsLayout(), and not need these changes?

I guess alternatively the sizing algorithm/strategy would only get const references to children when doing intrinsic size calculations, but that seems like a huge change to template everything.
Comment 14 Javier Fernandez 2022-03-17 15:29:47 PDT
Comment on attachment 400571 [details]
Patch

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

>> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1059
>> +    // performed in LayoutGrid::LayoutOrthogonalWritingModeRoots.
> 
> The constraint here appears to be that 'IndefiniteSizeStrategy + ForColumns' is unique to intrinsic size calculations, and thus we shouldn't be mutating 'child' (unless the RenderGrid needsLayout, in which case we can, since it'll get fixed again later).
> 
> That seems really hard to keep track of through the sizing algorithm code, and ensure we don't accidentally break this again in the future.
> 
> Could we just return cached values from RenderGrid::computeIntrinsicLogicalWidths when !needsLayout(), and not need these changes?
> 
> I guess alternatively the sizing algorithm/strategy would only get const references to children when doing intrinsic size calculations, but that seems like a huge change to template everything.

> Could we just return cached values from RenderGrid::computeIntrinsicLogicalWidths when !needsLayout(), and not need these changes?

Yes, you're right. We need to ensure we do this only in case the grid container needs layout. We should return the cached values otherwise, as you suggests.