We should recurse into subgrids when looking for items to contribute to a baseline sharing group.
<rdar://problem/91138101>
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].