Bug 83446 - Broken handling of pseudo-elements in selectors API
: Broken handling of pseudo-elements in selectors API
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
Depends on:
  Show dependency treegraph
Reported: 2012-04-08 18:25 PDT by Boris Zbarsky
Modified: 2012-05-03 07:44 PDT (History)
11 users (show)

See Also:

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

Note You need to log in before you can comment on or make changes to this bug.
Description Boris Zbarsky 2012-04-08 18:25:24 PDT
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 Boris Zbarsky 2012-04-08 18:30:49 PDT
Created attachment 136168 [details]
Comment 2 Arpita Bahuguna 2012-04-27 04:26:16 PDT
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 Arpita Bahuguna 2012-05-02 00:17:38 PDT
Created attachment 139758 [details]
Comment 4 Arpita Bahuguna 2012-05-02 00:18:39 PDT
Proposed patch uploaded with the first approach mentioned in the previous comment.
Comment 5 Arpita Bahuguna 2012-05-02 04:04:18 PDT
Created attachment 139786 [details]
Comment 6 Arpita Bahuguna 2012-05-02 04:07:28 PDT
Added another patch with slight modifications.
Comment 7 Antti Koivisto 2012-05-02 04:17:30 PDT
Comment on 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 Arpita Bahuguna 2012-05-03 04:36:48 PDT
Created attachment 139984 [details]
Comment 9 Arpita Bahuguna 2012-05-03 04:52:40 PDT
Thank-you for the review Antti. Have uploaded another patch incorporating the review comments.
Comment 10 Antti Koivisto 2012-05-03 04:58:28 PDT
Comment on 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 Arpita Bahuguna 2012-05-03 05:34:25 PDT
Created attachment 139989 [details]
Comment 12 Antti Koivisto 2012-05-03 05:37:09 PDT
Comment on attachment 139989 [details]

Comment 13 Arpita Bahuguna 2012-05-03 06:50:59 PDT
Comment on attachment 139989 [details]

Thanks for the review Antti.
Comment 14 WebKit Review Bot 2012-05-03 07:44:29 PDT
Comment on attachment 139989 [details]

Clearing flags on attachment: 139989

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