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