Bug 78595 – CollectingRules and QueryingRules modes of SelectorChecker miss some complex selectors with pseudo elements

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

[Pavel Feldman]

Consider the case below:

<style>
.section .header::before {
  content: "COLLAPSED"
}
.section.expanded .header::before {
  content: "EXPANDED",
}
</style>
<div class="section">
  <div class="header">HEADER</div>
</div>
<div class="section expanded">
  <div class="header">HEADER</div>
</div>

While WebCore renders everything correctly, pseudo element is missing for the second header.

[Pavel Feldman]

(ignore the "," - that was a typo, not relevant to the bug described)

<style>
.section .header::before {
  content: "COLLAPSED"
}
.section.expanded .header::before {
  content: "EXPANDED"
}
</style>
<div class="section">
  <div class="header">HEADER</div>
</div>
<div class="section expanded">
  <div class="header">HEADER</div>
</div>

[Pavel Feldman]

Created attachment 126970 [details]
[TEST] Test case

[Alexander Pavlov (apavlov)]

Root cause summary: SelectorChecker::checkSelector() operates on the same dynamicPseudo reference when recursively calling self for non-SubSelector selectors, even though dynamicPseudo passed in from StyleResolver::checkSelector() is intended to get the value of the last selector in the list (describing the element to which the selector is applicable).

[Alexander Pavlov (apavlov)]

Created attachment 165622 [details]
Patch

[WebKit Review Bot]

Comment on attachment 165622 [details]
Patch

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

New failing tests:
fast/dom/Window/getMatchedCSSRules-with-pseudo-elements-complex.html

[Build Bot]

Comment on attachment 165622 [details]
Patch

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

New failing tests:
fast/dom/Window/getMatchedCSSRules-with-pseudo-elements-complex.html

[Alexander Pavlov (apavlov)]

Created attachment 165740 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 165740 [details]
Patch

This fix will look slightly nicer after bug 96445 lands.

[Alexander Pavlov (apavlov)]

(In reply to comment #8)
> (From update of attachment 165740 [details])
> This fix will look slightly nicer after bug 96445 lands.

Indeed! Waiting for it to land.

[Antti Koivisto]

Comment on attachment 165740 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Do not use the same dynamicPseudo reference when recursively invoking checkSelector() for non-SubSelector selectors.

Why does this work (or does it?) in ResolvingStyle case?

> Source/WebCore/css/SelectorChecker.cpp:476
> +    PseudoId nonSubSelectorPseudo(NOPSEUDO);
> +    PseudoId& currentDynamicPseudo = relation == CSSSelector::SubSelector ? dynamicPseudo : nonSubSelectorPseudo;

That looks bit funny but I suppose it is ok.

[Alexander Pavlov (apavlov)]

(In reply to comment #10)
> (From update of attachment 165740 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165740&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Do not use the same dynamicPseudo reference when recursively invoking checkSelector() for non-SubSelector selectors.
> 
> Why does this work (or does it?) in ResolvingStyle case?

It works in the ResolvingStyle case because the only failing code so far is found in "case CSSSelector::SubSelector" inside SelectorChecker::checkSelector(), and the bail-out condition involves the "m_mode == CollectingRules || m_mode == QueryingRules" check. ResolvingStyle case is not in this list, so the code does not bail out during the style resolve.

> > Source/WebCore/css/SelectorChecker.cpp:476
> > +    PseudoId nonSubSelectorPseudo(NOPSEUDO);
> > +    PseudoId& currentDynamicPseudo = relation == CSSSelector::SubSelector ? dynamicPseudo : nonSubSelectorPseudo;
> 
> That looks bit funny but I suppose it is ok.

I know it's actually ugly :) but I did not want to migrate the entire code to pointers...

[Alexander Pavlov (apavlov)]

Committed r129746: <http://trac.webkit.org/changeset/129746>