The first step for subgrid is to add it to the parser, and making sure we can serialise it back again.
Created attachment 450731 [details] Patch
I think you should add css/CSSSubgridValue.h into Headers.cmake for WebKitGTK.
Comment on attachment 450731 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450731&action=review Need a rebased patch that works in EWS > Source/WebCore/css/parser/CSSPropertyParser.cpp:3685 > + RefPtr<CSSGridLineNamesValue> lineNames = consumeGridLineNames(args, nullptr, true); auto > Source/WebCore/css/parser/CSSPropertyParser.cpp:3707 > + RefPtr<CSSValueList> values = CSSSubgridValue::create(); auto > Source/WebCore/css/parser/CSSPropertyParser.cpp:3712 > + } else if (RefPtr<CSSValue> value = consumeGridLineNames(range, nullptr, true)) auto > Source/WebCore/rendering/style/RenderStyle.h:1775 > + static bool initialGridSubgrid() { return false; } Why go the trouble of using a function here. It only share one for rows and columns? > Source/WebCore/rendering/style/StyleGridData.h:106 > + bool subgridRows; Can we just initialize to false here instead of using the cumbersome RenderStyle approach? > Source/WebCore/style/StyleBuilderConverter.h:1007 > + bool m_isSubgrid { false }; This whole struct uses incorrect coding style. Public struct members should not have m_ prefixes.
Comment on attachment 450731 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450731&action=review >> Source/WebCore/rendering/style/RenderStyle.h:1775 >> + static bool initialGridSubgrid() { return false; } > > Why go the trouble of using a function here. It only share one for rows and columns? Meant “but only share one”
(In reply to Darin Adler from comment #3) > > Source/WebCore/rendering/style/RenderStyle.h:1775 > > + static bool initialGridSubgrid() { return false; } > > Why go the trouble of using a function here. It only share one for rows and > columns? > > > Source/WebCore/rendering/style/StyleGridData.h:106 > > + bool subgridRows; > > Can we just initialize to false here instead of using the cumbersome > RenderStyle approach? It looks like we have one of these for every initial value, even though they're all only used in one place. I did the same for this just to keep in sync with the surrounding code, but I don't have any attachment to doing it this way. > > > Source/WebCore/style/StyleBuilderConverter.h:1007 > > + bool m_isSubgrid { false }; > > This whole struct uses incorrect coding style. Public struct members should > not have m_ prefixes. Do you want me to fix all the members here, or just make the new one correct for now? I just matched the surrounding style.
Created attachment 450807 [details] Patch
(In reply to Matt Woodrow from comment #5) > It looks like we have one of these for every initial value, even though > they're all only used in one place. That’s not right. The first one I checked was initialStrokeMiterLimit, and I found it was used in four places, importantly one of those places was in generated code. I now realize that the primary reason these exist are for that generated code, in StyleBuilderGenerated.cpp, generated from CSSProperties.json by the makeprop script. > I did the same for this just to keep in sync with the surrounding code, but > I don't have any attachment to doing it this way. I think it’s possible you misunderstood the existing pattern. I think the new function in this patch doesn’t match the naming pattern well enough to work for the generated code. I think we should only follow that pattern if this new code is participating in the CSSProperties.json mechanism. > Do you want me to fix all the members here, or just make the new one correct > for now? I just matched the surrounding style. I would suggest making the new one correct even though that is inconsistent. And as a contribution to the health of the project, follow through afterward with a patch to fix the rest.
Created attachment 450826 [details] Patch
(In reply to Darin Adler from comment #7) > (In reply to Matt Woodrow from comment #5) > > It looks like we have one of these for every initial value, even though > > they're all only used in one place. > > That’s not right. The first one I checked was initialStrokeMiterLimit, and I > found it was used in four places, importantly one of those places was in > generated code. I now realize that the primary reason these exist are for > that generated code, in StyleBuilderGenerated.cpp, generated from > CSSProperties.json by the makeprop script. > That's fair, I was just looking at the surrounding grid properties. I've changed this to the simpler thing, and I'll come back to cleaning up some of this code soon.
Comment on attachment 450826 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450826&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1001 > +static void populateSubgridTrackList(CSSValueList& list, OrderedNamedLinesCollector& collector, int start, int end) Not really a track list, rather a line name list. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1058 > + OrderedNamedLinesCollectorInsideRepeat repeatCollector(style, isRowAxis); Nit: to avoid checking isSubgrid continuosuly, maybe handle that case separately? if (isSubgrid) { OrderedNamedLinesCollectorInsideRepeat repeatCollector(style, isRowAxis); if (!repeatCollector.namedGridLineCount()) { populateSubgridTrackList(list.get(), collector); return list; } // Add the line names and track sizes that precede the auto repeat(). int autoRepeatInsertionPoint = isRowAxis ? style.gridAutoRepeatColumnsInsertionPoint() : style.gridAutoRepeatRowsInsertionPoint(); populateSubgridTrackList(list.get(), collector, 0, autoRepeatInsertionPoint); // Add a CSSGridAutoRepeatValue with the contents of the auto repeat(). ASSERT((isRowAxis ? style.gridAutoRepeatColumnsType() : style.gridAutoRepeatRowsType()) == AutoRepeatType::Fill); auto repeatedValues = CSSGridAutoRepeatValue::create(CSSValueAutoFill); populateSubgridTrackList(repeatedValues.get(), repeatCollector); list->append(repeatedValues.get()); // Add the line names and track sizes that follow the auto repeat(). populateSubgridTrackList(list.get(), collector, autoRepeatInsertionPoint, collector.namedGridLineCount() + 1); return list; } No strong opinion. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1059 > + if (isSubgrid ? !repeatCollector.namedGridLineCount() : autoRepeatTrackSizes.isEmpty()) { I don't think !repeatCollector.namedGridLineCount() is the right condition. `subgrid repeat(auto-fill, [])` has 0 lines but should not be handled here. And if there is no WPT for this, please add one. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1073 > + autoRepeatInsertionPoint = std::clamp<int>(autoRepeatInsertionPoint, 0, trackSizes.size()); Not sure if clamping was really necessary here. But if it was, maybe subgrid needs it too? > Source/WebCore/css/parser/CSSPropertyParser.cpp:3722 > + if (!consumeGridNameRepeatFunction(range, values)) This is allowing multiple auto repeats: CSS.supports("grid-template-columns", "subgrid repeat(auto-fill, [a]) repeat(auto-fill, [b])"); // true But https://www.w3.org/TR/css-grid-2/#auto-repeat > On a subgridded axis, the auto-fill keyword is only valid once per <line-name-list> And if there is no WPT for this, please add one. > Source/WebCore/style/StyleBuilderConverter.h:1039 > ASSERT(tracksData.m_autoRepeatTrackSizes.isEmpty()); This assert was enough to check that there would be a single auto-repeat. That's no longer the case with subgrid, can you add another assert?
Created attachment 451141 [details] Patch
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
(In reply to Oriol Brufau from comment #10) > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1059 > > + if (isSubgrid ? !repeatCollector.namedGridLineCount() : autoRepeatTrackSizes.isEmpty()) { > > I don't think !repeatCollector.namedGridLineCount() is the right condition. > `subgrid repeat(auto-fill, [])` has 0 lines but should not be handled here. > And if there is no WPT for this, please add one. I think this would count as having one line, but zero actual names assigned to that line. I've added a WPT to check this, and the duplicated auto repeat case. Thanks for the review!
Created attachment 451158 [details] Patch
Comment on attachment 451158 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451158&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1060 > + if (!repeatCollector.namedGridLineCount()) { I still think this condition is a bit loose. document.body.style.gridTemplateRows = "subgrid repeat(auto-fill, [])"; getComputedStyle(document.body).gridTemplateRows; // Firefox: "subgrid repeat(auto-fill, [])" // WebKit: "subgrid" > Source/WebCore/css/parser/CSSPropertyParser.cpp:3702 > + while (!args.atEnd()) { This could be a do...while to avoid repeating the code. > Source/WebCore/rendering/style/StyleGridData.h:107 > + bool subgridColumns; May be done in another patch, but I guess that GridTemplateTracksWrapper in CSSPropertyAnimation.cpp should check these.
Created attachment 451192 [details] Patch
(In reply to Oriol Brufau from comment #15) > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1060 > > + if (!repeatCollector.namedGridLineCount()) { > > I still think this condition is a bit loose. > > document.body.style.gridTemplateRows = "subgrid repeat(auto-fill, [])"; > getComputedStyle(document.body).gridTemplateRows; > // Firefox: "subgrid repeat(auto-fill, [])" > // WebKit: "subgrid" Good catch, the test I added only checked serialised style, not computed. I've added a few new tests (all variations of [] were broken). > > Source/WebCore/rendering/style/StyleGridData.h:107 > > + bool subgridColumns; > > May be done in another patch, but I guess that GridTemplateTracksWrapper in > CSSPropertyAnimation.cpp should check these. Indeed, I haven't tried to handle animation in this patch.
Comment on attachment 451192 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451192&action=review Other than these comments, informal r+ (I'm not a reviewer). > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1077 > + populateSubgridLineNameList(list.get(), collector, autoRepeatInsertionPoint + 1, collector.namedGridLineCount() + 1); The +1 seem wrong, a workaround for StyleBuilderConverter increasing the index after an auto-repeat. I would rather fix the StyleBuilderConverter and here use autoRepeatInsertionPoint = std::clamp<int>(autoRepeatInsertionPoint, 0, collector.namedGridLineCount()); populateSubgridLineNameList(list.get(), collector, 0, autoRepeatInsertionPoint); // ... populateSubgridLineNameList(list.get(), collector, autoRepeatInsertionPoint, collector.namedGridLineCount()); > Source/WebCore/style/StyleBuilderConverter.h:1028 > + currentNamedGridLine++; Some lines below, tracksData.m_autoRepeatInsertionPoint = currentNamedGridLine++; should be tracksData.m_autoRepeatInsertionPoint = currentNamedGridLine; if (!tracksData.isSubgrid) currentNamedGridLine++; No need to increase since it's already done here.
(In reply to Oriol Brufau from comment #18) > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1077 > > + populateSubgridLineNameList(list.get(), collector, autoRepeatInsertionPoint + 1, collector.namedGridLineCount() + 1); > > The +1 seem wrong, a workaround for StyleBuilderConverter increasing the > index after an auto-repeat. > I would rather fix the StyleBuilderConverter and here use > > autoRepeatInsertionPoint = > std::clamp<int>(autoRepeatInsertionPoint, 0, collector.namedGridLineCount()); > populateSubgridLineNameList(list.get(), collector, 0, > autoRepeatInsertionPoint); > // ... > populateSubgridLineNameList(list.get(), collector, > autoRepeatInsertionPoint, collector.namedGridLineCount()); > > > Source/WebCore/style/StyleBuilderConverter.h:1028 > > + currentNamedGridLine++; > > Some lines below, > > tracksData.m_autoRepeatInsertionPoint = currentNamedGridLine++; > > should be > > tracksData.m_autoRepeatInsertionPoint = currentNamedGridLine; > if (!tracksData.isSubgrid) > currentNamedGridLine++; > > No need to increase since it's already done here. Do you know why the non-sub grid code also does the same +1 after an auto-repeat? I left it the same to match (and it means the code in CSSComputedStyleDeclaration/NamedLineCollection gets to be more similar), though I agree that it's probably better just to not bother. I'll update this patch to not need it, but it might be nice to change the normal grid code at some point too.
Created attachment 451281 [details] Patch
(In reply to Matt Woodrow from comment #19) > Do you know why the non-sub grid code also does the same +1 after an > auto-repeat? > > I left it the same to match (and it means the code in > CSSComputedStyleDeclaration/NamedLineCollection gets to be more similar), > though I agree that it's probably better just to not bother. > > I'll update this patch to not need it, but it might be nice to change the > normal grid code at some point too. The non-subgrid code increases currentNamedGridLine at every track size. Then, it's necessary to also increase after an auto-repeat because in grid-template-columns: [a] repeat(auto-fill, 1px) [b] we want [a] and [b] to have different indexes, even if there is no track size between them (outside the auto-repeat, which is stored separately). But with subgrid, we are increasing at every line name list. Then, grid-template-columns: subgrid [a] repeat(auto-fill, [c]) [b] the [a] and [b] already have different indices. I don't think the subgrid and non-subgrid cases can be made much more consistent since line names have different behaviors around repeats: grid-template-columns: [a] repeat(2, [b] 1px [c]) [d]; /* same as: */ grid-template-columns: [a b] 1px [c b] 1px [c d]; grid-template-columns: subgrid [a] repeat(2, [b] [c]) [d]; /* same as: */ grid-template-columns: subgrid [a] [b] [c] [b] [c] [d]; In the non-subgrid case, adjacent name lists are merged, that's why we count track sizes instead. In the subgrid case, adjacent name lists are not merged and we don't have track sizes, so we count name lists.
Comment on attachment 451281 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451281&action=review Thanks for working on this, everyone is waiting for subgrid support. Also thanks for adding more WPT test cases. I've just left a few minor comments. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1040 > + // TODO: We need to handle computed subgrid here. NIt: In WebKit it's more common FIXME than TODO. > Source/WebCore/css/parser/CSSPropertyParser.cpp:3679 > +static bool consumeGridNameRepeatFunction(CSSParserTokenRange& range, CSSValueList& list, bool& isAutoRepeat) If I got it correctly, this is only used for subgrid, should we change the name so it's more clear that it's only for that? > LayoutTests/TestExpectations:1470 > +imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-subgridded-axis-auto-repeater-crash-001.html [ Crash ] > +imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-subgridded-axis-auto-repeater-crash-002.html [ Crash ] I guess we do want to fix these crashes ASAP, do you have a bug report to track them?
One more thing, not sure if this is addressed on this patch or it'll happen in a follow-up one, but this part from the spec (https://drafts.csswg.org/css-grid-2/#subgrid-listing): > If there is no parent grid, or if the grid container is otherwised forced to establish an independent formatting context (for example, due to layout containment [CSS-CONTAIN-2]), the used value is the initial value, none, and the grid container is not a subgrid. I haven't find a test checking that, maybe I didn't look in the right place, or we should add a new test to cover that case.
(In reply to Manuel Rego Casasnovas from comment #23) > One more thing, not sure if this is addressed on this patch or it'll happen > in a follow-up one, but this part from the spec > (https://drafts.csswg.org/css-grid-2/#subgrid-listing): > > If there is no parent grid, or if the grid container is otherwised forced to establish an independent formatting context (for example, due to layout containment [CSS-CONTAIN-2]), the used value is the initial value, none, and the grid container is not a subgrid. > > I haven't find a test checking that, maybe I didn't look in the right place, > or we should add a new test to cover that case. I'm not sure there is one! There's a test that checks that the computed style is handled correctly for grid's using 'sub grid' without a grid parent, but not when we do have a grid parent, but also contain:layout. I don't think we can detect that at this point in the patch queue, I can add a test to one of the later patches for it.
(In reply to Manuel Rego Casasnovas from comment #22) > > > LayoutTests/TestExpectations:1470 > > +imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-subgridded-axis-auto-repeater-crash-001.html [ Crash ] > > +imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-subgridded-axis-auto-repeater-crash-002.html [ Crash ] > > I guess we do want to fix these crashes ASAP, do you have a bug report to > track them? Not a specific bug, but they do get fixed in the part that handles line name resolution for sub grids.
Created attachment 451645 [details] Patch for landing
mattwoodrow@apple.com does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json. Rejecting attachment 451645 [details] from commit queue.
ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.
Created attachment 451647 [details] Patch for landing
Committed r289722 (247207@main): <https://commits.webkit.org/247207@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451647 [details].