Bug 223570 – [css-contain] Support contain:size

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

[cathiechen]

Support contain:size;

[Radar WebKit Bug Importer]

<rdar://problem/75956816>

[cathiechen]

Created attachment 424901 [details]
size-containment-WIP

[cathiechen]

Created attachment 424903 [details]
WIP-patch-for-size-containment

[cathiechen]

Created attachment 425756 [details]
Patch

[Rob Buis]

Comment on attachment 425756 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        This patch brings initial support of CSS contain:size. According to https://www.w3.org/TR/css-contain-2/#containment-size,

Probably better to refer to contain-1, could also be a footnote ([1]). In general these lines should be kept short IIUC.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1152
> +    if (!m_strategy->isComputingSizeContainment()) {

The strategy stuff probably needs explaining in the Changelog.

> Source/WebCore/rendering/RenderBlock.cpp:1078
> +        downcast<RenderBlockFlow>(*this).adjustSizeContainmentChildForPagination(r, r.logicalTop());

The pagination probably needs explaining in the Changelog.

> Source/WebCore/rendering/RenderBox.cpp:2880
> +        auto borderAndPading = RenderBox::borderBefore() + RenderBox::paddingBefore() + RenderBox::borderAfter() + RenderBox::paddingAfter();

There is borderAndPaddingBefore and borderAndPaddingAfter. *borderAndPadding*

> Source/WebCore/rendering/RenderMultiColumnSet.cpp:240
> +    if (columnCount() <= computedColumnCount() /*&& m_minSpaceShortage <= 0*/) {

For the final patch we should have no code in comments.

> Source/WebCore/rendering/RenderMultiColumnSet.cpp:245
> +    if (forcedBreaksCount() >= computedColumnCount() /*&& m_minSpaceShortage <= 0*/) {

Ditto.

> Source/WebCore/rendering/RenderMultiColumnSet.cpp:257
> +        return m_computedColumnHeight + sizeContainmentShortage; // So bail out rather than looping infinitely.

The sizeContainmentShortage probably also needs ChangeLog explanation.

[cathiechen]

Comment on attachment 425756 [details]
Patch

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

Hi Rob,
Thanks for the review!

>> Source/WebCore/ChangeLog:8
>> +        This patch brings initial support of CSS contain:size. According to https://www.w3.org/TR/css-contain-2/#containment-size,
> 
> Probably better to refer to contain-1, could also be a footnote ([1]). In general these lines should be kept short IIUC.

Done, thanks!

>> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1152
>> +    if (!m_strategy->isComputingSizeContainment()) {
> 
> The strategy stuff probably needs explaining in the Changelog.

Yeah, will explain in the latest version:)

>> Source/WebCore/rendering/RenderBlock.cpp:1078
>> +        downcast<RenderBlockFlow>(*this).adjustSizeContainmentChildForPagination(r, r.logicalTop());
> 
> The pagination probably needs explaining in the Changelog.

Ditto.

>> Source/WebCore/rendering/RenderBox.cpp:2880
>> +        auto borderAndPading = RenderBox::borderBefore() + RenderBox::paddingBefore() + RenderBox::borderAfter() + RenderBox::paddingAfter();
> 
> There is borderAndPaddingBefore and borderAndPaddingAfter. *borderAndPadding*

There is a slight difference for fieldset. borderAndPaddingBefore add extra intrinsicBorderForFieldset for fieldset. but size containment doesn't need that.
I will add a comment in the new patch.

>> Source/WebCore/rendering/RenderMultiColumnSet.cpp:240
>> +    if (columnCount() <= computedColumnCount() /*&& m_minSpaceShortage <= 0*/) {
> 
> For the final patch we should have no code in comments.

Ah, done! Thanks!

>> Source/WebCore/rendering/RenderMultiColumnSet.cpp:257
>> +        return m_computedColumnHeight + sizeContainmentShortage; // So bail out rather than looping infinitely.
> 
> The sizeContainmentShortage probably also needs ChangeLog explanation.

Done!

[cathiechen]

Created attachment 425987 [details]
Patch

[cathiechen]

Comment on attachment 425987 [details]
Patch

Hi,
I think this patch is ready for review. Thanks!

[Darin Adler]

Looks good to me, but I think it should be reviewed by someone who’s been working in this area recently.

[cathiechen]

Created attachment 426439 [details]
Patch

[cathiechen]

(In reply to Darin Adler from comment #9)
> Looks good to me, but I think it should be reviewed by someone who’s been
> working in this area recently.

Hi Darin,
Thanks! I added Simon to the cc list:)

The new patch is now rebased on the code which supports contain:layout.
Luckily we have 3 more tests passed in this patch.
  imported/w3c/web-platform-tests/css/css-contain/contain-animation-001.html
  imported/w3c/web-platform-tests/css/css-contain/contain-layout-size-003.html
  imported/w3c/web-platform-tests/css/css-contain/contain-strict-003.html

[cathiechen]

Created attachment 426537 [details]
Patch

[cathiechen]

Created attachment 426543 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 426543 [details]
Patch

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

Nice work. I've mostly focused of the parts I know the most like grid.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1186
>      for (auto trackIndex : m_contentSizedTracksIndex) {

So, if I understood this correctly, this loop is the only thing you have to do for the case of computing content:size. It that's the case then I'd do the following:
1- move the loop into a lambda inside this method.
2- at the beginning of the method do something like
if (m_strategy->isComputingSizeContainment()) {
   execTheCallback();
   return;
}

That would allow you to keep the method almost untouched (except for the lambda) and there would be no need to change indentation and all that.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1247
> +            m_sizingState = ColumnSizingSecondIteration;

I am not sure I understand this change. I guess you basically compute content:size as if nothing had happened, so the first iteration of row computing could happen. My question is, why not adding a new state to the state machine? Something that would happen after RowSizingFirstIteration?

> Source/WebCore/rendering/RenderBox.cpp:2880
> +        // borderAndPaddingLogicalHeight contains intrinsicBorderForFieldset() which add extra border for fieldset, so we can't use it directly.

Are you sure this is correct? I don't see any intrinsicBorderForFieldset() in the computation of borderAndPaddingLogicalHeight()

> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:211
> +            }

Same comment here as the method in grid. I'd create a lambda with the code bellow that just assigns the maxLogicalWidth and then adds the scrollbarwith and then at the beginning of the method
if (shouldApplySizeContainment(*this)) {
   callLambda();
   return;
}
.... body of the method untouched ...
callLambda()

> Source/WebCore/rendering/RenderFlexibleBox.cpp:141
> +        }

Ditto for this method. If the only thing you have to do is to add the scrollbarWidth to the min and max for the case of size containment, then move it to a lambda and
if (shouldApplySizeContainment()) {
 lambda();
 return;
}
... body of the method untouched ...
lambda()

> Source/WebCore/rendering/RenderGrid.cpp:467
> +        return 0;

I am not sure I understand this change. Auto-fit is basically like auto-fill except that empty columns collapse. The fact that they collapse do not mean that they don't exist. But here we're saying that there are no auto-fit tracks. Maybe I am missing something?

[cathiechen]

Comment on attachment 426543 [details]
Patch

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

Hi Sergio,
Thanks for the review!

>> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1186
>>      for (auto trackIndex : m_contentSizedTracksIndex) {
> 
> So, if I understood this correctly, this loop is the only thing you have to do for the case of computing content:size. It that's the case then I'd do the following:
> 1- move the loop into a lambda inside this method.
> 2- at the beginning of the method do something like
> if (m_strategy->isComputingSizeContainment()) {
>    execTheCallback();
>    return;
> }
> 
> That would allow you to keep the method almost untouched (except for the lambda) and there would be no need to change indentation and all that.

Done! Yeah, a lambda function makes it more readable, the code is put to resolveIntrinsicTrackSizesFromChildren().
`sizeTrackToFitNonSpanningItem(span, *gridItem, track);` changes track sizes, so we also need to put this loop into the lambda.
And we need to expand tack.growthLimit() to track.baseSize() if tack.growthLimit() is infinity which is handled at the end of this function, so instead of retrun early, it will be changed to 
    if (!m_strategy->isComputingSizeContainment())
        resolveIntrinsicTrackSizesFromChildren();

>> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1247
>> +            m_sizingState = ColumnSizingSecondIteration;
> 
> I am not sure I understand this change. I guess you basically compute content:size as if nothing had happened, so the first iteration of row computing could happen. My question is, why not adding a new state to the state machine? Something that would happen after RowSizingFirstIteration?

Yes, in RenderGrid::layoutBlock it computes track height twice if shouldApplySizeContainment(*this).
  * computeTrackSizesForIndefiniteSize(m_trackSizingAlgorithm, ForRows); To compute trackBasedLogicalHeight as if there's no content. trackBasedLogicalHeight is used in the second time.
  * computeTrackSizesForDefiniteSize(ForRows, trackBasedLogicalHeight); Using trackBasedLogicalHeight computed by the first time to distribute the track sizes.
So the state stays in RowSizingFirstIteration twice.
I will add a new state RowSizingExtraIterationForSizeContainment to indicate that is the extra try for size containment.

>> Source/WebCore/rendering/RenderBox.cpp:2880
>> +        // borderAndPaddingLogicalHeight contains intrinsicBorderForFieldset() which add extra border for fieldset, so we can't use it directly.
> 
> Are you sure this is correct? I don't see any intrinsicBorderForFieldset() in the computation of borderAndPaddingLogicalHeight()

Yes, borderAndPaddingLogicalHeight calls borderBefore() and borderAfter(). In RenderBlock::borderBefore(), it adds intrinsicBorderForFieldset().
I'll point out it is specific to RenderBlock in the comment.

>> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:211
>> +            }
> 
> Same comment here as the method in grid. I'd create a lambda with the code bellow that just assigns the maxLogicalWidth and then adds the scrollbarwith and then at the beginning of the method
> if (shouldApplySizeContainment(*this)) {
>    callLambda();
>    return;
> }
> .... body of the method untouched ...
> callLambda()

Done! Put the code to computeIntrinsicLogicalWidthsFromChildren.
And also we need to handle the scrollbar width at the end of function, the code is now looked like this:
    if (!shouldApplySizeContainment(*this))
        computeIntrinsicLogicalWidthsFromChildren();

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:141
>> +        }
> 
> Ditto for this method. If the only thing you have to do is to add the scrollbarWidth to the min and max for the case of size containment, then move it to a lambda and
> if (shouldApplySizeContainment()) {
>  lambda();
>  return;
> }
> ... body of the method untouched ...
> lambda()

Done! Create a lambda function computeIntrinsicLogicalWidthsFromChildren. And it looks like this now:
    if (!shouldApplySizeContainment(*this))
        computeIntrinsicLogicalWidthsFromChildren();

>> Source/WebCore/rendering/RenderGrid.cpp:467
>> +        return 0;
> 
> I am not sure I understand this change. Auto-fit is basically like auto-fill except that empty columns collapse. The fact that they collapse do not mean that they don't exist. But here we're saying that there are no auto-fit tracks. Maybe I am missing something?

Hmmm, make sense. I think I should check size containment in computeEmptyTracksForAutoRepeat() instead.

[cathiechen]

Created attachment 427668 [details]
Patch

[cathiechen]

Created attachment 427740 [details]
Patch

[cathiechen]

(In reply to cathiechen from comment #15)
> Comment on attachment 426543 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426543&action=review
> 
> Hi Sergio,
> Thanks for the review!
> 
> >> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1186
> >>      for (auto trackIndex : m_contentSizedTracksIndex) {
> > 
> > So, if I understood this correctly, this loop is the only thing you have to do for the case of computing content:size. It that's the case then I'd do the following:
> > 1- move the loop into a lambda inside this method.
> > 2- at the beginning of the method do something like
> > if (m_strategy->isComputingSizeContainment()) {
> >    execTheCallback();
> >    return;
> > }
> > 
> > That would allow you to keep the method almost untouched (except for the lambda) and there would be no need to change indentation and all that.
> 
> Done! Yeah, a lambda function makes it more readable, the code is put to
> resolveIntrinsicTrackSizesFromChildren().
> `sizeTrackToFitNonSpanningItem(span, *gridItem, track);` changes track
> sizes, so we also need to put this loop into the lambda.
> And we need to expand tack.growthLimit() to track.baseSize() if
> tack.growthLimit() is infinity which is handled at the end of this function,
> so instead of retrun early, it will be changed to 
>     if (!m_strategy->isComputingSizeContainment())
>         resolveIntrinsicTrackSizesFromChildren();
> 

Sorry, I misunderstood this! The new patch puts the code handle infinity growthLimit into a lambda instead:)

[Sergio Villar Senin]

Comment on attachment 427740 [details]
Patch

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

I've reviewed the grid and flex parts and they both look great to me now.

> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:190
> +    auto handleScrollbarWidth = [&]() {

Nit: addScrollbarWidth ?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:86
> +    auto handleScrollbarWidth = [&]() {

Nit: maybe addScrollbarWidth?

[cathiechen]

Comment on attachment 427740 [details]
Patch

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

>> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:190
>> +    auto handleScrollbarWidth = [&]() {
> 
> Nit: addScrollbarWidth ?

Done, thanks!

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:86
>> +    auto handleScrollbarWidth = [&]() {
> 
> Nit: maybe addScrollbarWidth?

Done!

[cathiechen]

Created attachment 427744 [details]
Patch

[zalan]

Comment on attachment 427744 [details]
Patch

This is one of those "hard to review" patches. it's really difficult to tell if it covers all the cases or whether it does not cause any unexpected states in the renderers. I even added "contain: size" to LFC to have a better understanding but it did not really help. Guess we'll just need to deal with the potential fallout from this.

[Rob Buis]

Comment on attachment 427744 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:2865
> +        auto borderAndPading = RenderBox::borderBefore() + RenderBox::paddingBefore() + RenderBox::borderAfter() + RenderBox::paddingAfter();

Can use borderAndPaddingBefore/borderAndPaddingAfter here.

[cathiechen]

Created attachment 428256 [details]
Patch

[cathiechen]

(In reply to Rob Buis from comment #23)
> Comment on attachment 427744 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427744&action=review
> 
> > Source/WebCore/rendering/RenderBox.cpp:2865
> > +        auto borderAndPading = RenderBox::borderBefore() + RenderBox::paddingBefore() + RenderBox::borderAfter() + RenderBox::paddingAfter();
> 
> Can use borderAndPaddingBefore/borderAndPaddingAfter here.

Thanks, Rob!

The borderAndPadding* interfaces eventually call RenderBlock::borderBefore which addes intrinsicBorderForFieldset(). I've added more detailed explanation in the comments.

[cathiechen]

(In reply to zalan from comment #22)
> Comment on attachment 427744 [details]
> Patch
> 
> This is one of those "hard to review" patches. it's really difficult to tell
> if it covers all the cases or whether it does not cause any unexpected
> states in the renderers. I even added "contain: size" to LFC to have a
> better understanding but it did not really help. Guess we'll just need to
> deal with the potential fallout from this.

Hi zalan,

Thanks a lot for doing that!
I think I can keep eyes on the follow-up bugs if any.

[EWS]

Zalan Bujtas found in /Volumes/Data/worker/Commit-Queue/build/LayoutTests/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/worker/Commit-Queue/build/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[cathiechen]

Created attachment 428264 [details]
Patch

[EWS]

Committed r277321 (237580@main): <https://commits.webkit.org/237580@main>

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