Axes that are sub gridded shouldn't do their own track sizing, they should inherit the computed track sizes directly from their parent.
Created attachment 451318 [details] Patch
Created attachment 451968 [details] Patch
Created attachment 451969 [details] Patch for EWS
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
Created attachment 452074 [details] Patch
<rdar://problem/88991551>
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.
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.
(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.
(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.
Created attachment 452299 [details] Patch
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
Created attachment 452325 [details] Patch
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].