Summary: | Implement support for aligning baselines through subgrids | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Woodrow <mattwoodrow> | ||||||||||
Component: | Layout and Rendering | Assignee: | Matt Woodrow <mattwoodrow> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bfulgham, changseok, clopez, dino, esprehn+autocc, ews-watchlist, glenn, jfernandez, kondapallykalyan, pdr, rego, 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/33464 | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 236959 | ||||||||||||
Attachments: |
|
Description
Matt Woodrow
2022-03-31 18:03:23 PDT
Created attachment 456302 [details]
Patch
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 456795 [details]
Patch
Created attachment 456856 [details]
Patch
Comment on attachment 456856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456856&action=review The change looks good, but I'd like to understand better why some ASSERTs has been removed. > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:-923 > - ASSERT(!child.needsLayout()); Why this ASSERT is removed ? > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:-948 > - ASSERT(m_renderGrid->isBaselineAlignmentForChild(item, axis)); Why this ASSERT has been removed ? Comment on attachment 456856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456856&action=review >> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:-923 >> - ASSERT(!child.needsLayout()); > > Why this ASSERT is removed ? We only require the child to have completed layout if we are going to synthesise a baseline. I moved this assertion to two places within GridBaselineAlignment so that it only gets checked when we attempt to synthesise a baseline (and means we can align subgrid children that haven't been laid out yet, as long as they have a real baseline). >> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:-948 >> - ASSERT(m_renderGrid->isBaselineAlignmentForChild(item, axis)); > > Why this ASSERT has been removed ? m_renderGrid is the outermost grid, and isBaselineAlignmentForChild is only guaranteed to return true on the parent grid of item. I could change this to ASSERT(downcast<RenderGrid>(item.parent())->isBaselineAlignmentForChild(item, axis));, but since the only callers of this function also check that, it didn't seem necessary. Comment on attachment 456856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456856&action=review >>> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:-948 >>> - ASSERT(m_renderGrid->isBaselineAlignmentForChild(item, axis)); >> >> Why this ASSERT has been removed ? > > m_renderGrid is the outermost grid, and isBaselineAlignmentForChild is only guaranteed to return true on the parent grid of item. > > I could change this to ASSERT(downcast<RenderGrid>(item.parent())->isBaselineAlignmentForChild(item, axis));, but since the only callers of this function also check that, it didn't seem necessary. If I remember correctly, this assert was added to detect outdated elements in the cache, which might be target of a style change that could affect to the align-self or justify-self properties. Theoretically, we call cacheBaselineAlignedItem only if the items have baseline as value for theses properties, hence the ASSERT (eg, RenderGrid::computeIntrinsicLogicalWidths) Created attachment 457591 [details]
Patch
(In reply to Javier Fernandez from comment #8) > Comment on attachment 456856 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=456856&action=review > > >>> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:-948 > >>> - ASSERT(m_renderGrid->isBaselineAlignmentForChild(item, axis)); > >> > >> Why this ASSERT has been removed ? > > > > m_renderGrid is the outermost grid, and isBaselineAlignmentForChild is only guaranteed to return true on the parent grid of item. > > > > I could change this to ASSERT(downcast<RenderGrid>(item.parent())->isBaselineAlignmentForChild(item, axis));, but since the only callers of this function also check that, it didn't seem necessary. > > If I remember correctly, this assert was added to detect outdated elements > in the cache, which might be target of a style change that could affect to > the align-self or justify-self properties. Theoretically, we call > cacheBaselineAlignedItem only if the items have baseline as value for theses > properties, hence the ASSERT (eg, RenderGrid::computeIntrinsicLogicalWidths) Alright, I've added this check back. Comment on attachment 457591 [details]
Patch
r=me
Committed r292973 (249736@main): <https://commits.webkit.org/249736@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 457591 [details]. |