Created attachment 393981 [details]
Safari grid as flex item with baseline can have wrong column widths
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;
Could be related to Bug #199648
I'll take a look.
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.
I'm working on this bug now.
Created attachment 400323 [details]
Created attachment 400328 [details]
Created attachment 400561 [details]
Created attachment 400571 [details]
Comment on attachment 400571 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=400571&action=review
> + There are already Web Platform Tests covering this change, which a
Nit: I think "which" is not needed in this sentence.
> + // 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.
> + 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?
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.
(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.