Implement css property contain-intrinsic-size

[cathiechen]

Implement css property contain-intrinsic-size

[cathiechen]

Created attachment 456818 [details]
WIP-patch

[Radar WebKit Bug Importer]

<rdar://problem/91688651>

[cathiechen]

Created attachment 457691 [details]
WIP-patch-for-ews

[cathiechen]

Created attachment 457692 [details]
WIP-patch-for-review

[cathiechen]

Created attachment 457693 [details]
WIP-patch-for-ews

[cathiechen]

Created attachment 457694 [details]
WIP-patch-for-review

[cathiechen]

Created attachment 457699 [details]
WIP-patch-for-ews

[cathiechen]

Created attachment 457700 [details]
WIP-patch-for-review

[cathiechen]

Created attachment 457701 [details]
WIP-patch-for-ews

[cathiechen]

Created attachment 457702 [details]
WIP-patch-for-review

[cathiechen]

Comment on attachment 457702 [details]
WIP-patch-for-review

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

> Source/WebCore/rendering/style/RenderStyle.cpp:783
> +    if (first.containIntrinsicWidth != second.containIntrinsicWidth

Maybe we should move this change to the parsing patch?

[cathiechen]

Created attachment 457794 [details]
Patch-for-review

[cathiechen]

Created attachment 457795 [details]
patch-for-ews-with-parsing-code

[cathiechen]

Comment on attachment 457794 [details]
Patch-for-review

Hi,
I think this patch is ready for review. Thanks!
* The patch-for-review only contains the implementation part, not the parsing and runtime flags code. 
* The patch-for-ews contains the parsing and runtime flags code, see bug 238181.

[cathiechen]

Created attachment 457797 [details]
Patch-for-review

[cathiechen]

Created attachment 457799 [details]
patch-for-ews-with-parsing-code

[cathiechen]

Created attachment 457800 [details]
Patch-for-review

[Rob Buis]

Comment on attachment 457800 [details]
Patch-for-review

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

> Source/WebCore/rendering/RenderBox.cpp:5520
> +        return false;

I think it would be nice to minimise shouldApplySizeContainment calls and also maybe turning this into ASSERT.

> Source/WebCore/rendering/RenderReplaced.cpp:526
> +    // Don't let size containment effect aspect ratio. Maybe we should split this out?

Affect.

> Source/WebCore/rendering/style/RenderStyle.cpp:779
> +    // This change should be slipt out, maybe only size containment needs relayout

Split.

> Source/WebCore/rendering/style/RenderStyle.cpp:784
> +        || first.containIntrinsicHeight != second.containIntrinsicHeight)

Can probably go on one line.

[cathiechen]

Comment on attachment 457800 [details]
Patch-for-review

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

Hi Rob, thanks for the reivew!

>> Source/WebCore/rendering/RenderBox.cpp:5520
>> +        return false;
> 
> I think it would be nice to minimise shouldApplySizeContainment calls and also maybe turning this into ASSERT.

Hmm, there are some scenarios we need to check shouldApplySizeContainment first. How about we add an new interface sizeContainmentHasExplicitIntrinsicInnerLogicalWidth()?

>> Source/WebCore/rendering/RenderReplaced.cpp:526
>> +    // Don't let size containment effect aspect ratio. Maybe we should split this out?
> 
> Affect.

Done

>> Source/WebCore/rendering/style/RenderStyle.cpp:779
>> +    // This change should be slipt out, maybe only size containment needs relayout
> 
> Split.

Done

>> Source/WebCore/rendering/style/RenderStyle.cpp:784
>> +        || first.containIntrinsicHeight != second.containIntrinsicHeight)
> 
> Can probably go on one line.

Done

[cathiechen]

Created attachment 458117 [details]
Patch

[cathiechen]

Created attachment 458208 [details]
Patch

[cathiechen]

Created attachment 458252 [details]
Patch

[cathiechen]

Comment on attachment 458252 [details]
Patch

Hi, I think this patch is ready for review:) Please take a look, thanks!

[Rob Buis]

Comment on attachment 458252 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:3095
> +        LayoutUnit explicitIntrinsicHeight = sizeContainmentHasExplicitIntrinsicInnerLogicalHeight() ? explicitIntrinsicInnerLogicalHeight() : LayoutUnit(0);

LayoutUnit(0) -> LayoutUnit().

> Source/WebCore/rendering/RenderBox.cpp:3234
> +        estimatedHeight += explicitIntrinsicInnerLogicalHeight() + scrollbarLogicalHeight();

So is the explicit intrinsic height always excluding any scrollbar heights?

> Source/WebCore/rendering/RenderListBox.cpp:209
> +    maxLogicalWidth =  2 * optionsSpacingHorizontal;

In case sizeContainmentHasExplicitIntrinsicInnerLogicalWidth holds true, we do this statement for no good reason, since we know it will be overwritten.

> Source/WebCore/rendering/RenderListBox.cpp:214
> +                minLogicalWidth = maxLogicalWidth;

Is minLogicalWidth undefined in case style().width().isPercentOrCalculated() is true?

> Source/WebCore/rendering/RenderSlider.cpp:74
> +            minLogicalWidth = maxLogicalWidth = explicitIntrinsicInnerLogicalWidth();

I see some places min = max = .... and some max = min = ... . Better to pick one style.

[cathiechen]

Comment on attachment 458252 [details]
Patch

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

Hi Rob, thanks for the review!

>> Source/WebCore/rendering/RenderBox.cpp:3095
>> +        LayoutUnit explicitIntrinsicHeight = sizeContainmentHasExplicitIntrinsicInnerLogicalHeight() ? explicitIntrinsicInnerLogicalHeight() : LayoutUnit(0);
> 
> LayoutUnit(0) -> LayoutUnit().

Done

>> Source/WebCore/rendering/RenderBox.cpp:3234
>> +        estimatedHeight += explicitIntrinsicInnerLogicalHeight() + scrollbarLogicalHeight();
> 
> So is the explicit intrinsic height always excluding any scrollbar heights?

Nope, scrollbar is not included in the explicit intrinsic size.

>> Source/WebCore/rendering/RenderListBox.cpp:209
>> +    maxLogicalWidth =  2 * optionsSpacingHorizontal;
> 
> In case sizeContainmentHasExplicitIntrinsicInnerLogicalWidth holds true, we do this statement for no good reason, since we know it will be overwritten.

Yes, this is outside because it can be reused by `!shouldApplySizeContainment(*this)` and `shouldApplySizeContainment(*this) && !sizeContainmentHasExplicitIntrinsicInnerLogicalWidth()`

>> Source/WebCore/rendering/RenderListBox.cpp:214
>> +                minLogicalWidth = maxLogicalWidth;
> 
> Is minLogicalWidth undefined in case style().width().isPercentOrCalculated() is true?

I guess so. This is copied from the following code, we don't want to change the behavior. Hmm, looks like we can reuse the following code, I didn't because contain-intrinsic-size-009.html fails. It looks like the failure is related to `overflow:visible` doesn't make select's scroll bar disappeared. The value of contain-intrinsic-width shouldn't include scrollbar. I'll try to explain this in TestExpectations.

>> Source/WebCore/rendering/RenderSlider.cpp:74
>> +            minLogicalWidth = maxLogicalWidth = explicitIntrinsicInnerLogicalWidth();
> 
> I see some places min = max = .... and some max = min = ... . Better to pick one style.

Done

[cathiechen]

Comment on attachment 458252 [details]
Patch

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

>>> Source/WebCore/rendering/RenderListBox.cpp:209
>>> +    maxLogicalWidth =  2 * optionsSpacingHorizontal;
>> 
>> In case sizeContainmentHasExplicitIntrinsicInnerLogicalWidth holds true, we do this statement for no good reason, since we know it will be overwritten.
> 
> Yes, this is outside because it can be reused by `!shouldApplySizeContainment(*this)` and `shouldApplySizeContainment(*this) && !sizeContainmentHasExplicitIntrinsicInnerLogicalWidth()`

This seems hard to read, I'll rewrite it.

[cathiechen]

Created attachment 458257 [details]
Patch

[Rob Buis]

Comment on attachment 458257 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.h:673
> +    LayoutUnit explicitIntrinsicInnerWidth() const;

This could return optional, then maybe some hasFoo methods are not needed.

> Source/WebCore/rendering/RenderListBox.cpp:213
> +            maxLogicalWidth = 2* optionsSpacingHorizontal;

2* -> 2 *

[Darin Adler]

Comment on attachment 458257 [details]
Patch

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

> Source/WebCore/rendering/RenderReplaced.h:46
> +            LayoutSize size = LayoutSize();

The "= LayoutSize()" here doesn’t do anything and can be omitted. LayoutUnit values are initialized to 0, unlike scalars which can be left uninitialized.

> Source/WebCore/rendering/RenderVideo.cpp:117
> +        LayoutSize size = LayoutSize();

The "= LayoutSize()" here doesn’t do anything and can be omitted.

[cathiechen]

Comment on attachment 458257 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.h:673
>> +    LayoutUnit explicitIntrinsicInnerWidth() const;
> 
> This could return optional, then maybe some hasFoo methods are not needed.

Looks like optional could make the interface much more clear. Done!

>> Source/WebCore/rendering/RenderListBox.cpp:213
>> +            maxLogicalWidth = 2* optionsSpacingHorizontal;
> 
> 2* -> 2 *

Done, thanks!

>> Source/WebCore/rendering/RenderReplaced.h:46
>> +            LayoutSize size = LayoutSize();
> 
> The "= LayoutSize()" here doesn’t do anything and can be omitted. LayoutUnit values are initialized to 0, unlike scalars which can be left uninitialized.

Done, thanks!

>> Source/WebCore/rendering/RenderVideo.cpp:117
>> +        LayoutSize size = LayoutSize();
> 
> The "= LayoutSize()" here doesn’t do anything and can be omitted.

Done, thanks!

[cathiechen]

Created attachment 458438 [details]
Patch

[cathiechen]

Created attachment 458439 [details]
Patch

[cathiechen]

Created attachment 458440 [details]
Patch

[Rob Buis]

Comment on attachment 458440 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        This patch add support for contain-intrinsic-size value "none | <length>". The value "auto && <length>"

add -> adds.

> Source/WebCore/rendering/RenderBox.cpp:5535
> +    return std::optional<LayoutUnit>(width.value().value());

This probably does not need the explicit cast.

> Source/WebCore/rendering/RenderBox.cpp:5551
> +    return std::optional<LayoutUnit>(height.value().value());

Ditto.

> Source/WebCore/rendering/RenderGrid.cpp:544
> +        return std::optional<LayoutUnit>();

Ditto.

[cathiechen]

Comment on attachment 458440 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        This patch add support for contain-intrinsic-size value "none | <length>". The value "auto && <length>"
> 
> add -> adds.

Done

>> Source/WebCore/rendering/RenderBox.cpp:5535
>> +    return std::optional<LayoutUnit>(width.value().value());
> 
> This probably does not need the explicit cast.

Tried locally, it seems needed, for width.value().value() is a float, not LayoutUnit.

>> Source/WebCore/rendering/RenderGrid.cpp:544
>> +        return std::optional<LayoutUnit>();
> 
> Ditto.

Done.

[cathiechen]

Created attachment 458448 [details]
Patch

[Rob Buis]

Comment on attachment 458448 [details]
Patch

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

Patch is now good enough for me, maybe nice to use the has_value() though.

> Source/WebCore/rendering/RenderGrid.cpp:509
> +        LayoutUnit totalGuttersSize = explicitIntrinsicInnerLogicalSize(ForColumns) ? 0_lu : guttersSize(m_grid, ForColumns, 0, numTracks(ForColumns), std::nullopt);

Maybe has_value() is clearer here.

> Source/WebCore/rendering/RenderGrid.cpp:531
> +    LayoutUnit totalGuttersSize = direction == ForColumns && explicitIntrinsicInnerLogicalSize(direction) ? 0_lu : guttersSize(grid, direction, 0, numberOfTracks, std::nullopt);

Ditto.

[cathiechen]

Comment on attachment 458448 [details]
Patch

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

>> Source/WebCore/rendering/RenderGrid.cpp:509
>> +        LayoutUnit totalGuttersSize = explicitIntrinsicInnerLogicalSize(ForColumns) ? 0_lu : guttersSize(m_grid, ForColumns, 0, numTracks(ForColumns), std::nullopt);
> 
> Maybe has_value() is clearer here.

Done, thanks:)

>> Source/WebCore/rendering/RenderGrid.cpp:531
>> +    LayoutUnit totalGuttersSize = direction == ForColumns && explicitIntrinsicInnerLogicalSize(direction) ? 0_lu : guttersSize(grid, direction, 0, numberOfTracks, std::nullopt);
> 
> Ditto.

Done

[cathiechen]

Created attachment 458452 [details]
Patch

[Rob Buis]

I am happy with the patch now, thanks Cathie!

[cathiechen]

Created attachment 459164 [details]
Patch

[cathiechen]

Rebase the code.

[Rob Buis]

Created attachment 459749 [details]
Patch

[Rob Buis]

Pull request: https://github.com/WebKit/WebKit/pull/1373

[Brent Fulgham]

*** Bug 240888 has been marked as a duplicate of this bug. ***

[cathiechen]

Rebase the code: https://github.com/WebKit/WebKit/pull/1799

[EWS]

Committed 255971@main (7ed83ab2929f): <https://commits.webkit.org/255971@main>

Reviewed commits have been landed. Closing PR #1799 and removing active labels.