Bug 102919 – [Shadow DOM]: custom pseudo in styles in shadow dom trees doesn't work

• Component: CSS
• Status: RESOLVED INVALID

[Takashi Sakamoto]

Since a treescope where the style is registered is different from a treescope where the style is applied to.

 <#shadow-root1>
           |
           +---<style id=A> progress::-webkit-progress-value...
           |
           +---<progress> ----<#shadow-root2>
                                                     |
                                                     +---<div id=-webkit-progress-bar>
                                                                 |
                                                                +---<div id=-webkit-progress-value>

The current implementation:
- <style id=A> is registered with <#shadow-root1>.
- when resolving <div id=-webkit-progress-value>'s style, <style id=A> is in the different treescope from <#shadow-root2>. So no style will applied to the div.
- however, the rule, i.e. progress::-webkit-progress-value..., should be applied.

[Takashi Sakamoto]

Created attachment 175406 [details]
repro

[Takashi Sakamoto]

Created attachment 179464 [details]
WIP

[Takashi Sakamoto]

My patch is still "WIP", however I would like to ask feedbacks. I'm afraid that I'm on the wrong way to fix this issue.

What I'm trying to do is:
- custom pseudo in UA stylesheets
   --> should check whether elements in any treescope match or not.

e.g.
html.css:
progress:-webkit-progress-value { ... }

#shadow-root
    #shadow-root
       div::-webkit-progress-value <--- always apply above rule


- custom pseudo in an author stylesheet 
   --> should check whether elements in any child treescopes of document and in some treescopes whose parent treescopes have apply-author-styles true. If so, try to apply. Otherwise, ignore.

e.g.
document:
<style>
div::x-foo { ... }
</style>

#shadow-root
   div::x-foo <--- apply
   #shadow-root
       #shadow-root
           div::x-foo <--- not apply

- custom pseudo in some stylesheet in a shadow dom tree
  --> for the given element, looking at enclosing shadow dom trees from the element's parent treescope:
      - an enclosing shadow dom tree has apply-author-styles false, stop.
      - check all custom pseudo styles in the current shadow dom tree,
      - walk up to an shadow dom tree which encloses the current one.

e.g.
#shadow-root
   <style> div::x-foo { ... }
   #shadow-root (apply-author-styles: true)
       #shadow-root
           div::x-foo <--- apply

Best regards,
Takashi Sakamoto

[WebKit Review Bot]

Comment on attachment 179464 [details]
WIP

Attachment 179464 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15353001

New failing tests:
fast/dom/shadow/shadow-nested-pseudo-id.html

[WebKit Review Bot]

Comment on attachment 179464 [details]
WIP

Attachment 179464 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15343003

New failing tests:
fast/dom/shadow/shadow-nested-pseudo-id.html

[Dimitri Glazkov (Google)]

(In reply to comment #3)
> My patch is still "WIP", however I would like to ask feedbacks. I'm afraid that I'm on the wrong way to fix this issue.
> 
> What I'm trying to do is:
> - custom pseudo in UA stylesheets
>    --> should check whether elements in any treescope match or not.
> 
> e.g.
> html.css:
> progress:-webkit-progress-value { ... }
> 
> #shadow-root
>     #shadow-root
>        div::-webkit-progress-value <--- always apply above rule

I am assuming that by "div::-webkit-progress-value" you mean "div[pseudo=-webkit-progress-value]". Right?

In this case, yes. (naturally, only if the host of the tree in which the div is located has tag name "progress")

I apologize, I didn't have a chance to look at this over the weekend, but I'll attack this Monday! :)

> 
> 
> - custom pseudo in an author stylesheet 
>    --> should check whether elements in any child treescopes of document and in some treescopes whose parent treescopes have apply-author-styles true. If so, try to apply. Otherwise, ignore.
> 
> e.g.
> document:
> <style>
> div::x-foo { ... }
> </style>
> 
> #shadow-root
>    div::x-foo <--- apply
>    #shadow-root
>        #shadow-root
>            div::x-foo <--- not apply
> 
> - custom pseudo in some stylesheet in a shadow dom tree
>   --> for the given element, looking at enclosing shadow dom trees from the element's parent treescope:
>       - an enclosing shadow dom tree has apply-author-styles false, stop.
>       - check all custom pseudo styles in the current shadow dom tree,
>       - walk up to an shadow dom tree which encloses the current one.
> 
> e.g.
> #shadow-root
>    <style> div::x-foo { ... }
>    #shadow-root (apply-author-styles: true)
>        #shadow-root
>            div::x-foo <--- apply
> 
> Best regards,
> Takashi Sakamoto

[Takashi Sakamoto]

(In reply to comment #6)
> (In reply to comment #3)
> > My patch is still "WIP", however I would like to ask feedbacks. I'm afraid that I'm on the wrong way to fix this issue.
> > 
> > What I'm trying to do is:
> > - custom pseudo in UA stylesheets
> >    --> should check whether elements in any treescope match or not.
> > 
> > e.g.
> > html.css:
> > progress:-webkit-progress-value { ... }
> > 
> > #shadow-root
> >     #shadow-root
> >        div::-webkit-progress-value <--- always apply above rule
> 
> I am assuming that by "div::-webkit-progress-value" you mean "div[pseudo=-webkit-progress-value]". Right?

Yes, right. Sorry for writing ambiguous things.

> In this case, yes. (naturally, only if the host of the tree in which the div is located has tag name "progress")
> 
> I apologize, I didn't have a chance to look at this over the weekend, but I'll attack this Monday! :)

Thank you!

So I updated my ambiguous diagrams...

> > - custom pseudo in an author stylesheet 
> >    --> should check whether elements in any child treescopes of document and in some treescopes whose parent treescopes have apply-author-styles true. If so, try to apply. Otherwise, ignore.
> > 
> > e.g.
> > document:
> > <style>
> > div::x-foo { ... }
> > </style>
> > 
> > #shadow-root
> >    div::x-foo <--- apply
> >    #shadow-root
> >        #shadow-root
> >            div::x-foo <--- not apply

#document:
<style>
div::x-foo { ... }
</style>

div
  #shadow-root
      div[pseudo=x-foo] <--- apply
...
      div
          #shadow-root
             div
                 #shadow-root
                     div[pseudo=x-foo] <--- not apply


With apply-author-styles,

#document:

div
  #shadow-root
      <style>
          div::x-foo { ... }
      </style>
...
      div
          #shadow-root (apply-author-styles: true)
             div
                 #shadow-root (apply-author-styles: false)
                     div[pseudo=x-foo] <--- apply

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

I missed the following case, reported by layout test: shadow-netsted-pseudo-id.html:

p::x-shadow-child::x-nested-shadow-child {
  background-color: green;
}

So I think, we need scopeOfLastElement in SelectorChecker, introduced by hayato@ in https://bugs.webkit.org/show_bug.cgi?id=82169,

#if ENABLE(SHADOW_DOM)
        if (context.scopeOfLastElement) {
            // FIXME: Check the scope more strictly.                            
            // Obtained from https://bugs.webkit.org/attachment.cgi?id=176892   
            // Should wait till the original bug is fixed.                      
            return context.scopeOfLastElement->treeScope() == context.element->\
treeScope() ? SelectorMatches : SelectorFailsLocally;
        }
#endif

I'm thinking of:
- for author stylesheets (not scoped), set scopeOfLastElement document. So I can fix:

New failing tests:
fast/dom/shadow/shadow-nested-pseudo-id.html

I will update the patch which uses hayato@'s code.

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

Created attachment 179709 [details]
Patch

[Build Bot]

Comment on attachment 179709 [details]
Patch

Attachment 179709 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15378623

[Dimitri Glazkov (Google)]

Comment on attachment 179709 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:641
> +void StyleResolver::collectMatchingRulesForPseudo(RuleSet* rules, int& firstRuleIndex, int& lastRuleIndex, const MatchOptions& options)

collectMatchingPseudoElementRules

[Dimitri Glazkov (Google)]

Comment on attachment 179709 [details]
Patch

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

> Source/WebCore/css/StyleScopeResolver.cpp:161
> +    applyAuthorStyles = false;
> +    // m_stackParentBoundsIndex + 1 doesn't match any items in m_stack.
> +    if (element->shadowPseudoId().isEmpty())
> +        return m_stackParentBoundsIndex + 1;

This seems like it could just be moved outside of this function.

> Source/WebCore/css/StyleScopeResolver.cpp:167
> +    for (; parent && !parent->isShadowRoot(); parent = parent->parentOrHostNode()) { }

Why can't we just go parentTreeScope = m_stackParent->treeScope()?

> Source/WebCore/css/StyleScopeResolver.cpp:174
> +    return toShadowRoot(parent)->applyAuthorStyles() ? m_stackParentBoundsIndex : m_stackParentBoundsIndex - 1;

So it's always either at the end of the stack or just preceding it?

[Dimitri Glazkov (Google)]

Comment on attachment 179709 [details]
Patch

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

> Source/WebCore/css/StyleResolver.h:375
> +    bool checkSelector(const RuleData&, const ContainerNode* scope, const ContainerNode* scopeOfLastElement);

This is nasty :-\

[Dimitri Glazkov (Google)]

Comment on attachment 179709 [details]
Patch

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

> Source/WebCore/css/SelectorChecker.cpp:481
> +            return context.scopeOfLastElement->treeScope() == context.element->treeScope() ? SelectorMatches : SelectorFailsLocally;

Why are we doing this in checkSelector? I should know this way above (at the level of a rule) whether the tree scope will match. This is wrong.

[Takashi Sakamoto]

Comment on attachment 179709 [details]
Patch

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

Thank you for reviewing.

>> Source/WebCore/css/SelectorChecker.cpp:481
>> +            return context.scopeOfLastElement->treeScope() == context.element->treeScope() ? SelectorMatches : SelectorFailsLocally;
> 
> Why are we doing this in checkSelector? I should know this way above (at the level of a rule) whether the tree scope will match. This is wrong.

I wrote the document about this:
https://docs.google.com/document/d/1yu-zVrNAO79JFEbnfjI20yrfZ428l3hqKf82k5CU69Q/edit

>> Source/WebCore/css/StyleResolver.cpp:641
>> +void StyleResolver::collectMatchingRulesForPseudo(RuleSet* rules, int& firstRuleIndex, int& lastRuleIndex, const MatchOptions& options)
> 
> collectMatchingPseudoElementRules

Done.

>> Source/WebCore/css/StyleResolver.h:375
>> +    bool checkSelector(const RuleData&, const ContainerNode* scope, const ContainerNode* scopeOfLastElement);
> 
> This is nasty :-\

I see. I checked to use "const MatchOptions&".

>> Source/WebCore/css/StyleScopeResolver.cpp:161
>> +        return m_stackParentBoundsIndex + 1;
> 
> This seems like it could just be moved outside of this function.

Done.

>> Source/WebCore/css/StyleScopeResolver.cpp:167
>> +    for (; parent && !parent->isShadowRoot(); parent = parent->parentOrHostNode()) { }
> 
> Why can't we just go parentTreeScope = m_stackParent->treeScope()?

Done.

>> Source/WebCore/css/StyleScopeResolver.cpp:174
>> +    return toShadowRoot(parent)->applyAuthorStyles() ? m_stackParentBoundsIndex : m_stackParentBoundsIndex - 1;
> 
> So it's always either at the end of the stack or just preceding it?

Yeah.  m_stackParentBoundsIndex always have m_stackParent's shadow root's bounds index. So,

- If m_stackParent's shadow root has apply-author-styles true, the parent tree scope of m_stackParent has the same bounds index.
- Otherwise, since ++m_stackParentBoundsIndex when we find a shadow root whose apply-author-styles is false, m_stackParentBoundsIndex-1 is the same as the parent tree scope's bounds index.

https://docs.google.com/document/d/1HRlJdq6kIFWKHzXL4uDSspsCmnHyiGxwwVbROpySrOg/edit

[Takashi Sakamoto]

Created attachment 179881 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 179881 [details]
Patch

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

> Source/WebCore/css/StyleScopeResolver.cpp:154
> +int StyleScopeResolver::findStyleBoundsOfParentTreeScope(const Element* element, bool& applyAuthorStyles) const

You don't need element arg anymore.

[Dimitri Glazkov (Google)]

(In reply to comment #15)

> I wrote the document about this:
> https://docs.google.com/document/d/1yu-zVrNAO79JFEbnfjI20yrfZ428l3hqKf82k5CU69Q/edit

I read the doc. It seems that you've arrived at the same conclusion as I have (in solution 1) -- the number of pseudo-elements should be stored with StyleRule (since it's known at parsing and does not change for each style recalc), and certainly not re-determined for each checkSelector.

That stored number of pseudo-elements is exactly the increment to determine the scope on which to match elements.

Or am I still missing something?

> 
> I see. I checked to use "const MatchOptions&".

The reason why I thought it was nasty is because this does not belong in checking selector. This information is known well before we get to individual selector checking. We can discard an entire rule simply based on the number of pseudo-elements it contains.

[Takashi Sakamoto]

Created attachment 180078 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #18)
> (In reply to comment #15)
> 
> > I wrote the document about this:
> > https://docs.google.com/document/d/1yu-zVrNAO79JFEbnfjI20yrfZ428l3hqKf82k5CU69Q/edit
> 
> I read the doc. It seems that you've arrived at the same conclusion as I have (in solution 1) -- the number of pseudo-elements should be stored with StyleRule (since it's known at parsing and does not change for each style recalc), and certainly not re-determined for each checkSelector.
> 
> That stored number of pseudo-elements is exactly the increment to determine the scope on which to match elements.
> 
> Or am I still missing something?

Yeah. For fixing only this issue, I agree that we don't need solution2 (=lastScopeOfElement).  I forgot to add the reason why we need solution2, not solution1 to the document.

The idea, lastScopeOfElement comes from https://bugs.webkit.org/show_bug.cgi?id=82169, "implement /select/ reference combinator". To implement the function, we will need solution2, because number of /select/ doesn't provide any information about how many treescopes we have to check.

So I think, it would be better to re-use lastScopeOfElement for fixing this issue. If this reason is weak, I will add 82169 to "Depends on" and will wait till hayato-san's patch is landed.

> > I see. I checked to use "const MatchOptions&".
> 
> The reason why I thought it was nasty is because this does not belong in checking selector. This information is known well before we get to individual selector checking. We can discard an entire rule simply based on the number of pseudo-elements it contains.

The changes about checkSelector(...)  also comes from https://bugs.webkit.org/show_bug.cgi?id=82169.

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

Comment on attachment 179881 [details]
Patch

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

>> Source/WebCore/css/StyleScopeResolver.cpp:154
>> +int StyleScopeResolver::findStyleBoundsOfParentTreeScope(const Element* element, bool& applyAuthorStyles) const
> 
> You don't need element arg anymore.

Thank you. I missed this.

[Dimitri Glazkov (Google)]

(In reply to comment #20)
> (In reply to comment #18)
> > (In reply to comment #15)
> > 
> > > I wrote the document about this:
> > > https://docs.google.com/document/d/1yu-zVrNAO79JFEbnfjI20yrfZ428l3hqKf82k5CU69Q/edit
> > 
> > I read the doc. It seems that you've arrived at the same conclusion as I have (in solution 1) -- the number of pseudo-elements should be stored with StyleRule (since it's known at parsing and does not change for each style recalc), and certainly not re-determined for each checkSelector.
> > 
> > That stored number of pseudo-elements is exactly the increment to determine the scope on which to match elements.
> > 
> > Or am I still missing something?
> 
> Yeah. For fixing only this issue, I agree that we don't need solution2 (=lastScopeOfElement).  I forgot to add the reason why we need solution2, not solution1 to the document.
> 
> The idea, lastScopeOfElement comes from https://bugs.webkit.org/show_bug.cgi?id=82169, "implement /select/ reference combinator". To implement the function, we will need solution2, because number of /select/ doesn't provide any information about how many treescopes we have to check.
> 
> So I think, it would be better to re-use lastScopeOfElement for fixing this issue. If this reason is weak, I will add 82169 to "Depends on" and will wait till hayato-san's patch is landed.

That patch won't ever land, since the syntax is changing. Also, I don't think this is the right approach anyway. Let's not do this. Please.

[Takashi Sakamoto]

(In reply to comment #22)
> (In reply to comment #20)
> > (In reply to comment #18)
> > > (In reply to comment #15)
> > > 
> > > > I wrote the document about this:
> > > > https://docs.google.com/document/d/1yu-zVrNAO79JFEbnfjI20yrfZ428l3hqKf82k5CU69Q/edit
> > > 
> > > I read the doc. It seems that you've arrived at the same conclusion as I have (in solution 1) -- the number of pseudo-elements should be stored with StyleRule (since it's known at parsing and does not change for each style recalc), and certainly not re-determined for each checkSelector.
> > > 
> > > That stored number of pseudo-elements is exactly the increment to determine the scope on which to match elements.
> > > 
> > > Or am I still missing something?
> > 
> > Yeah. For fixing only this issue, I agree that we don't need solution2 (=lastScopeOfElement).  I forgot to add the reason why we need solution2, not solution1 to the document.
> > 
> > The idea, lastScopeOfElement comes from https://bugs.webkit.org/show_bug.cgi?id=82169, "implement /select/ reference combinator". To implement the function, we will need solution2, because number of /select/ doesn't provide any information about how many treescopes we have to check.
> > 
> > So I think, it would be better to re-use lastScopeOfElement for fixing this issue. If this reason is weak, I will add 82169 to "Depends on" and will wait till hayato-san's patch is landed.
> 
> That patch won't ever land, since the syntax is changing. Also, I don't think this is the right approach anyway. Let's not do this. Please.

I know. I'm not sure whether hayato-san will re-use the existing bug or file a new bug for the new syntax. However his new patch for the new syntax still requires lastScopeOfElement as far as I know.

Best regards,
Takashi Sakamoto

[Dominic Cooney]

Because dglazkov prefers the counting solution, and because bug 82169 is blocked on the CSS4 selectors spec, let’s use the counting approach to unblock this work. Let’s abandon "solution 2".

[Takashi Sakamoto]

(In reply to comment #24)
> Because dglazkov prefers the counting solution, and because bug 82169 is blocked on the CSS4 selectors spec, let’s use the counting approach to unblock this work. Let’s abandon "solution 2".

However if we have some css selector which has both :distributed and x-???, "solution 1"(i.e. couting) doesn't work.
After :distributed patch is landed, we will have to remove "solution 1" code and to re-add "solution 2".

Is this good approach? If so, I will implement "solution 1" and wait for ":distributed".

Best regards,
Takashi Sakamoto

[Dominic Cooney]

Comment on attachment 180078 [details]
Patch

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

Some nits inline. I am having a hard time following the code change because I am not very familiar with selector checking.

> LayoutTests/fast/dom/shadow/pseudo-style-in-shadow.html:11
> +var host = document.getElementById("host");

I think it is neater to use ' for JavaScript string literals. That way you use the same quotes (") for attributes both in markup, as on line 4, and markup in JavaScript string literals, as on line 13.

Line 31, etc. will also be much cleaner if you use '.

> LayoutTests/fast/dom/shadow/pseudo-style-in-shadow.html:30
> +debug("Case1: A pseudo style is in the same treescope as div with x-foo pseudo.");

It would be typical to put a space after "Case".

> LayoutTests/fast/dom/shadow/pseudo-style-in-shadow.html:67
> +</html>

What about successfullyParsed and including js-test-post.js?

[Dominic Cooney]

Comment on attachment 180078 [details]
Patch

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

> LayoutTests/fast/dom/shadow/pseudo-style-in-shadow.html:1
> +<!doctype html>

You should add tests with multiple shadow roots and reprojection through <shadow> and <content> as well as nesting.

[Dominic Cooney]

(In reply to comment #25)
> … if we have some css selector which has both :distributed and x-???, "solution 1"(i.e. couting) doesn't work.

My intuition is that CSS selectors should work compositionally, and anywhere they don’t is a bad sign!

The reason you think counting does not work is because given a rule R:

A::distributed(::x-foo) { … }

The scope associated with R (A’s scope) has no countable relationship to the scope of ::x-foo because of reprojection.

However can’t you simply treat the subselector ::distributed(…) is matching as a separate selector? Set the count to zero, and interpret relative to the scope of the current element (ie wherever ::distributed tried to start matching?)

[Takashi Sakamoto]

(In reply to comment #28)
> (In reply to comment #25)
> > … if we have some css selector which has both :distributed and x-???, "solution 1"(i.e. couting) doesn't work.
> 
> My intuition is that CSS selectors should work compositionally, and anywhere they don’t is a bad sign!
> 
> The reason you think counting does not work is because given a rule R:
> 
> A::distributed(::x-foo) { … }
> 
> The scope associated with R (A’s scope) has no countable relationship to the scope of ::x-foo because of reprojection.
> 
> However can’t you simply treat the subselector ::distributed(…) is matching as a separate selector? Set the count to zero, and interpret relative to the scope of the current element (ie wherever ::distributed tried to start matching?)

I found that we can ignore all custom pseudo in ::distributed. So talking about the above example,
we don't need to check "count". However, if we have

   C::x-foobar > A::x-bar::distributed(B::x-foo)

we have to count all custom pseudo outside of ::distributed, e.g. 2 (x-foobar and x-bar), in CSSParser.

... however after ::distributed patch is landed, we will have lastScopeOfElement.
I don't know any good reason why we have to use both "count" and "lastScopeOfElement" at the same time or why we have to select "count" solution or "lastScopeOfElement" solution in StyleResolver (or somewhere). Just we can set lastScopeOfElement as the scope of matched rules.

Best regards,
Takashi Sakamoto






Since all selector matches from right to left order, firstly we have to check whether the given element matches ::x-foo or not. Next, we will see ::distributed.

[Takashi Sakamoto]

(In reply to comment #29)

I'm sorry. I forgot to delete the following sentence. Please ignore this.

> Since all selector matches from right to left order, firstly we have to check whether the given element matches ::x-foo or not. Next, we will see ::distributed.

[Takashi Sakamoto]

Created attachment 181890 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 180078 [details]
Patch

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

Thank you for reviewing.

>> LayoutTests/fast/dom/shadow/pseudo-style-in-shadow.html:1
>> +<!doctype html>
> 
> You should add tests with multiple shadow roots and reprojection through <shadow> and <content> as well as nesting.

I see. Done.

>> LayoutTests/fast/dom/shadow/pseudo-style-in-shadow.html:11
>> +var host = document.getElementById("host");
> 
> I think it is neater to use ' for JavaScript string literals. That way you use the same quotes (") for attributes both in markup, as on line 4, and markup in JavaScript string literals, as on line 13.
> 
> Line 31, etc. will also be much cleaner if you use '.

Done.

>> LayoutTests/fast/dom/shadow/pseudo-style-in-shadow.html:30
>> +debug("Case1: A pseudo style is in the same treescope as div with x-foo pseudo.");
> 
> It would be typical to put a space after "Case".

Done.

>> LayoutTests/fast/dom/shadow/pseudo-style-in-shadow.html:67
>> +</html>
> 
> What about successfullyParsed and including js-test-post.js?

Done.

[Dimitri Glazkov (Google)]

Comment on attachment 181890 [details]
Patch

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

The vision of the actual architecture eludes me, but all this new code makes me wonder why we make a distinction between scoped styles and document styles. After all, they are simply styles scoped at the document. I think this is where the path to the right architecture lies.

> Source/WebCore/css/StyleResolver.cpp:752
> +        bool elementHasShadowPseudo = !m_element->shadowPseudoId().isEmpty();

This is a bad smell. We do this earlier in collectMatchingPseudoElementRules, and this logic looks like duplication, but not quite.

[Takashi Sakamoto]

Comment on attachment 181890 [details]
Patch

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

>> Source/WebCore/css/StyleResolver.cpp:752
>> +        bool elementHasShadowPseudo = !m_element->shadowPseudoId().isEmpty();
> 
> This is a bad smell. We do this earlier in collectMatchingPseudoElementRules, and this logic looks like duplication, but not quite.

So... do you mean we have to always invoke collectMatchingPseudoElementRules even if a given element has no pseudo id?

[Takashi Sakamoto]

Comment on attachment 181890 [details]
Patch

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

>>> Source/WebCore/css/StyleResolver.cpp:752
>>> +        bool elementHasShadowPseudo = !m_element->shadowPseudoId().isEmpty();
>> 
>> This is a bad smell. We do this earlier in collectMatchingPseudoElementRules, and this logic looks like duplication, but not quite.
> 
> So... do you mean we have to always invoke collectMatchingPseudoElementRules even if a given element has no pseudo id?

I mean, if possible, I would like to do in the following way:

if (hasPseudoId) {
   // loop with collectMatchingPseudoElementRules
   for ( ... ) {
      // ...
      collectMatchingPseudoElementRuels(...);
      ...
      if (documnetScope && ... ) {
         collectMatchingRules(...);
         ...
      }
   }
} else {
   // loop without collectMatchingPseudoElementRules
   for ( ... ) {
      // This is the same as the code before applying this patch.
   }
}

[Takashi Sakamoto]

Created attachment 182106 [details]
WIP

[Takashi Sakamoto]

I tried to make bad layout tests, which look like fuzzer, better.

Best regards,
Takashi Sakamoto

[Ryosuke Niwa]

This bug is no longer relevant since the old shadow DOM implementation has been replaced by a new one.