Bug 232617 – [css-grid] Track sizing algorithm not repeated even if used flex fraction would change

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

This has caused the following WPT tests fail:

imported/w3c/web-platform-tests/css/css-grid/layout-algorithm/flex-sizing-rows-indefinite-height.html
imported/w3c/web-platform-tests/css/css-grid/layout-algorithm/grid-template-flexible-rerun-track-sizing.html

Created attachment 443080 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/85196142>

[Javier Fernandez]

Comment on attachment 443080 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443080&action=review

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1168
> +    ASSERT(!m_maxTrackBreadthIsFlexible);

nit: Perhaps a better name would be m_hasAnyFlexibleMaxTrackBreadth

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1191
> +        if (!m_maxTrackBreadthIsFlexible && indefiniteHeight) {

Maybe this deserves a comment; the reason why we are considering flex maxTrackBreadth only in the case of indefiniteHeight is not obvious to me. 

Additionally, wouldn't be clearer to use move the indefiniteHeight condition to the logic that uses the flexMaxTrackBreadk flag ?

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1197
>          if (!m_hasPercentSizedRowsIndefiniteHeight && indefiniteHeight) {

If I've understood it correctly what we are doing now for tracks with flexible values for their max-sizing functions, it's the same we need to do for the tracks with percentage values. Wouldn't make sense to do some refactoring (perhaps changing the name of the class attribute we use as flag for this scenario) and reuse all the logic behind it ? I mean, the purpose of all this is to repeat the track sizing algorithm in those situations, isn't ?

> Source/WebCore/rendering/RenderGrid.cpp:164
> +    if (m_hasAnyOrthogonalItem || m_trackSizingAlgorithm.hasAnyPercentSizedRowsIndefiniteHeight() || (m_trackSizingAlgorithm.hasFlexMaxTrackBreadth() && !m_hasAnyBaselineAlignmentItem)) {

The issue regarding the baseline alignment participation deserves a comment (perhaps also a FIXME) here. It'd be useful also to add links to the CSS WG resolutions regarding this. As reference, you can check a CL I was working on for solving this issue in blink. 

https://chromium-review.googlesource.com/c/chromium/src/+/1598815

Another interesting info about the baseline alignment participation is the FIXME comment we already have in GridTrackSizingAlgorithm::computeBaselineAlignmentContext. 


BTW, isn't this cyclic-dependency issue related to the participation of a grid item in Baseline Alignment also applies in the case of percentage sized tracks ?

Created attachment 444512 [details]
Patch

(In reply to Javier Fernandez from comment #3)
> Comment on attachment 443080 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443080&action=review
> 
> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1168
> > +    ASSERT(!m_maxTrackBreadthIsFlexible);
> 
> nit: Perhaps a better name would be m_hasAnyFlexibleMaxTrackBreadth
> 
> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1191
> > +        if (!m_maxTrackBreadthIsFlexible && indefiniteHeight) {
> 
> Maybe this deserves a comment; the reason why we are considering flex
> maxTrackBreadth only in the case of indefiniteHeight is not obvious to me. 
> 
> Additionally, wouldn't be clearer to use move the indefiniteHeight condition
> to the logic that uses the flexMaxTrackBreadk flag ?
> 
> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1197
> >          if (!m_hasPercentSizedRowsIndefiniteHeight && indefiniteHeight) {
> 
> If I've understood it correctly what we are doing now for tracks with
> flexible values for their max-sizing functions, it's the same we need to do
> for the tracks with percentage values. Wouldn't make sense to do some
> refactoring (perhaps changing the name of the class attribute we use as flag
> for this scenario) and reuse all the logic behind it ? I mean, the purpose
> of all this is to repeat the track sizing algorithm in those situations,
> isn't ?
> 
> > Source/WebCore/rendering/RenderGrid.cpp:164
> > +    if (m_hasAnyOrthogonalItem || m_trackSizingAlgorithm.hasAnyPercentSizedRowsIndefiniteHeight() || (m_trackSizingAlgorithm.hasFlexMaxTrackBreadth() && !m_hasAnyBaselineAlignmentItem)) {
> 
> The issue regarding the baseline alignment participation deserves a comment
> (perhaps also a FIXME) here. It'd be useful also to add links to the CSS WG
> resolutions regarding this. As reference, you can check a CL I was working
> on for solving this issue in blink. 
> 
> https://chromium-review.googlesource.com/c/chromium/src/+/1598815
> 
> Another interesting info about the baseline alignment participation is the
> FIXME comment we already have in
> GridTrackSizingAlgorithm::computeBaselineAlignmentContext. 
> 
> 
> BTW, isn't this cyclic-dependency issue related to the participation of a
> grid item in Baseline Alignment also applies in the case of percentage sized
> tracks ?

Thanks very much for the review comments! Patch has been updated, PTAL.

[Javier Fernandez]

Comment on attachment 444512 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444512&action=review

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:896
> +        m_isCyclicDependency = true;

First of all, caches like this are dangerous. We already have a cache to determine whether an item participates in baseline alignment and it's casing us a lot of troubles. I'd rather explore a different solution to fix this bug.

Additionally, I think the code is not correct anyway. The canParticipateInBaslineAlignment function doesn't have the purpose of detecting cycling dependencies, but just discarding them. The fact that an item (using baseline alignment, as it's assumed by the callers of this function), doesn't imply that there is a cyclic dependency. If you see the return instruction, we are evaluating whether the grid item has relative size as well.

If you want to continue with the  m_isCyclicDependency cache approach, you'd need to apply some changes in this function so that it considers the grid item's width/height as well in order to determine whether there is a cycling dependency or not.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1200
> +            if (rawTrackSize.minTrackBreadth().isPercentage() || rawTrackSize.maxTrackBreadth().isPercentage() || (rawTrackSize.maxTrackBreadth().isFlex() && !m_isCyclicDependency))

I don't understand why the baseline cyclic dependency issue only affects in the case if flex track's max-sizing function. Wouldn't we have the same problem when repeating the algorithm in case of percentage track's sizing function as well ?

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1201
> +                m_hasPercentSizedRowsOrFlexMaxTrackBreadthIndefiniteHeight = true;

I'd rather have independent flags for these 2 cases.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1371
> +    m_hasPercentSizedRowsOrFlexMaxTrackBreadthIndefiniteHeight = false;

As I said, I'd rather have independent flags here.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1442
> +    m_hasPercentSizedRowsOrFlexMaxTrackBreadthIndefiniteHeight = false;

Ditto

> Source/WebCore/rendering/GridTrackSizingAlgorithm.h:145
> +    bool hasAnyPercentSizedRowsOrFlexMaxTrackBreadthIndefiniteHeight() const { return m_hasPercentSizedRowsOrFlexMaxTrackBreadthIndefiniteHeight; }

Ditto

> Source/WebCore/rendering/GridTrackSizingAlgorithm.h:174
> +    bool canParticipateInBaselineAlignment(const RenderBox&, GridAxis);

Ditto

> Source/WebCore/rendering/GridTrackSizingAlgorithm.h:201
> +    bool m_hasPercentSizedRowsOrFlexMaxTrackBreadthIndefiniteHeight { false };

Ditto

Created attachment 444812 [details]
Patch

[Javier Fernandez]

Comment on attachment 444812 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444812&action=review

r=me 

I think the patch looks good and it's correct. Bear in mind my comments, but non of them are blockers for landing the patch.

> Source/WebCore/rendering/RenderGrid.cpp:168
> +

Nit: remove this empty line

> Source/WebCore/rendering/RenderGrid.cpp:169
> +    if (m_hasAnyOrthogonalItem || m_trackSizingAlgorithm.hasAnyPercentSizedRowsIndefiniteHeight() || (m_trackSizingAlgorithm.hasAnyFlexibleMaxTrackBreadth() && !m_hasAnyBaselineAlignmentItem) || m_hasAspectRatioBlockSizeDependentItem) {

Why we are avoiding the new run of the track sizing algorithm if there is any grid item with baseline alignment ONLY in the case of using flex max-sizing functions ? Wouldn't be possible to affect the grid item's participation in baseline alignment due to the presence of percent-sized rows and indefinite height ? 

I know that we weren't considering that case in the current code, but I'd like to understand better the effect of avoiding the algorithm repetition in that case as well (maybe we that would fix some additional test failures). We could at least add a FIXME comment and try to investigate it in a follow up patch.

> Source/WebCore/rendering/RenderGrid.cpp:207
> +        m_hasAnyBaselineAlignmentItem = false;

Perhaps we could just initialize the class field after completing the loop below, just checking out whether there is any value in the cacheBaselineAlignedItem. 

Idea: we could do the same for the m_hasAnyOrthogonalItem class field.

Created attachment 445025 [details]
Patch

(In reply to Javier Fernandez from comment #8)
> Comment on attachment 444812 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444812&action=review
> 
> r=me 
> 
> I think the patch looks good and it's correct. Bear in mind my comments, but
> non of them are blockers for landing the patch.
> 
> > Source/WebCore/rendering/RenderGrid.cpp:168
> > +
> 
> Nit: remove this empty line
> 
> > Source/WebCore/rendering/RenderGrid.cpp:169
> > +    if (m_hasAnyOrthogonalItem || m_trackSizingAlgorithm.hasAnyPercentSizedRowsIndefiniteHeight() || (m_trackSizingAlgorithm.hasAnyFlexibleMaxTrackBreadth() && !m_hasAnyBaselineAlignmentItem) || m_hasAspectRatioBlockSizeDependentItem) {
> 
> Why we are avoiding the new run of the track sizing algorithm if there is
> any grid item with baseline alignment ONLY in the case of using flex
> max-sizing functions ? Wouldn't be possible to affect the grid item's
> participation in baseline alignment due to the presence of percent-sized
> rows and indefinite height ? 
> 
> I know that we weren't considering that case in the current code, but I'd
> like to understand better the effect of avoiding the algorithm repetition in
> that case as well (maybe we that would fix some additional test failures).
> We could at least add a FIXME comment and try to investigate it in a follow
> up patch.
> 
> > Source/WebCore/rendering/RenderGrid.cpp:207
> > +        m_hasAnyBaselineAlignmentItem = false;
> 
> Perhaps we could just initialize the class field after completing the loop
> below, just checking out whether there is any value in the
> cacheBaselineAlignedItem. 
> 
> Idea: we could do the same for the m_hasAnyOrthogonalItem class field.

Checking on cacheBaselineAlignedItem, I guess we need check if m_columnBaselineItemsMap and m_rowBaselineItemsMap are empty. m_columnBaselineItemsMap and m_rowBaselineItemsMap are private in GridTrackSizingAlgorithm unless we change them to public. Maybe leave them as they are?

[Javier Fernandez]

(In reply to zsun from comment #10)
> (In reply to Javier Fernandez from comment #8)
> > Comment on attachment 444812 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=444812&action=review
> > 
> > r=me 
> > 
> > I think the patch looks good and it's correct. Bear in mind my comments, but
> > non of them are blockers for landing the patch.
> > 
> > > Source/WebCore/rendering/RenderGrid.cpp:168
> > > +
> > 
> > Nit: remove this empty line
> > 
> > > Source/WebCore/rendering/RenderGrid.cpp:169
> > > +    if (m_hasAnyOrthogonalItem || m_trackSizingAlgorithm.hasAnyPercentSizedRowsIndefiniteHeight() || (m_trackSizingAlgorithm.hasAnyFlexibleMaxTrackBreadth() && !m_hasAnyBaselineAlignmentItem) || m_hasAspectRatioBlockSizeDependentItem) {
> > 
> > Why we are avoiding the new run of the track sizing algorithm if there is
> > any grid item with baseline alignment ONLY in the case of using flex
> > max-sizing functions ? Wouldn't be possible to affect the grid item's
> > participation in baseline alignment due to the presence of percent-sized
> > rows and indefinite height ? 
> > 
> > I know that we weren't considering that case in the current code, but I'd
> > like to understand better the effect of avoiding the algorithm repetition in
> > that case as well (maybe we that would fix some additional test failures).
> > We could at least add a FIXME comment and try to investigate it in a follow
> > up patch.
> > 
> > > Source/WebCore/rendering/RenderGrid.cpp:207
> > > +        m_hasAnyBaselineAlignmentItem = false;
> > 
> > Perhaps we could just initialize the class field after completing the loop
> > below, just checking out whether there is any value in the
> > cacheBaselineAlignedItem. 
> > 
> > Idea: we could do the same for the m_hasAnyOrthogonalItem class field.
> 
> Checking on cacheBaselineAlignedItem, I guess we need check if
> m_columnBaselineItemsMap and m_rowBaselineItemsMap are empty.
> m_columnBaselineItemsMap and m_rowBaselineItemsMap are private in
> GridTrackSizingAlgorithm unless we change them to public. Maybe leave them
> as they are?

We may add a function in the GridTrackSizingAlgorithm to know whether there is any item in the baseline maps, which we could use instead of the flag in the RenderGrid class. This could have advantages, like detecting when an item changes its participation (we don't rely on a possible outdated flag). 

But I agree that change is probably unrelated to the bug we are fixing now and more appropriated for a follow up patch. Perhaps we can add FIXME comment to remember later about this idea ?

Created attachment 445031 [details]
Patch

Created attachment 445033 [details]
Patch

(In reply to Javier Fernandez from comment #11)
> (In reply to zsun from comment #10)
> > (In reply to Javier Fernandez from comment #8)
> > > Comment on attachment 444812 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=444812&action=review
> > > 
> > > r=me 
> > > 
> > > I think the patch looks good and it's correct. Bear in mind my comments, but
> > > non of them are blockers for landing the patch.
> > > 
> > > > Source/WebCore/rendering/RenderGrid.cpp:168
> > > > +
> > > 
> > > Nit: remove this empty line
> > > 
> > > > Source/WebCore/rendering/RenderGrid.cpp:169
> > > > +    if (m_hasAnyOrthogonalItem || m_trackSizingAlgorithm.hasAnyPercentSizedRowsIndefiniteHeight() || (m_trackSizingAlgorithm.hasAnyFlexibleMaxTrackBreadth() && !m_hasAnyBaselineAlignmentItem) || m_hasAspectRatioBlockSizeDependentItem) {
> > > 
> > > Why we are avoiding the new run of the track sizing algorithm if there is
> > > any grid item with baseline alignment ONLY in the case of using flex
> > > max-sizing functions ? Wouldn't be possible to affect the grid item's
> > > participation in baseline alignment due to the presence of percent-sized
> > > rows and indefinite height ? 
> > > 
> > > I know that we weren't considering that case in the current code, but I'd
> > > like to understand better the effect of avoiding the algorithm repetition in
> > > that case as well (maybe we that would fix some additional test failures).
> > > We could at least add a FIXME comment and try to investigate it in a follow
> > > up patch.
> > > 
> > > > Source/WebCore/rendering/RenderGrid.cpp:207
> > > > +        m_hasAnyBaselineAlignmentItem = false;
> > > 
> > > Perhaps we could just initialize the class field after completing the loop
> > > below, just checking out whether there is any value in the
> > > cacheBaselineAlignedItem. 
> > > 
> > > Idea: we could do the same for the m_hasAnyOrthogonalItem class field.
> > 
> > Checking on cacheBaselineAlignedItem, I guess we need check if
> > m_columnBaselineItemsMap and m_rowBaselineItemsMap are empty.
> > m_columnBaselineItemsMap and m_rowBaselineItemsMap are private in
> > GridTrackSizingAlgorithm unless we change them to public. Maybe leave them
> > as they are?
> 
> We may add a function in the GridTrackSizingAlgorithm to know whether there
> is any item in the baseline maps, which we could use instead of the flag in
> the RenderGrid class. This could have advantages, like detecting when an
> item changes its participation (we don't rely on a possible outdated flag). 
> 
> But I agree that change is probably unrelated to the bug we are fixing now
> and more appropriated for a follow up patch. Perhaps we can add FIXME
> comment to remember later about this idea ?

I have update the patch to address this. PTAL. Thank you!

[Javier Fernandez]

Comment on attachment 445033 [details]
Patch

It looks good to me, thanks !

[EWS]

Committed r286148 (244532@main): <https://commits.webkit.org/244532@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445033 [details].