Summary: | Lookup line names for sub grids using line names from ancestor grids | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Woodrow <mattwoodrow> | ||||||||||||||||||||
Component: | CSS | Assignee: | 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
Matt Woodrow
2022-02-07 22:25:52 PST
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
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]. |