Subgrid inherit lines names from their parent grid on lines that they share, we need line name resolution to handle this.
Created attachment 451211 [details] Patch
Created attachment 451212 [details] Patch for EWS
Created attachment 451213 [details] Patch for NEWs
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 451280 [details] Patch
Created attachment 451284 [details] Patch
Created attachment 451961 [details] Patch
Created attachment 451962 [details] Patch for EWS
<rdar://problem/88947825>
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.
Created attachment 452283 [details] Patch
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
Created attachment 452298 [details] Patch
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].