Bug 236148 – Implement getComputedStyle for sub grid

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

[Matt Woodrow]

getComputedStyle should return the actual number of explicit tracks if it's a sub grid, otherwise falling back to the specified value.

[Matt Woodrow]

Created attachment 451004 [details]
Patch

[Matt Woodrow]

Created attachment 451005 [details]
Patch for EWS

[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 451006 [details]
Patch

[Matt Woodrow]

Created attachment 451007 [details]
Patch for EWS

[Matt Woodrow]

Created attachment 451958 [details]
Patch

[Matt Woodrow]

Created attachment 451959 [details]
Patch for EWS

[Oriol Brufau]

Comment on attachment 451958 [details]
Patch

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

Looks good to me with these comments.

> Source/WebCore/ChangeLog:28
> +        Inherit track count from parent grid for subgridded axes and clamp item placement to that explicit grid.

This is from the previous patch.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:928
> +    unsigned m_autoRepeatTotalTracks;

Not counting tracks. m_autoRepeatTotalLineSets?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:929
> +    unsigned m_autoRepeatTrackListLength;

Not counting tracks. m_autoRepeatLineSetListLength?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1002
> +    if (!m_autoRepeatTotalTracks) {

I don't think this can match. If m_autoRepeatTotalTracks is 0, then we will either match `i < m_insertionPoint` or `i >= m_insertionPoint + 0` above.

> Source/WebCore/rendering/RenderGrid.cpp:498
> +    if (isRowAxis ? isSubgridColumns() : isSubgridRows())

Could be isSubgrid(direction)

[Manuel Rego Casasnovas]

Comment on attachment 451958 [details]
Patch

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

We miss the ChangeLog for the LayoutTest changes.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:907
> +        , m_totalTracks(totalTracksCount)

Could we call this m_totalLines and store it with the +1 already? It looks we're always using it adding +1.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1092
> +    // an appropriate grid parent?), then we fall back to using the specified value.

Why a question in the comment?

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:82
> +    // TODO: Handle subgrid auto repeat tracks

Nit: s/TODO/FIXME/

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:83
> +    if (isRowAxis ? gridContainerStyle.gridSubgridColumns() : gridContainerStyle.gridSubgridRows()) {

Could we use isSubgrid() here?

> LayoutTests/TestExpectations:-1450
> -imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-subgridded-axis-auto-repeater-crash-002.html [ Crash ]

Aren't we passing any other test after this change?

[Manuel Rego Casasnovas]

Comment on attachment 451958 [details]
Patch

This patch looks good too.

[Matt Woodrow]

Created attachment 452235 [details]
Patch

[Matt Woodrow]

Created attachment 452236 [details]
Patch

[Matt Woodrow]

Created attachment 452237 [details]
Patch for EWS

[Matt Woodrow]

(In reply to Manuel Rego Casasnovas from comment #9)
> 
> > LayoutTests/TestExpectations:-1450
> > -imported/w3c/web-platform-tests/css/css-grid/subgrid/grid-subgridded-axis-auto-repeater-crash-002.html [ Crash ]
> 
> Aren't we passing any other test after this change?

Unfortunately not! A lot of tests check getComputedStyle, but as part of a ref test. This change gets them closer to being correct, but the grids still render incorrectly.

[Oriol Brufau]

Comment on attachment 452236 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:929
> +    unsigned m_totalLines;

Can you stick to either "line" or "line set" terminology?
Technically speaking it's called "line name set" https://www.w3.org/TR/css-grid-2/#line-name-set
Both "line" and "line set" are probably understandable, but should be consistent.

[Matt Woodrow]

(In reply to Oriol Brufau from comment #15)
> Comment on attachment 452236 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452236&action=review
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:929
> > +    unsigned m_totalLines;
> 
> Can you stick to either "line" or "line set" terminology?
> Technically speaking it's called "line name set"
> https://www.w3.org/TR/css-grid-2/#line-name-set
> Both "line" and "line set" are probably understandable, but should be
> consistent.

m_totalLines is the actual number of lines in the grid (initialised used Grid::numTracks + 1), and isn't a count of line name sets coming from style properties.

[EWS]

Committed r289993 (247379@main): <https://commits.webkit.org/247379@main>

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