[Webkit-unassigned] [Bug 236054] Add support for parsing 'subgrid' in grid-template-columns/row

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=236054

--- Comment #10 from Oriol Brufau <obrufau at igalia.com> ---
Comment on attachment 450826
  --> https://bugs.webkit.org/attachment.cgi?id=450826
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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20220207/a4c43ecd/attachment.htm>