Bug 236054 – Add support for parsing 'subgrid' in grid-template-columns/row

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Matt Woodrow]

The first step for subgrid is to add it to the parser, and making sure we can serialise it back again.

[Matt Woodrow]

Created attachment 450731 [details]
Patch

[Oriol Brufau]

I think you should add css/CSSSubgridValue.h into Headers.cmake for WebKitGTK.

[Darin Adler]

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.

[Darin Adler]

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”

[Matt Woodrow]

(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.

[Matt Woodrow]

Created attachment 450807 [details]
Patch

[Darin Adler]

(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.

[Matt Woodrow]

Created attachment 450826 [details]
Patch

[Matt Woodrow]

(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.

[Oriol Brufau]

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?

[Matt Woodrow]

Created attachment 451141 [details]
Patch

[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]

(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!

[Matt Woodrow]

Created attachment 451158 [details]
Patch

[Oriol Brufau]

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.

[Matt Woodrow]

Created attachment 451192 [details]
Patch

[Matt Woodrow]

(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.

[Oriol Brufau]

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.

[Matt Woodrow]

(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.

[Matt Woodrow]

Created attachment 451281 [details]
Patch

[Oriol Brufau]

(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.

[Manuel Rego Casasnovas]

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?

[Manuel Rego Casasnovas]

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.

[Matt Woodrow]

(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.

[Matt Woodrow]

(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.

[Matt Woodrow]

Created attachment 451645 [details]
Patch for landing

[EWS]

[e-mail] does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json.

Rejecting attachment 451645 [details] from commit queue.

[EWS]

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

[Matt Woodrow]

Created attachment 451647 [details]
Patch for landing

[EWS]

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].