Bug 246216

<rdar://problem/100904445>

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.

(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.

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

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.

Created attachment 463088 [details]
Patch

Created attachment 463096 [details]
Patch

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

(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

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.

Created attachment 463189 [details]
Patch

Created attachment 463197 [details]
Patch

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.

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!

Created attachment 463201 [details]
Patch

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].