Bug 239489 - The layout is not updated when style.contain is changed from "size"/"inline-size" to empty string
Summary: The layout is not updated when style.contain is changed from "size"/"inline-s...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: cathiechen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-19 02:22 PDT by cathiechen
Modified: 2022-04-22 23:46 PDT (History)
12 users (show)

See Also:


Attachments
Patch (8.22 KB, patch)
2022-04-19 02:42 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (8.22 KB, patch)
2022-04-19 02:44 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (9.78 KB, patch)
2022-04-19 03:55 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (11.94 KB, patch)
2022-04-22 06:41 PDT, cathiechen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (11.98 KB, patch)
2022-04-22 07:42 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (11.97 KB, patch)
2022-04-22 07:50 PDT, cathiechen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description cathiechen 2022-04-19 02:22:32 PDT
After removing inline-size from style.contain, the element should trigger layout
Comment 1 cathiechen 2022-04-19 02:42:42 PDT
Created attachment 457869 [details]
Patch
Comment 2 cathiechen 2022-04-19 02:44:30 PDT
Created attachment 457871 [details]
Patch
Comment 3 EWS Watchlist 2022-04-19 02:44:35 PDT
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
Comment 4 cathiechen 2022-04-19 03:55:50 PDT
Created attachment 457876 [details]
Patch
Comment 5 cathiechen 2022-04-19 04:25:21 PDT
Comment on attachment 457876 [details]
Patch

Hi, I think this patch is ready for review. Please take a look, thanks!
Comment 6 Rob Buis 2022-04-19 04:34:40 PDT
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?
Comment 7 cathiechen 2022-04-19 22:58:03 PDT
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.
Comment 8 Rob Buis 2022-04-22 01:12:30 PDT
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.
Comment 9 cathiechen 2022-04-22 06:35:27 PDT
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
Comment 10 cathiechen 2022-04-22 06:41:04 PDT
Created attachment 458141 [details]
Patch
Comment 11 Rob Buis 2022-04-22 06:59:13 PDT
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?
Comment 12 cathiechen 2022-04-22 07:39:52 PDT
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
Comment 13 cathiechen 2022-04-22 07:42:59 PDT
Created attachment 458145 [details]
Patch
Comment 14 cathiechen 2022-04-22 07:50:50 PDT
Created attachment 458146 [details]
Patch
Comment 15 EWS 2022-04-22 23:45:14 PDT
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].
Comment 16 Radar WebKit Bug Importer 2022-04-22 23:46:15 PDT
<rdar://problem/92207316>