Bug 236287 – Lookup line names for sub grids using line names from ancestor grids

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

[Matt Woodrow]

Subgrid inherit lines names from their parent grid on lines that they share, we need line name resolution to handle this.

[Matt Woodrow]

Created attachment 451211 [details]
Patch

[Matt Woodrow]

Created attachment 451212 [details]
Patch for EWS

[Matt Woodrow]

Created attachment 451213 [details]
Patch for NEWs

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

Created attachment 451280 [details]
Patch

[Matt Woodrow]

Created attachment 451284 [details]
Patch

[Matt Woodrow]

Created attachment 451961 [details]
Patch

[Matt Woodrow]

Created attachment 451962 [details]
Patch for EWS

[Radar WebKit Bug Importer]

<rdar://problem/88947825>

[Simon Fraser (smfr)]

Comment on attachment 451961 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451961&action=review

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:74
> +    if (side == ColumnStartSide)
> +        return RowStartSide;
> +    if (side == ColumnEndSide)
> +        return RowEndSide;
> +    if (side == RowStartSide)
> +        return ColumnStartSide;
> +    return ColumnEndSide;

switch?

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:80
> +    NamedGridAreaMap::const_iterator gridAreaIt = areas.find(name);

auto

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:83
> +        const GridArea& gridArea = gridAreaIt->value;
> +        GridSpan gridSpan = isRowAxis ? gridArea.columns : gridArea.rows;

auto

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:88
> +    return -1;

Rather than returning -1 on failure, use a std::optional<int> return value

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:95
> +    GridTrackSizingDirection direction = directionFromSide(side);

auto

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:97
> +    const RenderGrid* grid = &initialGrid;
> +    const RenderStyle* gridContainerStyle = &grid->style();

auto

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:105
> +    const NamedGridLinesMap& gridLineNames = isRowAxis ? gridContainerStyle->namedGridColumnLines() : gridContainerStyle->namedGridRowLines();
> +    const NamedGridLinesMap& autoRepeatGridLineNames = isRowAxis ? gridContainerStyle->autoRepeatNamedGridColumnLines() : gridContainerStyle->autoRepeatNamedGridRowLines();
> +    const NamedGridLinesMap& implicitGridLineNames = isRowAxis ? gridContainerStyle->implicitNamedGridColumnLines() : gridContainerStyle->implicitNamedGridRowLines();
> +

auto

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:133
> +        // subgrid might have inherited less tracks than needed to cover the specified area. We want to clamp

fewer tracks

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:174
> +void NamedLineCollectionBase::ensureLocalStorage()

"local storage" has a different meaning on the web. Maybe ensureNamedIndices() or something.

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:176
> +    if (m_implicitNamedLinesIndexes != &m_inheritedNamedLinesIndexes) {

nit: indexes -> indices

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:202
> +        unsigned autoRepeatIndexInFirstRepetition = (line - m_insertionPoint) % m_autoRepeatTrackListLength;

Can m_autoRepeatTrackListLength be 0?

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:230
> +    GridSpan search = GridSpan::translatedDefiniteGridSpan(0, m_lastLine);
> +    GridPositionSide currentSide = side;
> +    GridTrackSizingDirection direction = directionFromSide(currentSide);

auto

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:237
> +        const RenderGrid* parent = downcast<RenderGrid>(grid->parent());

auto

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:247
> +        GridSpan span = parent->gridSpanForChild(*grid, direction);

auto

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:253
> +            i -= search.startLine();
> +            if (GridLayoutFunctions::isFlippedDirection(*parent, direction) != initialFlipped)
> +                i = m_lastLine - i;

Hopefully these can't underflow?

> Source/WebCore/rendering/style/GridPositionsResolver.h:68
> +    bool m_subgrid;

This should have "is" or "has" in the name. Auto initialize it here: bool m_isSubgrid { false }. Same for the other members.

> Source/WebCore/rendering/style/GridPositionsResolver.h:81
> +    unsigned lastLine() const;

Maybe it's clear that a "line" here is really a "line index"?
I'm not sure what a "position" is in this context.

[Matt Woodrow]

Created attachment 452283 [details]
Patch

[Oriol Brufau]

Comment on attachment 452283 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452283&action=review

> Source/WebCore/rendering/RenderGrid.cpp:776
> +

Newline not needed

> Source/WebCore/rendering/style/GridArea.h:169
> +    void clamp(int min, int max)

Please add ASSERT(min < max);

> Source/WebCore/rendering/style/GridArea.h:175
> +            m_startLine = m_endLine;

GridSpan has this assert: ASSERT(startLine < endLine);
Your code can break that.
And why assign the start line to the end one and not e.g. the opposite?

Maybe you could instead return a bool indicating success:

    bool clamp(int min, int max)
    {
        ASSERT(min < max);
        ASSERT(m_startLine < m_endLine);
        ASSERT(m_type != Indefinite);
        if (min >= m_endLine || max <= m_startLine)
          return false;
        m_startLine = std::max(m_startLine, min);
        m_endLine = std::min(m_endLine, max);
        ASSERT(m_startLine < m_endLine);
        return true;
    }

And then:

        if (gridSpan.clamp(min, max))
            return isStartSide ? gridSpan.startLine() : gridSpan.endLine();

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:122
> +            // value of the style should be the initial 'none' values.

Typo: value

[Matt Woodrow]

Created attachment 452298 [details]
Patch

[EWS]

Committed r289998 (247384@main): <https://commits.webkit.org/247384@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452298 [details].