Bug 230252 – [css-grid] Accommodate spanning items crossing flexible tracks

• Component: CSS
• Status: RESOLVED FIXED

The specification added a step to increase sizes to accommodate spanning items crossing flexible tracks instead of ignoring their contents completely. 

https://drafts.csswg.org/css-grid/#algo-spanning-flex-items

Created attachment 438348 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 438348 [details]
Patch

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

Awesome patch, glad to see so many subtests passing now.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:230
> +        return trackSize.minTrackBreadth().isContentSized() ? LayoutUnit(infinity) : baseSize;

I don't remember the details why we were using baseSize as the growth limit for flex tracks but the specs mention that it must be always infinity in that case.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:268
> +        if (trackSize.maxTrackBreadth().isFlex())

flex cannot be indeed used as min track sizing function, but perhaps this change is unrelated and landed separatedely?

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:488
> +static double getSizeDistributionWeight(const GridTrack* track)

The track cannot be null so let's just use a reference instead of a pointer.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:532
> +    ASSERT(growthShare >= 0);

This was an ASSERT_WITH_MESSAGE(), let's keep it as it was.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:556
> +    for (uint32_t i = tracksSize; i-- > 0;) {

This looks a bit weird, I'd write it as 

for (uint32_t i = tracksSize - 1; i > 0; --i)

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:852
> +                return LayoutUnit();

I believe return { }; is preferred nowadays

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:855
> +            else

no need for the else, we have an early return above

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1233
> +    auto tacksEnd = itemsCrossingFlexibleTracks.end();    

Type tacks->tracks.

Actually we don't need these two variables, just initialize the tracksGroupRange directly with itemsCrossingFlexibleTracks

> Source/WebCore/rendering/GridTrackSizingAlgorithm.h:41
> +};

Not a big fan of the TrackSizeComputationVariant name nor the names of values.

I think we should use an enum class here something like:
enum class SpanningItemsCrossing : uint8_t { FlexibleTracks, ContentSizedTracks }

and then use them like SpanningItemsCrossing::FlexibleTracks, etc.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.h:55
> +};

Same here, let's use a enum class, and let's not repeat the Limit suffix

enum class : uint8_t SpaceDistributionLimit {
UpToGrowth,
BeyondGrowth
}

Created attachment 438476 [details]
Patch

[Oriol Brufau]

(In reply to Sergio Villar Senin from comment #2)
> I don't remember the details why we were using baseSize as the growth limit
> for flex tracks but the specs mention that it must be always infinity in
> that case.

Because base size is what the spec used to say, but then during the distribution it would use infinity.
So just using infinity from the beginning was simpler. https://github.com/w3c/csswg-drafts/issues/4313

> Same here, let's use a enum class, and let's not repeat the Limit suffix
> 
> enum class : uint8_t SpaceDistributionLimit {
> UpToGrowth,
> BeyondGrowth
> }

But it's not that we use a growth as a limit. It's that we are using the "growth limit" size as a limit. So UpToGrowth and BeyondGrowth seem confusing to me.

[Oriol Brufau]

Comment on attachment 438476 [details]
Patch

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

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:439
> +        ASSERT(variant == SpanningItemsCrossing::ContentSizedTracks || itemSpan.integerSpan() > 1u);

Should be SpanningItemsCrossing::FlexibleTracks. Items spanning flexible tracks are all handled together here. It's just non-spanning items in a non-flex track which are handled specially in another, simpler, function.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:448
> +            if (variant == SpanningItemsCrossing::ContentSizedTracks && !trackSize.maxTrackBreadth().isFlex())

Should be SpanningItemsCrossing::FlexibleTracks, right? We want to avoid distributing into non-flexible tracks when the item spans some flex track.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:490
> +    if (variant != SpanningItemsCrossing::ContentSizedTracks)

Again, SpanningItemsCrossing::FlexibleTracks? Or use ==

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:541
> +    if (variant == SpanningItemsCrossing::FlexibleTracks) {

This is items NOT crossing flexible tracks.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:856
> +            else 

Nit: maybe use `else if (allFixed)` since if it's false, we will either return minSize after the loop, or 0 if a following track is flexible. So maxBreadth won't really matter.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1232
> +        increaseSizesToAccommodateSpanningItems<SpanningItemsCrossing::FlexibleTracks>(spanGroupRange);

First the non-flexible ones.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1236
> +    increaseSizesToAccommodateSpanningItems<SpanningItemsCrossing::ContentSizedTracks>(tracksGroupRange);

Then the flexible ones.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.h:40
> +    ContentSizedTracks,

I don't like the name, flexible tracks can also be content sized.

Created attachment 438646 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/83347985>

Updated the patch and addressed review comments. Please review the updated patch. Thank you!

[Oriol Brufau]

Informal LGTM.

[Sergio Villar Senin]

Comment on attachment 438646 [details]
Patch

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

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:556
> +    for (uint32_t i = tracksSize; i-- > 0;) {

seems like you didn't change this

[Oriol Brufau]

(In reply to Sergio Villar Senin from comment #2)
> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:556
> > +    for (uint32_t i = tracksSize; i-- > 0;) {
> 
> This looks a bit weird, I'd write it as 
> 
> for (uint32_t i = tracksSize - 1; i > 0; --i)

So, we want to iterate tracksSize-1, tracksSize-2, ..., 1, 0.

for (uint32_t i = tracksSize - 1; i > 0; --i) is wrong because it doesn't include 0.

for (uint32_t i = tracksSize - 1; i >= 0; --i) is an infinite loop (-1 becomes 4294967295).

One possibility would be for (uint32_t i = tracksSize; i > 0; --i) and then use i-1 in the loop body.
But it doesn't seem great.

Maybe this is clearer?

for (uint32_t i = tracksSize; i > 0;) {
  --i;
  // ...
}

Created attachment 439605 [details]
Patch

[Sergio Villar Senin]

(In reply to Oriol Brufau from comment #11)
> (In reply to Sergio Villar Senin from comment #2)
> > > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:556
> > > +    for (uint32_t i = tracksSize; i-- > 0;) {
> > 
> > This looks a bit weird, I'd write it as 
> > 
> > for (uint32_t i = tracksSize - 1; i > 0; --i)
> 
> So, we want to iterate tracksSize-1, tracksSize-2, ..., 1, 0.
> 
> for (uint32_t i = tracksSize - 1; i > 0; --i) is wrong because it doesn't
> include 0.
> 
> for (uint32_t i = tracksSize - 1; i >= 0; --i) is an infinite loop (-1
> becomes 4294967295).
> 
> One possibility would be for (uint32_t i = tracksSize; i > 0; --i) and then
> use i-1 in the loop body.
> But it doesn't seem great.
> 
> Maybe this is clearer?
> 
> for (uint32_t i = tracksSize; i > 0;) {
>   --i;
>   // ...
> }

Right, I didn't realize tracksSize might be 0. I don't like this either because it's kind of manually managing the loop. Since those two loops are the last thing we do, I'll better do this:

if (!tracksSize)
  return;

for(uint32_t i = tracksSize - 1; i >= 0; --i)

for(uint32_t i = tracksSize - 1; i >= 0; --i)

[Oriol Brufau]

(In reply to Sergio Villar Senin from comment #13)
> Right, I didn't realize tracksSize might be 0. I don't like this either
> because it's kind of manually managing the loop. Since those two loops are
> the last thing we do, I'll better do this:
> 
> if (!tracksSize)
>   return;
> 
> for(uint32_t i = tracksSize - 1; i >= 0; --i)

No, but the problem is not just tracksSize being 0 (not sure if it's actually possible).
The problem is that `i` is signed so if it's 0, `--i` will set it to a big positive number. >= 0 always holds for unsigned variables.
Another possibility is making `i` signed.

[Oriol Brufau]

(In reply to Oriol Brufau from comment #14)
> The problem is that `i` is signed

I meant `i` is unsigned.

[Sergio Villar Senin]

(In reply to Oriol Brufau from comment #14)
> (In reply to Sergio Villar Senin from comment #13)
> > Right, I didn't realize tracksSize might be 0. I don't like this either
> > because it's kind of manually managing the loop. Since those two loops are
> > the last thing we do, I'll better do this:
> > 
> > if (!tracksSize)
> >   return;
> > 
> > for(uint32_t i = tracksSize - 1; i >= 0; --i)
> 
> No, but the problem is not just tracksSize being 0 (not sure if it's
> actually possible).
> The problem is that `i` is signed so if it's 0, `--i` will set it to a big
> positive number. >= 0 always holds for unsigned variables.
> Another possibility is making `i` signed.

Ah yeah, I think that's the way to go, let's just use int and the early return. 

Note that I haven't read that code for a while, if we can ensure that tracksSize > 0 then instead of the early return let's just use an ASSERT.

Created attachment 439716 [details]
Patch

Created attachment 439727 [details]
Patch

[Oriol Brufau]

Comment on attachment 439727 [details]
Patch

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

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:556
> +    if (!tracksSize)

If we are going to have this check, better move it to the top of the function?

Created attachment 439750 [details]
Patch

[EWS]

Committed r283439 (242426@main): <https://commits.webkit.org/242426@main>

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