83446 – Broken handling of pseudo-elements in selectors API

RESOLVED FIXED 83446

Broken handling of pseudo-elements in selectors API

https://bugs.webkit.org/show_bug.cgi?id=83446

Summary Broken handling of pseudo-elements in selectors API

Boris Zbarsky

Reported 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.

Attachments
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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Boris Zbarsky

Comment 1 2012-04-08 18:30:49 PDT

Created attachment 136168 [details] Testcase

Arpita Bahuguna

Comment 2 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.

Arpita Bahuguna

Comment 3 2012-05-02 00:17:38 PDT

Created attachment 139758 [details] Patch

Arpita Bahuguna

Comment 4 2012-05-02 00:18:39 PDT

Proposed patch uploaded with the first approach mentioned in the previous comment.

Arpita Bahuguna

Comment 5 2012-05-02 04:04:18 PDT

Created attachment 139786 [details] Patch

Arpita Bahuguna

Comment 6 2012-05-02 04:07:28 PDT

Added another patch with slight modifications.

Antti Koivisto

Comment 7 2012-05-02 04:17:30 PDT

Comment on attachment 139786 [details] Patch 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.

Arpita Bahuguna

Comment 8 2012-05-03 04:36:48 PDT

Created attachment 139984 [details] Patch

Arpita Bahuguna

Comment 9 2012-05-03 04:52:40 PDT

Thank-you for the review Antti. Have uploaded another patch incorporating the review comments.

Antti Koivisto

Comment 10 2012-05-03 04:58:28 PDT

Comment on attachment 139984 [details] Patch 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

Arpita Bahuguna

Comment 11 2012-05-03 05:34:25 PDT

Created attachment 139989 [details] Patch

Antti Koivisto

Comment 12 2012-05-03 05:37:09 PDT

Comment on attachment 139989 [details] Patch r=me

Arpita Bahuguna

Comment 13 2012-05-03 06:50:59 PDT

Comment on attachment 139989 [details] Patch Thanks for the review Antti.

WebKit Review Bot

Comment 14 2012-05-03 07:44:29 PDT

Comment on attachment 139989 [details] Patch Clearing flags on attachment: 139989 Committed r115971: <http://trac.webkit.org/changeset/115971>

WebKit Review Bot

Comment 15 2012-05-03 07:44:41 PDT

All reviewed patches have been landed. Closing bug.

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

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component CSS

Assignee

Nobody

Reported

2012-04-08 18:25 PDT

Modified

2012-05-03 07:44 PDT History

CC List

11 users Show

URL

Keywords

Depends on

Blocks