getComputedStyle should return the actual number of explicit tracks if it's a sub grid, otherwise falling back to the specified value.
Created attachment 451004 [details] Patch
Created attachment 451005 [details] Patch for EWS
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
Created attachment 451006 [details] Patch
Created attachment 451007 [details] Patch for EWS
Created attachment 451958 [details] Patch
Created attachment 451959 [details] Patch for EWS
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)
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?
Comment on attachment 451958 [details] Patch This patch looks good too.
Created attachment 452235 [details] Patch
Created attachment 452236 [details] Patch
Created attachment 452237 [details] Patch for EWS
(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.
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.
(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.
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].