Bug 238643 – Implement support for aligning baselines through subgrids

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Matt Woodrow]

We should recurse into subgrids when looking for items to contribute to a baseline sharing group.

[Radar WebKit Bug Importer]

<rdar://problem/91138101>

[Matt Woodrow]

Created attachment 456302 [details]
Patch

[EWS Watchlist]

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]

Created attachment 456795 [details]
Patch

[Matt Woodrow]

Created attachment 456856 [details]
Patch

[Javier Fernandez]

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 ?

[Matt Woodrow]

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.

[Javier Fernandez]

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)

[Matt Woodrow]

Created attachment 457591 [details]
Patch

[Matt Woodrow]

(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.

[Javier Fernandez]

Comment on attachment 457591 [details]
Patch

r=me

[EWS]

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].