Bug 236287 - Lookup line names for sub grids using line names from ancestor grids
Summary: Lookup line names for sub grids using line names from ancestor grids
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Matt Woodrow
URL:
Keywords: InRadar
Depends on: 236148
Blocks: 202115 236336
  Show dependency treegraph
 
Reported: 2022-02-07 22:25 PST by Matt Woodrow
Modified: 2022-02-16 22:25 PST (History)
23 users (show)

See Also:


Attachments
Patch (38.67 KB, patch)
2022-02-07 22:44 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch for EWS (159.62 KB, patch)
2022-02-07 22:45 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch for NEWs (159.11 KB, patch)
2022-02-07 23:06 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (88.96 KB, patch)
2022-02-08 11:20 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (38.74 KB, patch)
2022-02-08 11:29 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (39.78 KB, patch)
2022-02-14 17:02 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch for EWS (85.35 KB, patch)
2022-02-14 17:03 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (41.14 KB, patch)
2022-02-16 18:02 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (40.81 KB, patch)
2022-02-16 20:20 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Woodrow 2022-02-07 22:25:52 PST
Subgrid inherit lines names from their parent grid on lines that they share, we need line name resolution to handle this.
Comment 1 Matt Woodrow 2022-02-07 22:44:13 PST
Created attachment 451211 [details]
Patch
Comment 2 Matt Woodrow 2022-02-07 22:45:21 PST
Created attachment 451212 [details]
Patch for EWS
Comment 3 Matt Woodrow 2022-02-07 23:06:23 PST
Created attachment 451213 [details]
Patch for NEWs
Comment 4 EWS Watchlist 2022-02-07 23:08:04 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
Comment 5 Matt Woodrow 2022-02-08 11:20:44 PST
Created attachment 451280 [details]
Patch
Comment 6 Matt Woodrow 2022-02-08 11:29:15 PST
Created attachment 451284 [details]
Patch
Comment 7 Matt Woodrow 2022-02-14 17:02:10 PST
Created attachment 451961 [details]
Patch
Comment 8 Matt Woodrow 2022-02-14 17:03:33 PST
Created attachment 451962 [details]
Patch for EWS
Comment 9 Radar WebKit Bug Importer 2022-02-14 22:26:18 PST
<rdar://problem/88947825>
Comment 10 Simon Fraser (smfr) 2022-02-16 17:22:31 PST
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.
Comment 11 Matt Woodrow 2022-02-16 18:02:58 PST
Created attachment 452283 [details]
Patch
Comment 12 Oriol Brufau 2022-02-16 18:20:53 PST
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
Comment 13 Matt Woodrow 2022-02-16 20:20:07 PST
Created attachment 452298 [details]
Patch
Comment 14 EWS 2022-02-16 22:25:28 PST
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].