Bug 209282 – [css-grid] Grid columns are wrong on successive layouts when align-items: baseline

• Component: Layout and Rendering
• Status: RESOLVED CONFIGURATION CHANGED

[Caleb Hearon]

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

[Javier Fernandez]

I'll take a look.

[Radar WebKit Bug Importer]

<rdar://problem/60662717>

[Javier Fernandez]

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.

[Javier Fernandez]

I'm working on this bug now.

[Javier Fernandez]

Created attachment 400323 [details]
Patch

[Javier Fernandez]

Created attachment 400328 [details]
Patch

[Javier Fernandez]

Created attachment 400561 [details]
Patch

[Javier Fernandez]

Created attachment 400571 [details]
Patch

[Manuel Rego Casasnovas]

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?

[Javier Fernandez]

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.

[Oriol Brufau]

(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.

[Matt Woodrow]

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.

[Matt Woodrow]

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.

[Javier Fernandez]

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.

[Ahmad Saleem]

All browsers are matching on JSFiddle (Safari Technology Preview 210, Chrome Canary 133 and Firefox Nightly 135) and also WPT shows that as well:

https://wpt.fyi/results/css/css-grid/layout-algorithm?label=master&label=experimental&aligned&q=grid-as-flex-item-should-not-shrink-to-fit

Marking this as `RESOLVED CONFIGURATION CHANGED`.