Bug 226458 – [css-contain] Support contain:style for counters

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

[Rob Buis]

Support contain:style

[Rob Buis]

Created attachment 430185 [details]
Patch

[Rob Buis]

Created attachment 430187 [details]
Patch

[Rob Buis]

Created attachment 430680 [details]
Patch

[Rob Buis]

Created attachment 430681 [details]
Patch

[Rob Buis]

Created attachment 430717 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/78937603>

[Rob Buis]

Created attachment 430721 [details]
Patch

[Rob Buis]

Created attachment 430750 [details]
Patch

[Antti Koivisto]

Comment on attachment 430750 [details]
Patch

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

> Source/WebCore/rendering/RenderCounter.cpp:64
> +    Element* ancestor = is<PseudoElement>(element) ? downcast<PseudoElement>(element).hostElement() : element.parentElement();
> +    while (ancestor) {
> +        if (auto* style = ancestor->existingComputedStyle()) {

I suppose this doesn't just traverse the render tree and get the style from there because you can have style containment in a non-rendered 'display:contents' element? Is that covered by tests?

> Source/WebCore/rendering/RenderCounter.cpp:68
> +        ancestor = ancestor->parentElement();

This should probably be doing composed tree traversal (parentInComposedTree). Of course the existing counter code doesn't do that either (so counters don't work accross shadow boundaries). At least this deserves a comment.

> Source/WebCore/rendering/RenderCounter.cpp:87
> +    Element* styleContainAncestor = ancestorStyleContainmentObject(*renderer.element());
> +
> +    while (true) {
> +        while (previous && !previous->renderer())
> +            previous = ElementTraversal::previousIncludingPseudo(*previous);
> +        if (!previous)
> +            return nullptr;
> +        Element* previousStyleContainAncestor = ancestorStyleContainmentObject(*previous);
> +        if (previousStyleContainAncestor == styleContainAncestor)

These traversal functions take optional 'stayWithin' parameter which I think should here be the styleContainAncestor. This avoids searching  for the ancestor repeatedly.

> Source/WebCore/rendering/RenderCounter.cpp:578
> +    bool crossedBoundary = false;

Which boundary? This could use a better name.

> Source/WebCore/rendering/RenderCounter.cpp:580
> +        crossedBoundary |= shouldApplyStyleContainment(*descendant);

using logical operator instead of a bitwise one will avoid running the function repeatedly

crossedBoundary = crossedBoundary || shouldApplyStyleContainment(*descendant);

> Source/WebCore/rendering/RenderObject.cpp:2520
> +    return renderer.style().containsStyle() && (!renderer.isInline() || renderer.isAtomicInlineLevelBox()) && !renderer.isRubyText() && (!renderer.isTablePart() || renderer.isTableCaption()) && !renderer.isTable();

if (!renderer.style().containsStyle())
   return false;
if (renderer.isInline() && !renderer.isAtomicInlineLevelBox())
   return false;
...etc organised in some readable manner

> Source/WebCore/rendering/style/RenderStyle.h:531
>      bool containsLayout() const { return m_rareNonInheritedData->contain.contains(Containment::Layout); }
>      bool containsSize() const { return m_rareNonInheritedData->contain.contains(Containment::Size); }
> +    bool containsStyle() const { return m_rareNonInheritedData->contain.contains(Containment::Style); }

Maybe these would read better as

hasLayoutContainment()/hasSizeContainment()/hasStyleContainmen()?

[Antti Koivisto]

Comment on attachment 430750 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        [css-contain] Support contain:style
> +        https://bugs.webkit.org/show_bug.cgi?id=226458

Besides counters the spec talks about quotes and this patch doesn't address those. The title should be updated to reflect that this is not a complete implementation.

The relevant quote code is in RenderTreeUpdater::GeneratedContent::updateQuotesUpTo

[Rob Buis]

Created attachment 442017 [details]
Patch

[Rob Buis]

Comment on attachment 430750 [details]
Patch

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

>> Source/WebCore/ChangeLog:4
>> +        https://bugs.webkit.org/show_bug.cgi?id=226458
> 
> Besides counters the spec talks about quotes and this patch doesn't address those. The title should be updated to reflect that this is not a complete implementation.
> 
> The relevant quote code is in RenderTreeUpdater::GeneratedContent::updateQuotesUpTo

I updated the title and opened https://bugs.webkit.org/show_bug.cgi?id=232083 to track the quote part.

>> Source/WebCore/rendering/RenderCounter.cpp:64
>> +        if (auto* style = ancestor->existingComputedStyle()) {
> 
> I suppose this doesn't just traverse the render tree and get the style from there because you can have style containment in a non-rendered 'display:contents' element? Is that covered by tests?

Yes. I believe this is covered by contain-style-counters-002.html.

>> Source/WebCore/rendering/RenderCounter.cpp:68
>> +        ancestor = ancestor->parentElement();
> 
> This should probably be doing composed tree traversal (parentInComposedTree). Of course the existing counter code doesn't do that either (so counters don't work accross shadow boundaries). At least this deserves a comment.

Done.

>> Source/WebCore/rendering/RenderCounter.cpp:87
>> +        if (previousStyleContainAncestor == styleContainAncestor)
> 
> These traversal functions take optional 'stayWithin' parameter which I think should here be the styleContainAncestor. This avoids searching  for the ancestor repeatedly.

Done. This may actually have fixed one more test.

>> Source/WebCore/rendering/RenderCounter.cpp:578
>> +    bool crossedBoundary = false;
> 
> Which boundary? This could use a better name.

Done.

>> Source/WebCore/rendering/RenderCounter.cpp:580
>> +        crossedBoundary |= shouldApplyStyleContainment(*descendant);
> 
> using logical operator instead of a bitwise one will avoid running the function repeatedly
> 
> crossedBoundary = crossedBoundary || shouldApplyStyleContainment(*descendant);

Done.

>> Source/WebCore/rendering/RenderObject.cpp:2520
>> +    return renderer.style().containsStyle() && (!renderer.isInline() || renderer.isAtomicInlineLevelBox()) && !renderer.isRubyText() && (!renderer.isTablePart() || renderer.isTableCaption()) && !renderer.isTable();
> 
> if (!renderer.style().containsStyle())
>    return false;
> if (renderer.isInline() && !renderer.isAtomicInlineLevelBox())
>    return false;
> ...etc organised in some readable manner

I was only partially able to do this.

>> Source/WebCore/rendering/style/RenderStyle.h:531
>> +    bool containsStyle() const { return m_rareNonInheritedData->contain.contains(Containment::Style); }
> 
> Maybe these would read better as
> 
> hasLayoutContainment()/hasSizeContainment()/hasStyleContainmen()?

Good suggestion, but I think it is for another patch.

[EWS]

Committed r284642 (243362@main): <https://commits.webkit.org/243362@main>

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

[Antti Koivisto]

Comment on attachment 442017 [details]
Patch

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

> Source/WebCore/rendering/RenderCounter.cpp:88
> +        Element* previousStyleContainAncestor = ancestorStyleContainmentObject(*previous);
> +        if (previousStyleContainAncestor == styleContainAncestor)

The idea was to remove this stuff. You are staying within the styleContainAncestor by providing it to previousIncludingPseudo so the test here should never fail. 

In fact I think the whole outer loop can go away.

[Rob Buis]

Reopening to attach new patch.

[Rob Buis]

Created attachment 442303 [details]
Patch

[Rob Buis]

Comment on attachment 442017 [details]
Patch

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

>> Source/WebCore/rendering/RenderCounter.cpp:88
>> +        if (previousStyleContainAncestor == styleContainAncestor)
> 
> The idea was to remove this stuff. You are staying within the styleContainAncestor by providing it to previousIncludingPseudo so the test here should never fail. 
> 
> In fact I think the whole outer loop can go away.

Oops! Yes I did not see it earlier but indeed this is trying to do too much. Fixed.

[EWS]

Committed r284755 (243464@main): <https://commits.webkit.org/243464@main>

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