RESOLVED FIXED 236336
Copy sub grid track sizes from the parent grid
https://bugs.webkit.org/show_bug.cgi?id=236336
Summary Copy sub grid track sizes from the parent grid
Matt Woodrow
Reported 2022-02-08 15:39:43 PST
Axes that are sub gridded shouldn't do their own track sizing, they should inherit the computed track sizes directly from their parent.
Attachments
Patch (10.67 KB, patch)
2022-02-08 16:10 PST, Matt Woodrow
no flags
Patch (13.49 KB, patch)
2022-02-14 17:24 PST, Matt Woodrow
no flags
Patch for EWS (95.40 KB, patch)
2022-02-14 17:26 PST, Matt Woodrow
no flags
Patch (16.39 KB, patch)
2022-02-15 12:39 PST, Matt Woodrow
no flags
Patch (16.39 KB, patch)
2022-02-16 20:24 PST, Matt Woodrow
no flags
Patch (16.45 KB, patch)
2022-02-16 23:30 PST, Matt Woodrow
no flags
Matt Woodrow
Comment 1 2022-02-08 16:10:00 PST
Matt Woodrow
Comment 2 2022-02-14 17:24:58 PST
Matt Woodrow
Comment 3 2022-02-14 17:26:07 PST
Created attachment 451969 [details] Patch for EWS
EWS Watchlist
Comment 4 2022-02-14 17:27:54 PST
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
Comment 5 2022-02-15 12:39:08 PST
Radar WebKit Bug Importer
Comment 6 2022-02-15 15:40:21 PST
Dean Jackson
Comment 7 2022-02-15 21:41:15 PST
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 8 2022-02-16 18:16:40 PST
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
Comment 9 2022-02-16 18:25:46 PST
(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
Comment 10 2022-02-16 20:18:11 PST
(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
Comment 11 2022-02-16 20:24:25 PST
Jean-Yves Avenard [:jya]
Comment 12 2022-02-16 22:42:28 PST
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
Comment 13 2022-02-16 23:30:50 PST
EWS
Comment 14 2022-02-17 01:54:33 PST
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].
Note You need to log in before you can comment on or make changes to this bug.