Bug 236336 – Copy sub grid track sizes from the parent grid

• Component: CSS
• Status: RESOLVED FIXED

[Matt Woodrow]

Axes that are sub gridded shouldn't do their own track sizing, they should inherit the computed track sizes directly from their parent.

[Matt Woodrow]

Created attachment 451318 [details]
Patch

[Matt Woodrow]

Created attachment 451968 [details]
Patch

[Matt Woodrow]

Created attachment 451969 [details]
Patch for EWS

[EWS Watchlist]

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

[Matt Woodrow]

Created attachment 452074 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/88991551>

[Dean Jackson]

Comment on attachment 452074 [details]
Patch

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

> Source/WebCore/rendering/GridLayoutFunctions.cpp:102
> +    return !isOrthogonalParent(grid, parent) ? direction : (direction == ForColumns ? ForRows : ForColumns);

Nit: I think this would read better as a positive test

return isOrthogonalParent ? (direction == ForColumns ? ForRows : ForColumns) : direction;

But that's a minor opinion.

[Oriol Brufau]

Comment on attachment 452074 [details]
Patch

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

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1402
> +static void removeSubgridMarginBorderPaddingFromTracks(Vector<GridTrack>& tracks, LayoutUnit mbp, bool forwards)

I don't really get the reasoning for shrinking the track sizes.
What should happen is https://drafts.csswg.org/css-grid/#subgrid-item-contribution

> The subgrid’s own grid items participate in the sizing of its in the subgridded dimension(s) and are aligned to it in those dimensions.
> In this process, the sum of the subgrid’s margin, padding, and borders at each edge are applied as an extra layer of (potentially negative) margin to the items at those edges. This extra layer of “margin” accumulates through multiple levels of subgrids.

Seems a different thing.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1432
> +    int numTracks = tracks(m_direction).size();

Maybe do

    Vector<GridTrack>& allTracks = tracks(direction);

to avoid calling tracks(m_direction) continuously.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1515
> +    // Subgrids inherit their sizing directly from the parent, so may be unrelated

But doesn't the condition hold anyways? Or maybe not, if you shrink them.

[Matt Woodrow]

(In reply to Oriol Brufau from comment #8)
> Comment on attachment 452074 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452074&action=review
> 
> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1402
> > +static void removeSubgridMarginBorderPaddingFromTracks(Vector<GridTrack>& tracks, LayoutUnit mbp, bool forwards)
> 
> I don't really get the reasoning for shrinking the track sizes.
> What should happen is
> https://drafts.csswg.org/css-grid/#subgrid-item-contribution
> 
> > The subgrid’s own grid items participate in the sizing of its in the subgridded dimension(s) and are aligned to it in those dimensions.
> > In this process, the sum of the subgrid’s margin, padding, and borders at each edge are applied as an extra layer of (potentially negative) margin to the items at those edges. This extra layer of “margin” accumulates through multiple levels of subgrids.
> 
> Seems a different thing.

That is implemented over the next two parts. First we add recursion into sub grids as part of doing sizing for the outer grid (236337) and then we add the subgrid elements margin/border/padding as extra size to those inner items (236338).

The track shrinking needs to happen since the tracks will now be sized to be big enough to hold the subgrid items as well as the M/B/P. If we just copy those sizes directly, then any 'auto' sized subgrid items would compute width/heights that cover the subgrids M/B/P area.

It appears that this is how Firefox implements the same functionality, https://searchfox.org/mozilla-central/rev/832f67743873852018eef35995c42e4dac3de4df/layout/generic/nsGridContainerFrame.cpp#3482


> 
> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1432
> > +    int numTracks = tracks(m_direction).size();
> 
> Maybe do
> 
>     Vector<GridTrack>& allTracks = tracks(direction);
> 
> to avoid calling tracks(m_direction) continuously.
> 
> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1515
> > +    // Subgrids inherit their sizing directly from the parent, so may be unrelated
> 
> But doesn't the condition hold anyways? Or maybe not, if you shrink them.

Yeah indeed, the shrinking causes this condition to fail.

[Matt Woodrow]

(In reply to Matt Woodrow from comment #9)
> 
> That is implemented over the next two parts. First we add recursion into sub
> grids as part of doing sizing for the outer grid (236337) and then we add
> the subgrid elements margin/border/padding as extra size to those inner
> items (236338).
> 
> The track shrinking needs to happen since the tracks will now be sized to be
> big enough to hold the subgrid items as well as the M/B/P. If we just copy
> those sizes directly, then any 'auto' sized subgrid items would compute
> width/heights that cover the subgrids M/B/P area.
> 
> It appears that this is how Firefox implements the same functionality,
> https://searchfox.org/mozilla-central/rev/
> 832f67743873852018eef35995c42e4dac3de4df/layout/generic/nsGridContainerFrame.
> cpp#3482
> 

I also tried an alternative approach, where I copied the track sizes directly into the child grid.

When laying out the child items inside the subgrid and calling setOverridingContainingBlockContentLogicalWidth/Height, we then needed to subtract any contributions from the subgrid's MBP from the track sizes before telling the child how much space it has. That got significantly more complicated than this approach.

[Matt Woodrow]

Created attachment 452299 [details]
Patch

[Jean-Yves Avenard [:jya]]

Comment on attachment 452299 [details]
Patch

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

> Source/WebCore/rendering/GridLayoutFunctions.h:33
> +class RenderElement;

nit: should be alphabetically ordered.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1423
> +    RenderGrid* outer = downcast<RenderGrid>(m_renderGrid->parent());

downcast is fallible; so an assertion should be added or test if it could ever be null

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1427
> +    GridSpan span = outer->gridSpanForChild(*m_renderGrid, direction);

could be defined after the test to avoid unnecessary method call.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1461
> +    if (m_renderGrid->isSubgrid(m_direction)) {

nit: this would have been nicer with a &&

> Source/WebCore/rendering/RenderGrid.cpp:366
> +        if (isSubgrid(direction)) {

to avoid multiple level of nesting, returning earlier if !isSubgrid

[Matt Woodrow]

Created attachment 452325 [details]
Patch

[EWS]

Committed r290007 (247393@main): <https://commits.webkit.org/247393@main>

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