Bug 83446 - Broken handling of pseudo-elements in selectors API
: Broken handling of pseudo-elements in selectors API
: WebKit
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
  Show dependency treegraph
Reported: 2012-04-08 18:25 PST by
Modified: 2012-05-03 07:44 PST (History)

Testcase (145 bytes, text/html)
2012-04-08 18:30 PST, Boris Zbarsky
no flags Details
Patch (15.76 KB, patch)
2012-05-02 00:17 PST, Arpita Bahuguna
no flags Review Patch | Details | Formatted Diff | Diff
Patch (15.82 KB, patch)
2012-05-02 04:04 PST, Arpita Bahuguna
no flags Review Patch | Details | Formatted Diff | Diff
Patch (18.62 KB, patch)
2012-05-03 04:36 PST, Arpita Bahuguna
no flags Review Patch | Details | Formatted Diff | Diff
Patch (18.03 KB, patch)
2012-05-03 05:34 PST, Arpita Bahuguna
no flags Review Patch | Details | Formatted Diff | Diff


You need to log in before you can comment on or make changes to this bug.

Description From 2012-04-08 18:25:24 PST
The attached testcase should say "PASS".  In WebKit it says "FAIL".

Specifically, there are no elements in the document which match the given selector, but the WebKit implementation of Selectors API seems to ignore the pseudo-element altogether and return a match set containing the root <html> element.

Note that the selectors API has an explicit informative note about this, in addition to the normative text that says this selector should lead to an empty list:

  Authors are advised that while the use of pseudo-elements in selectors is permitted, they will
  not match any elements in the document, and thus would not result in any elements being returned.
------- Comment #1 From 2012-04-08 18:30:49 PST -------
Created an attachment (id=136168) [details]
------- Comment #2 From 2012-04-27 04:26:16 PST -------
The issue:
The SelectorChecker::checkSelector() method is common for both setting and retrieving the rules. For differentiating between the same m_isCollectingRulesOnly (member of SelectorChecker) is used. This boolean is set when querying for the rules i.e. whenever APIs such as getMatchedCSSRules() or querySelectorAll() are called. Thus in the checkOneSelector() function we are able to skip the code that tries to set flags etc. at the time of setting the renderStyle for renderers.

Now, when querying for pseudo-element selectors via the querySelectorAll() API, checkOneSelector() method returns a true irrespective of whether the element really has the specified selector applied on it or not.
While handling the pseudo-element in checkOneSelector() we do an initial check [if (!context.elementStyle && !m_isCollectingRulesOnly)] but since we set m_isCollectingRuleOnly to true (SelectorQuery constructor) we actually fall-through thereby returning a true.
The check for !m_isCollectingRulesOnly is required for the case when the call comes from CSSStyleSelector::pseudoStyleRulesForElement() [the getMatchedCSSRules() API], in which case we don't want it to return false immediately.

The possible approaches:
1. Instead of maintaining the m_isCollectingRulesOnly flag we can rather have an enumeration that associates the checkSelector() call with its originating context. For instance, instead of setting m_isCollectingRulesOnly whenever a query rules call is made (i.e. from either SelectorQuery or CSSStyleSelector::pseudoStyleRulesForElement()), we can instead set the corresponding enum based on which we can either skip checking for pseudo-elements (querySelector) and return a false from checkOneSelector() method or continue (CSSStyleSelector::pseudoStyleRulesForElement() - getMatchedCSSRules()).

2. In the templated SelectorDataList::execute() method itself (called from SelectorDataList::queryAll()/queryFirst() alone) we can skip making the selectorChecker.checkSelector() call altogether if the selector queried for is a pseudo-element selector.

3. Overloading the SelectorChecker::checkSelector() method and passing an extra boolean parameter (default - false) which is set only when called from the SelectorDataList::execute() method.

Would really appreciate if someone could review the aforementioned scenarios and recommend the best approach for fixing this issue.
In my opinion, having an enum that associates the checkSelector() call with its originating context would be more appropriate.
------- Comment #3 From 2012-05-02 00:17:38 PST -------
Created an attachment (id=139758) [details]
------- Comment #4 From 2012-05-02 00:18:39 PST -------
Proposed patch uploaded with the first approach mentioned in the previous comment.
------- Comment #5 From 2012-05-02 04:04:18 PST -------
Created an attachment (id=139786) [details]
------- Comment #6 From 2012-05-02 04:07:28 PST -------
Added another patch with slight modifications.
------- Comment #7 From 2012-05-02 04:17:30 PST -------
(From update of attachment 139786 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=139786&action=review

> Source/WebCore/css/SelectorChecker.cpp:776
> +                if (SettingRules == m_selectorCheckerMode) {

Putting the constant first looks strange.

> Source/WebCore/css/SelectorChecker.h:101
> +    void setCollectingRulesOnly(bool b) { b ? m_selectorCheckerMode = CollectingRules : m_selectorCheckerMode = SettingRules; }
> +    void setQueryingRulesOnly() { m_selectorCheckerMode = QueryingRules; }

Abuse of ternary operator. You should just replace these boolean setters with one that takes the mode enum.

> Source/WebCore/css/SelectorChecker.h:139
> +    enum SelectorCheckerMode { SettingRules = 0, CollectingRules, QueryingRules };

The name SettingRules makes no sense. We are never setting any rules. ResolvingStyle perhaps.

SelectorChecker::SelectorCheckerMode is redundant. This should be just Mode.
------- Comment #8 From 2012-05-03 04:36:48 PST -------
Created an attachment (id=139984) [details]
------- Comment #9 From 2012-05-03 04:52:40 PST -------
Thank-you for the review Antti. Have uploaded another patch incorporating the review comments.
------- Comment #10 From 2012-05-03 04:58:28 PST -------
(From update of attachment 139984 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=139984&action=review

r-, I 'd still like to see another version with style fixes.

> Source/WebCore/css/SelectorChecker.cpp:529
> +        if ((context.elementStyle || m_selectorCheckerMode != ResolvingStyleRules) && dynamicPseudo != NOPSEUDO && dynamicPseudo != SELECTION

I think you should explicitly test against the modes that you want the behavior in instead of using !=

> Source/WebCore/css/SelectorChecker.h:55
> +    enum Mode { ResolvingStyleRules = 0, CollectingRules, QueryingRules };

I  think my proposal ResolvingStyle was better. We don't really resolve style rules, we resolve (element) style.

> Source/WebCore/css/SelectorChecker.h:101
> +    Mode selectorCheckerMode() const { return m_selectorCheckerMode; }
> +    void setSelectorCheckerMode(Mode mode) { m_selectorCheckerMode = mode; }

SelectorChecker::selectorCheckerMode and SelectorChecker::m_selectorCheckerMode are similarly redundant. -> mode(), m_mode
------- Comment #11 From 2012-05-03 05:34:25 PST -------
Created an attachment (id=139989) [details]
------- Comment #12 From 2012-05-03 05:37:09 PST -------
(From update of attachment 139989 [details])
------- Comment #13 From 2012-05-03 06:50:59 PST -------
(From update of attachment 139989 [details])
Thanks for the review Antti.
------- Comment #14 From 2012-05-03 07:44:29 PST -------
(From update of attachment 139989 [details])
Clearing flags on attachment: 139989

Committed r115971: <http://trac.webkit.org/changeset/115971>
------- Comment #15 From 2012-05-03 07:44:41 PST -------
All reviewed patches have been landed.  Closing bug.