WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(13.49 KB, patch)
2022-02-14 17:24 PST
,
Matt Woodrow
no flags
Details
Formatted Diff
Diff
Patch for EWS
(95.40 KB, patch)
2022-02-14 17:26 PST
,
Matt Woodrow
no flags
Details
Formatted Diff
Diff
Patch
(16.39 KB, patch)
2022-02-15 12:39 PST
,
Matt Woodrow
no flags
Details
Formatted Diff
Diff
Patch
(16.39 KB, patch)
2022-02-16 20:24 PST
,
Matt Woodrow
no flags
Details
Formatted Diff
Diff
Patch
(16.45 KB, patch)
2022-02-16 23:30 PST
,
Matt Woodrow
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Matt Woodrow
Comment 1
2022-02-08 16:10:00 PST
Created
attachment 451318
[details]
Patch
Matt Woodrow
Comment 2
2022-02-14 17:24:58 PST
Created
attachment 451968
[details]
Patch
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
Created
attachment 452074
[details]
Patch
Radar WebKit Bug Importer
Comment 6
2022-02-15 15:40:21 PST
<
rdar://problem/88991551
>
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
Created
attachment 452299
[details]
Patch
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
Created
attachment 452325
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug