Bug 236287

Summary: Lookup line names for sub grids using line names from ancestor grids
Product: WebKit Reporter: Matt Woodrow <mattwoodrow>
Component: CSSAssignee: Matt Woodrow <mattwoodrow>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, changseok, clopez, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jean-yves.avenard, jfernandez, koivisto, kondapallykalyan, macpherson, menard, obrufau, pdr, rego, ryuan.choi, sergio, simon.fraser, svillar, webkit-bug-importer, youennf, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://github.com/web-platform-tests/wpt/pull/32849
Bug Depends on: 236148    
Bug Blocks: 202115, 236336    
Attachments:
Description Flags
Patch
none
Patch for EWS
none
Patch for NEWs
none
Patch
none
Patch
none
Patch
none
Patch for EWS
none
Patch
none
Patch none

Matt Woodrow
Reported 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.
Attachments
Patch (38.67 KB, patch)
2022-02-07 22:44 PST, Matt Woodrow
no flags
Patch for EWS (159.62 KB, patch)
2022-02-07 22:45 PST, Matt Woodrow
no flags
Patch for NEWs (159.11 KB, patch)
2022-02-07 23:06 PST, Matt Woodrow
no flags
Patch (88.96 KB, patch)
2022-02-08 11:20 PST, Matt Woodrow
no flags
Patch (38.74 KB, patch)
2022-02-08 11:29 PST, Matt Woodrow
no flags
Patch (39.78 KB, patch)
2022-02-14 17:02 PST, Matt Woodrow
no flags
Patch for EWS (85.35 KB, patch)
2022-02-14 17:03 PST, Matt Woodrow
no flags
Patch (41.14 KB, patch)
2022-02-16 18:02 PST, Matt Woodrow
no flags
Patch (40.81 KB, patch)
2022-02-16 20:20 PST, Matt Woodrow
no flags
Matt Woodrow
Comment 1 2022-02-07 22:44:13 PST
Matt Woodrow
Comment 2 2022-02-07 22:45:21 PST
Created attachment 451212 [details] Patch for EWS
Matt Woodrow
Comment 3 2022-02-07 23:06:23 PST
Created attachment 451213 [details] Patch for NEWs
EWS Watchlist
Comment 4 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
Matt Woodrow
Comment 5 2022-02-08 11:20:44 PST
Matt Woodrow
Comment 6 2022-02-08 11:29:15 PST
Matt Woodrow
Comment 7 2022-02-14 17:02:10 PST
Matt Woodrow
Comment 8 2022-02-14 17:03:33 PST
Created attachment 451962 [details] Patch for EWS
Radar WebKit Bug Importer
Comment 9 2022-02-14 22:26:18 PST
Simon Fraser (smfr)
Comment 10 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.
Matt Woodrow
Comment 11 2022-02-16 18:02:58 PST
Oriol Brufau
Comment 12 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
Matt Woodrow
Comment 13 2022-02-16 20:20:07 PST
EWS
Comment 14 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].
Note You need to log in before you can comment on or make changes to this bug.