Bug 239489 – The layout is not updated when style.contain is changed from "size"/"inline-size" to empty string

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

[cathiechen]

After removing inline-size from style.contain, the element should trigger layout

[cathiechen]

Created attachment 457869 [details]
Patch

[cathiechen]

Created attachment 457871 [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

[cathiechen]

Created attachment 457876 [details]
Patch

[cathiechen]

Comment on attachment 457876 [details]
Patch

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

[Rob Buis]

Comment on attachment 457876 [details]
Patch

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

> Source/WebCore/rendering/style/RenderStyle.cpp:880
> +        if (containsSize() != other.containsSize() || containsInlineSize() != other.containsInlineSize())

Should these checks not be done outside of this if block since it also depends on container queries?

[cathiechen]

Comment on attachment 457876 [details]
Patch

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

>> Source/WebCore/rendering/style/RenderStyle.cpp:880
>> +        if (containsSize() != other.containsSize() || containsInlineSize() != other.containsInlineSize())
> 
> Should these checks not be done outside of this if block since it also depends on container queries?

Hmmm, containerType is inside m_rareNonInheritedData, if they are same, looks like there is no chance that containsInlineSize() is different.

[Rob Buis]

Comment on attachment 457876 [details]
Patch

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

>>> Source/WebCore/rendering/style/RenderStyle.cpp:880
>>> +        if (containsSize() != other.containsSize() || containsInlineSize() != other.containsInlineSize())
>> 
>> Should these checks not be done outside of this if block since it also depends on container queries?
> 
> Hmmm, containerType is inside m_rareNonInheritedData, if they are same, looks like there is no chance that containsInlineSize() is different.

Ah, I missed that. How about moving the body of effectiveContainment into StyleRareNonInheritedData and then rareNonInheritedDataChangeRequiresLayout can check containsSize/containsInlineSize? The benefit is that the part you changed above remains easy to read.

[cathiechen]

Comment on attachment 457876 [details]
Patch

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

>>>> Source/WebCore/rendering/style/RenderStyle.cpp:880
>>>> +        if (containsSize() != other.containsSize() || containsInlineSize() != other.containsInlineSize())
>>> 
>>> Should these checks not be done outside of this if block since it also depends on container queries?
>> 
>> Hmmm, containerType is inside m_rareNonInheritedData, if they are same, looks like there is no chance that containsInlineSize() is different.
> 
> Ah, I missed that. How about moving the body of effectiveContainment into StyleRareNonInheritedData and then rareNonInheritedDataChangeRequiresLayout can check containsSize/containsInlineSize? The benefit is that the part you changed above remains easy to read.

Done

[cathiechen]

Created attachment 458141 [details]
Patch

[Rob Buis]

Comment on attachment 458141 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        The layout is not updated when style.contain is changed from "size"/"inline-size"to empty string

Extra space after "size"/"inline-size"

> Source/WebCore/ChangeLog:8
> +        As contain: size/inline-size changes the layout result, but the values "layout", "paint" and

Can you rephrase the first sentence?

> Source/WebCore/ChangeLog:9
> +        "style" are not. So we need to require layout when the value is changed from "size"/"inline-size"

s/So we need to require layout/So we require layout

> LayoutTests/imported/w3c/web-platform-tests/css/css-contain/contain-inline-size-removed.html:13
> +<div id="container" style="contain: inline-size; width:fit-content; overflow: hidden;"><span>PASS</span></div>

Why is width:fit-content needed here and not in the other test case?

[cathiechen]

Comment on attachment 458141 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        The layout is not updated when style.contain is changed from "size"/"inline-size"to empty string
> 
> Extra space after "size"/"inline-size"

Done

>> Source/WebCore/ChangeLog:8
>> +        As contain: size/inline-size changes the layout result, but the values "layout", "paint" and
> 
> Can you rephrase the first sentence?

Done

>> Source/WebCore/ChangeLog:9
>> +        "style" are not. So we need to require layout when the value is changed from "size"/"inline-size"
> 
> s/So we need to require layout/So we require layout

Done

>> LayoutTests/imported/w3c/web-platform-tests/css/css-contain/contain-inline-size-removed.html:13
>> +<div id="container" style="contain: inline-size; width:fit-content; overflow: hidden;"><span>PASS</span></div>
> 
> Why is width:fit-content needed here and not in the other test case?

Because inline-size containment only affects the inline-axis sizing. If there is no "width:fit-content", the widths are same (its container's width). We can't tell if contain:inline-size is applied or not.
https://www.w3.org/TR/css-contain-3/#containment-inline-size

[cathiechen]

Created attachment 458145 [details]
Patch

[cathiechen]

Created attachment 458146 [details]
Patch

[EWS]

Committed r293288 (249913@main): <https://commits.webkit.org/249913@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/92207316>