Bug 246216 – Invalid counters after contain: style

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

[Michal Niedziolka]

Example: https://codepen.io/NiedziolkaMichal/pen/abGaeBg

Currently contain: style correctly starts independent counter value for #contained element, because of which "Contained Child 1" has "1." next to it instead of "3.".
Element "Child 3" which is located outside of contained element, should not have been inpacted by contained element, so it's counter value should be "3". On Chrome and Firefox example prints:

1. Child 1
2. Child 2
   1. Contained Child 1
3. Child 3


Second issue is that changing value of "contain" property doesn't affect counter values. That's why buttons in codepen example doesn't work. On Firefox they work fine, while on Chrome they also doesn't work and I filed issue in the too: https://bugs.chromium.org/p/chromium/issues/detail?id=1372435

[Radar WebKit Bug Importer]

<rdar://problem/100904445>

[Karl Dubost]

Created attachment 462931 [details]
rendering in safari, firefox, chrome

Safari

```
1. Child 1
2. Child 2
    1. Contained Child 1
1. Child 3
```

Firefox 
```
1. Child 1
2. Child 2
    1. Contained Child 1
3. Child 3
```

Chrome
```
1. Child 1
2. Child 2
    3. Contained Child 1
4. Child 3
```

3 browsers, 3 different results.

[Rob Buis]

(In reply to Karl Dubost from comment #2)
> Created attachment 462931 [details]
> rendering in safari, firefox, chrome
> 
> Safari
> 
> ```
> 1. Child 1
> 2. Child 2
>     1. Contained Child 1
> 1. Child 3
> ```
> 
> Firefox 
> ```
> 1. Child 1
> 2. Child 2
>     1. Contained Child 1
> 3. Child 3
> ```
> 
> Chrome
> ```
> 1. Child 1
> 2. Child 2
>     3. Contained Child 1
> 4. Child 3
> ```
> 
> 3 browsers, 3 different results.

On my Chrome 106 (Mac) I get same result as Firefox, ie. last counter is "3. Child 3", can you check? It does not react to the buttons, I can confirm that.

[Michal Niedziolka]

On Chrome counter value is sometimes 3 and sometimes its 1. You may notice it after refreshing the page few times:
https://drive.google.com/file/d/1iAF_Urcot_hv0LLlWXQRiw6fBi2t9XXQ/view?usp=sharing

[Karl Dubost]

My tests were done with 

Tested on macOS 13.0
---
Safari Technology Preview  155           18615.1.7.1
Firefox Nightly            107.0a1       10722.10.4
Google Chrome Canary       108.0.5355.0  5355.0

I always get the same result.

[Rob Buis]

Created attachment 463088 [details]
Patch

[Rob Buis]

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

[Rob Buis]

(In reply to Michal Niedziolka from comment #0)
> Second issue is that changing value of "contain" property doesn't affect
> counter values. That's why buttons in codepen example doesn't work. On
> Firefox they work fine, while on Chrome they also doesn't work and I filed
> issue in the too:
> https://bugs.chromium.org/p/chromium/issues/detail?id=1372435

This report describes two bugs, so I created a bug like the chromium one for the "contain" property changing problem:
https://bugs.webkit.org/show_bug.cgi?id=246764

[Darin Adler]

Comment on attachment 463096 [details]
Patch

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

> Source/WebCore/rendering/RenderCounter.cpp:76
>  // This function processes the renderer tree in the order of the DOM tree
>  // including pseudo elements as defined in CSS 2.1.

This comment does not seem quite accurate any more.

> Source/WebCore/rendering/RenderCounter.cpp:103
> +        while (previous && !previous->renderer())
> +            previous = ElementTraversal::previousIncludingPseudo(*previous, styleContainAncestor);
> +        if (!previous)
> +            return nullptr;
> +        Element* previousStyleContainAncestor = ancestorStyleContainmentObject(*previous);
> +        // If the candidate's containment ancestor is the same as elements, then
> +        // that's a valid candidate.
> +        if (previousStyleContainAncestor == styleContainAncestor)
> +            return previous->renderer();
> +
> +        // Otherwise, if previous does not have a containment ancestor, it means
> +        // that we have already escaped `element`'s containment ancestor, so return
> +        // nullptr.
> +        if (!previousStyleContainAncestor)
> +            return nullptr;
> +
> +        // If, however, the candidate does have a containment ancestor, it could be
> +        // that we entered a new sub-containment. Try again starting from the
> +        // contain ancestor.
> +        previous = previousStyleContainAncestor;

This can be written more economically:

    while (previous) {
        while (previous && !previous->renderer())
            previous = ElementTraversal::previousIncludingPseudo(*previous, styleContainAncestor);
        if (!previous)
            break;
        auto* previousStyleContainmentAncestor = ancestorStyleContainmentObject(*previous);
        if (previousStyleContainmentAncestor == styleContainAncestor)
            return previous->renderer();
        previous = previousStyleContainmentAncestor;
    }
    return nullptr;

Could still include the same comments, but don’t need as much code.

[Rob Buis]

Created attachment 463189 [details]
Patch

[Rob Buis]

Created attachment 463197 [details]
Patch

[Darin Adler]

Comment on attachment 463197 [details]
Patch

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

> Source/WebCore/rendering/RenderCounter.cpp:89
> +        auto* previousStyleContainAncestor = ancestorStyleContainmentObject(*previous);

Why do we use the words "contain ancestor" instead of "containment ancestor" in these two variable names (one new, one pre-existing)? The phrase "contain ancestor" means something else! We should rename both.

[Rob Buis]

Comment on attachment 463197 [details]
Patch

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

>> Source/WebCore/rendering/RenderCounter.cpp:89
>> +        auto* previousStyleContainAncestor = ancestorStyleContainmentObject(*previous);
> 
> Why do we use the words "contain ancestor" instead of "containment ancestor" in these two variable names (one new, one pre-existing)? The phrase "contain ancestor" means something else! We should rename both.

Right, that was just lazy, will fix before landing!

[Rob Buis]

Created attachment 463201 [details]
Patch

[EWS]

Committed 255945@main (380d2a88248a): <https://commits.webkit.org/255945@main>

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