Bug 226458

Summary: [css-contain] Support contain:style for counters
Product: WebKit Reporter: Rob Buis <rbuis>
Component: CSSAssignee: Rob Buis <rbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, koivisto, kondapallykalyan, macpherson, menard, pdr, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 172026    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Rob Buis 2021-05-31 00:47:56 PDT
Support contain:style
Comment 1 Rob Buis 2021-05-31 00:50:18 PDT
Created attachment 430185 [details]
Patch
Comment 2 Rob Buis 2021-05-31 02:00:26 PDT
Created attachment 430187 [details]
Patch
Comment 3 Rob Buis 2021-06-06 04:33:19 PDT
Created attachment 430680 [details]
Patch
Comment 4 Rob Buis 2021-06-06 07:02:53 PDT
Created attachment 430681 [details]
Patch
Comment 5 Rob Buis 2021-06-07 00:17:45 PDT
Created attachment 430717 [details]
Patch
Comment 6 Radar WebKit Bug Importer 2021-06-07 00:48:16 PDT
<rdar://problem/78937603>
Comment 7 Rob Buis 2021-06-07 01:21:21 PDT
Created attachment 430721 [details]
Patch
Comment 8 Rob Buis 2021-06-07 08:49:16 PDT
Created attachment 430750 [details]
Patch
Comment 9 Antti Koivisto 2021-10-21 03:06:43 PDT
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()?
Comment 10 Antti Koivisto 2021-10-21 03:14:54 PDT
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
Comment 11 Rob Buis 2021-10-21 07:12:51 PDT
Created attachment 442017 [details]
Patch
Comment 12 Rob Buis 2021-10-21 08:33:27 PDT
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.
Comment 13 EWS 2021-10-21 13:48:34 PDT
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].
Comment 14 Antti Koivisto 2021-10-21 21:53:22 PDT
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.
Comment 15 Rob Buis 2021-10-24 04:15:38 PDT
Reopening to attach new patch.
Comment 16 Rob Buis 2021-10-24 04:15:43 PDT
Created attachment 442303 [details]
Patch
Comment 17 Rob Buis 2021-10-24 06:11:45 PDT
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.
Comment 18 EWS 2021-10-24 07:50:11 PDT
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].