Bug 72933 – "Raw" pseudo selectors don't match if immediately after a child or descendant combinator

• Component: CSS
• Status: RESOLVED FIXED

[Roland Steiner]

"Raw" pseudo selectors, i.e., those that aren't preceded by anything within their compound selector (that is, having an implicit universal selector), will fail if they follow a child or descendant combinator. For examples:

div ::-webkit-meter-bar
div > ::-webkit-meter-bar

[see http://crbug.com/97744 for context]

[Roland Steiner]

Within a "larger" compound selector, they will have "ShadowDescendant" set as their relation, which handles the crossing of the shadow boundary.

However, in the "raw" case, the relation is "Child" or "Descendant", and there is no code to handle crossing over.

[Roland Steiner]

On further thought, this will also fail with direct and indirect sibling selectors:

span + ::-webkit-meter-bar
span ~ ::-webkit-meter-bar

[Roland Steiner]

Created attachment 116192 [details]
work-in-progress

work-in-progress patch. This just outlines the raw logic, but the implementation is very fugly! I need to figure out how to pass the necessary information between checkOneSelector (that it matched a pseudo-ID selector) and checkSelector (what to do in this case depending on combinators).

[Roland Steiner]

Created attachment 116195 [details]
work-in-progress #2

A bit more sensible work-in-progress patch. This has quite a bit of code duplication, though. However, the code IS subtly different, and at least it compartmentalizes the issue nicely (IMHO). Currently fails 2 tests because of styles crossing into the shadow that shouldn't.

[Roland Steiner]

Addendum: If I don't remove matchesInTreeScope(), 2 different tests fail, because that incorrectly blocks them.

[Roland Steiner]

Created attachment 116199 [details]
work-in-progress #2b

Things work better if one doesn't spuriously remove source lines... Tests with :hover after the pseudo-ID selector still don't work, however.

[Roland Steiner]

Created attachment 116481 [details]
Patch

New patch, handling pseudo-classes the same way as checkSelector(). (This looks like it can be optimized further.)

[Roland Steiner]

(Adding Darin, as this is related to http://trac.webkit.org/changeset/89742)

[Darin Adler]

Comment on attachment 116481 [details]
Patch

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

I’m going to say review+ but there are many style issues here so it’s a borderline case.

I think this is quite a bit of copied and pasted code. Is there a way we can avoid that?

> Source/WebCore/css/CSSStyleSelector.cpp:1799
> +    CSSSelector* sel = ruleData.selector();

Please use words like “selector” rather than abbreviations like “sel”.

> Source/WebCore/css/SelectorChecker.cpp:539
> +SelectorChecker::SelectorMatch SelectorChecker::checkPseudoIDSelector(
> +    CSSSelector* sel,
> +    Element* e,
> +    PseudoId& dynamicPseudo,
> +    VisitedMatchType visitedMatchType,
> +    RenderStyle* elementStyle,
> +    RenderStyle* elementParentStyle) const

Is this large new function a copy of the checkSelector function? If so, can we figure out a way to refactor so we don’t need to copy the entire function?

Please don’t use this “one argument per line” formatting in WebKit code since it’s not our normal style.

Please use words like “selector” and “element” rather than abbreviations like “sel” and “e” unless you are preserving existing code.

The name “dynamic pseudo” is missing a noun. Perhaps “dynamic pseudo ID” would be a better name?

> Source/WebCore/css/SelectorChecker.cpp:584
> +            e = host;
> +            for (;;) {
> +                ContainerNode* n = e->parentNode();
> +                if (!n || !n->isElementNode())
> +                    return SelectorFailsCompletely;
> +                e = static_cast<Element*>(n);
> +                SelectorMatch match = checkSelector(sel, e, dynamicPseudo, false, visitedMatchType);
> +                if (match != SelectorFailsLocally)
> +                    return match;
> +            }

This could be written more cleanly with the parentElement function.

    while ((e = e->parentElement())

Or written as a for loop (see an example below).

> Source/WebCore/css/SelectorChecker.cpp:588
> +        case CSSSelector::Child:
> +            {

Our normal style puts the brace on the same line as the : instead of the next line.

> Source/WebCore/css/SelectorChecker.cpp:592
> +                ContainerNode* n = host->parentNode();
> +                if (!n || !n->isElementNode())
> +                    return SelectorFailsCompletely;
> +                e = toElement(n);

Again, parentElement() is the best way to write this.

> Source/WebCore/css/SelectorChecker.cpp:610
> +                Node* n = host->previousSibling();
> +                while (n && !n->isElementNode())
> +                    n = n->previousSibling();
> +                if (!n)
> +                    return SelectorFailsCompletely;
> +                e = static_cast<Element*>(n);

This could just be:

    e = host->previousElementSibling();
    if (!e)
        return SelectorFailsCompletely;

> Source/WebCore/css/SelectorChecker.cpp:621
> +            if (!m_isCollectingRulesOnly && host->parentNode() && host->parentNode()->isElementNode()) {
> +                RenderStyle* parentStyle = host->parentNode()->renderStyle();
> +                if (parentStyle)
> +                    parentStyle->setChildrenAffectedByForwardPositionalRules();
> +            }

This code, repeated twice, should probably go into a helper function.

> Source/WebCore/css/SelectorChecker.cpp:634
> +            e = host;
> +            while (true) {
> +                Node* n = e->previousSibling();
> +                while (n && !n->isElementNode())
> +                    n = n->previousSibling();
> +                if (!n)
> +                    return SelectorFailsCompletely;
> +                e = static_cast<Element*>(n);
> +                SelectorMatch match = checkSelector(sel, e, dynamicPseudo, false, visitedMatchType);
> +                if (match != SelectorFailsLocally)
> +                    return match;
> +            };
> +            return SelectorFailsCompletely;

There is a stray semicolon here, but also this can be written as a conventional loop:

    element = host;
    while ((element = element->previousElementSibling())) {
        SelectorMatch match = checkSelector(selector, element, dynamicPseudo, false, visitedMatchType);
        if (match != SelectorFailsLocally)
            return match;
    }

Or it could be written as a for loop:

    for (element = host->previousElementSibling(); element; element = element->previousElementSibling()

> Source/WebCore/css/SelectorChecker.cpp:643
> +            // A selector is invalid if something follows a pseudo-element
> +            // We make an exception for scrollbar pseudo elements and allow a set of pseudo classes (but nothing else)
> +            // to follow the pseudo elements.
> +            if ((elementStyle || m_isCollectingRulesOnly)
> +                && dynamicPseudo != NOPSEUDO
> +                && dynamicPseudo != SELECTION
> +                && !((RenderScrollbar::scrollbarForStyleResolve() || dynamicPseudo == SCROLLBAR_CORNER || dynamicPseudo == RESIZER) && sel->m_match == CSSSelector::PseudoClass))

This rule is hard to read in a single expression like this. I suggest we put it in a named helper function.

[Antti Koivisto]

Comment on attachment 116481 [details]
Patch

I'm not sure I understand why fixing a case involving some webkit internal use selectors is important at all. It almost certainly shouldn't be fixed by adding an entirely new selector checker code path. Rather you'll want to setup the selector structures so that you can use the existing code path. I don't quite understand what is special about the universal selectors here. 

I'll set this to r- for now.

[Roland Steiner]

Created attachment 117325 [details]
patch, alternative solution

[Roland Steiner]

(In reply to comment #10)
> (From update of attachment 116481 [details])
> I'm not sure I understand why fixing a case involving some webkit internal use selectors is important at all. It almost certainly shouldn't be fixed by adding an entirely new selector checker code path. Rather you'll want to setup the selector structures so that you can use the existing code path. I don't quite understand what is special about the universal selectors here. 

The problem is the switch from the shadow tree to the host node. Currently this is handled by the CSSSelector::ShadowDescendant combinator. However, that doesn't get set if the pseudo-element isn't directly preceded by anything.

My previous attempt at solving this was to handle all such pseudo-elements explicitly. While very similar to the "regular" matching code, there are quite a few subtle differences that I felt a separate function is the best way to go.

However, given your comment, I uploaded a patch for an alternate solution that doesn't prevent the creation of an explicit universal selector before shadow pseudo-elements. The change is certainly smaller, but I'm not 100% sure it's a better solution - it effectively relies on 'foo ::bar' being rewritten to 'foo *::bar' for shadow pseudo-elements.

[WebKit Review Bot]

Comment on attachment 117325 [details]
patch, alternative solution

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

New failing tests:
fast/css/css-selector-text.html
fast/css/css-set-selector-text.html

[Roland Steiner]

Removed r? due to failing selector serialization tests - the '*' part of the selector created with the patch does not match the expectations (the test assumes universal selectors are suppressed).

Fixing that would require hacking the serialization code to remove the '*' on serialization or updating the tests. Both options strike me as somewhat fugly. Any suggestions?

[Antti Koivisto]

The patch is clearly much preferable to the first one. As I understand, either solution is fine since the serializations are equivalent.

[Roland Steiner]

Created attachment 117565 [details]
patch, v2

patch using the 2nd solution, suppressing '*' in front of shadow-ID pseudo-elements on serialization, extended test cases

[Antti Koivisto]

Comment on attachment 117565 [details]
patch, v2

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

> Source/WebCore/css/CSSParser.cpp:7677
> +    // Note that CSSSelector::ShadowDescendant needs to be used.
> +    // Therefore we create a new Selector here in any case, even if matching any (host) element in any namespace.

I have difficulties understanding this comment (especially if I don't see the old removed text).

> Source/WebCore/css/CSSSelector.cpp:637
> +            if (tagHistoryText.endsWith("*"))
> +                tagHistoryText.truncate(tagHistoryText.length() - 1);

It would be nicer not to add the * in the first place (or just leave it) but ok.

[Roland Steiner]

Created attachment 117826 [details]
patch v2, don't suppress '*'

Not generating the '*' in the first place is not trivial, but just leaving them may indeed be the better option, as it better represents the internal state and should so be easier for debugging purposes. Uploaded a new patch accordingly. (r? again, since it did substantially change).

[WebKit Review Bot]

Comment on attachment 117826 [details]
patch v2, don't suppress '*'

Clearing flags on attachment: 117826

Committed r101998: <http://trac.webkit.org/changeset/101998>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.