Bug 138214 – CSS4 Selectors: Add multiple pseudo elements support to :matches

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

CSS4 Selectors: Add multiple pseudo elements support to :matches

[Yusuke Suzuki]

Created attachment 240681 [details]
Patch

Added initial patch. In this patch, we support it in SelectorChecker, and makes CannotCompile on CSS JIT. I'll add the tests

[Yusuke Suzuki]

Created attachment 240682 [details]
Patch

rev2

[Yusuke Suzuki]

Created attachment 240683 [details]
Patch

rev3

[Yusuke Suzuki]

Created attachment 240791 [details]
Patch

rev3

[Yusuke Suzuki]

Created attachment 240817 [details]
Patch

rev4

[Yusuke Suzuki]

I'll add tests.

[Yusuke Suzuki]

Created attachment 240819 [details]
Patch

rev5

[Benjamin Poulain]

Comment on attachment 240819 [details]
Patch

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

The patch is brilliant. I did not find any correctness issue.

I have some minor comments. I r- because it is complex change, I'd like a chance to do a second round.

> Source/WebCore/ChangeLog:11
> +        * css/SelectorChecker.cpp:

It's about time you add your copyright on this one :)

> Source/WebCore/css/SelectorChecker.cpp:182
> +    if (context.pseudoId != NOPSEUDO && !(pseudoIdSet & (1 << context.pseudoId)))

I would make a nice little function to get the PseudoIdSet corresponding to a PseudoId, that seems useful.

Maybe even a constructor to PseudoIdSet taking PseudoId and use stronger typing for PseudoIdSet?

> Source/WebCore/css/SelectorChecker.cpp:232
>          // In Selectors Level 4, a pseudo element inside a functional pseudo class is undefined (issue 7).
> -        // Make it as matching failure until the spec clarifies this case.
> -        if (context.inFunctionalPseudoClass)
> -            return SelectorFailsCompletely;
> +        // Make it as matching failure until the spec clarifies this case, except for :matches.
> +        // In WebKit, we specially handles pseudo-elements in :matches.

Let's remove the comments.

I have talked about your patch with Tab (the spec editor), the spec will be updated.

> Source/WebCore/css/SelectorChecker.cpp:289
> +                return MatchResult::update(result, matchToNonPseudoElement);

"update" is a bit generic, I am not sure what would make a good name... :(

> Source/WebCore/css/SelectorChecker.cpp:688
> +                    PseudoIdSet localDynamicPseudoIdSet = NOPSEUDO;

I am not a fan of mixing PseudoIdSet and PseudoId (NOPSEUDO).

I think readers might get confused, localDynamicPseudoIdSet = 0 may be better.

> Source/WebCore/css/SelectorChecker.cpp:693
> +                            // When pseudo elements are not effective in this fragment (e.g. it's not righmost fragment),
> +                            // it's not necessary to check all selectors to collect pseudo element ids.

If I am not mistaken, we can assert that localDynamicPseudoIdSet is still 0/NOPSEUDO here?

If we matched a virtual pseudo element in a non-rightmost selector, something is wrong.

> Source/WebCore/css/SelectorChecker.h:52
> +    enum class MatchToNonPseudoElement {
> +        Matches,
> +        Fails,
> +    };

Since this is a subproperty of Match, maybe this should be a match type instead?
enum class MatchType {
    VirtualPseudoElementOnly,
    Element
}

> Source/WebCore/css/SelectorChecker.h:63
> +            return {
> +                Match::SelectorMatches,
> +                matchToNonPseudoElement
> +            };

I would put this on one line, it is simple enough.

> Source/WebCore/css/SelectorChecker.h:64
> +

Extra blank line.

> Source/WebCore/css/SelectorChecker.h:79
> +            return {
> +                match,
> +                MatchToNonPseudoElement::Fails
> +            };

I'd use one line here too.

> Source/WebCore/cssjit/SelectorCompiler.cpp:375
> +static FunctionType constructFragments(const CSSSelector* rootSelector, SelectorContext, SelectorFragmentList& selectorFragments, FragmentsLevel, FragmentPositionInRootFragments, bool visitedMatchEnabled, VisitedMode&, bool pseudoElementEffective);

Here we could improve readability with an enumeration.

What do you think of?:
enum class PseudoElementMatchingBehavior {
    CanMatch,
    NeverMatch
}

> Source/WebCore/cssjit/SelectorCompiler.cpp:887
>              // In Selectors Level 4, a pseudo element inside a functional pseudo class is undefined (issue 7).
>              // Make it as matching failure until the spec clarifies this case.

Let's remove the comment here too.

> Source/WebCore/cssjit/SelectorCompiler.cpp:888
> -            if (fragmentLevel == FragmentsLevel::InFunctionalPseudoType)
> +            if (fragmentLevel == FragmentsLevel::InFunctionalPseudoType && !pseudoElementEffective)

Wouldn't it be enough to have:
    if (!pseudoElementEffective)
?

> Source/WebCore/rendering/style/RenderStyle.h:239
> +            ASSERT(pseudo != NOPSEUDO);

Is it a bit odd to compare a PseudoIdSet with a PseudoId. Maybe this should be ASSERT(pseudo)?

> Source/WebCore/rendering/style/RenderStyle.h:241
> +            static_assert(pseudoBitsOffset >= 1, "(pseudoBitsOffset - 1) should be valid.");

Nice addition.

> LayoutTests/ChangeLog:9
> +        * fast/selectors/pseudo-element-inside-matches-expected.html: Added.
> +        * fast/selectors/pseudo-element-inside-matches.html: Added.

Smart way to test this.

I like that you included a test for each of the big 4.

[Yusuke Suzuki]

Comment on attachment 240819 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        * css/SelectorChecker.cpp:
> 
> It's about time you add your copyright on this one :)

Yay. I've added :)

>> Source/WebCore/css/SelectorChecker.cpp:182
>> +    if (context.pseudoId != NOPSEUDO && !(pseudoIdSet & (1 << context.pseudoId)))
> 
> I would make a nice little function to get the PseudoIdSet corresponding to a PseudoId, that seems useful.
> 
> Maybe even a constructor to PseudoIdSet taking PseudoId and use stronger typing for PseudoIdSet?

Introducing stronger typing sounds nice.
I'll wrap the unsigned flags with class and provides some methods, `has`, `add`, `merge`.

>> Source/WebCore/css/SelectorChecker.cpp:232
>> +        // In WebKit, we specially handles pseudo-elements in :matches.
> 
> Let's remove the comments.
> 
> I have talked about your patch with Tab (the spec editor), the spec will be updated.

Yeah! I'll drop this comment :)

> Source/WebCore/css/SelectorChecker.cpp:233
> +        if (context.inFunctionalPseudoClass && !context.pseudoElementEffective)

Makes it to `if (!context.pseudoElementEffective)`

>> Source/WebCore/css/SelectorChecker.cpp:289
>> +                return MatchResult::update(result, matchToNonPseudoElement);
> 
> "update" is a bit generic, I am not sure what would make a good name... :(

What do you think of changing it to `updateWithMatchType`?

>> Source/WebCore/css/SelectorChecker.cpp:688
>> +                    PseudoIdSet localDynamicPseudoIdSet = NOPSEUDO;
> 
> I am not a fan of mixing PseudoIdSet and PseudoId (NOPSEUDO).
> 
> I think readers might get confused, localDynamicPseudoIdSet = 0 may be better.

Looks nice. I'll rewrite it.

>> Source/WebCore/css/SelectorChecker.cpp:693
>> +                            // it's not necessary to check all selectors to collect pseudo element ids.
> 
> If I am not mistaken, we can assert that localDynamicPseudoIdSet is still 0/NOPSEUDO here?
> 
> If we matched a virtual pseudo element in a non-rightmost selector, something is wrong.

Previously, SelectorChecker simply ignores the virtual pseudo elements selectors inside non-rightmost fragments.
So here, we may see pseudo elements. In that case, we ignore it.

But what do you think of simply making pseudo-elements inside non-rightmost fragment unmatched?

>> Source/WebCore/css/SelectorChecker.h:52
>> +    };
> 
> Since this is a subproperty of Match, maybe this should be a match type instead?
> enum class MatchType {
>     VirtualPseudoElementOnly,
>     Element
> }

Sounds nice. Your suggestion clarify the meaning of this property :)
And since it states the property's job clearly, I think the above comment becomes necessary. I've removed it.

>> Source/WebCore/css/SelectorChecker.h:63
>> +            };
> 
> I would put this on one line, it is simple enough.

Fixed.

>> Source/WebCore/css/SelectorChecker.h:64
>> +
> 
> Extra blank line.

Oops. Thank you.

>> Source/WebCore/css/SelectorChecker.h:79
>> +            };
> 
> I'd use one line here too.

Yes, done :)

>> Source/WebCore/cssjit/SelectorCompiler.cpp:375
>> +static FunctionType constructFragments(const CSSSelector* rootSelector, SelectorContext, SelectorFragmentList& selectorFragments, FragmentsLevel, FragmentPositionInRootFragments, bool visitedMatchEnabled, VisitedMode&, bool pseudoElementEffective);
> 
> Here we could improve readability with an enumeration.
> 
> What do you think of?:
> enum class PseudoElementMatchingBehavior {
>     CanMatch,
>     NeverMatch
> }

Looks nice. It improves readability very much :)

>> Source/WebCore/cssjit/SelectorCompiler.cpp:888
>> +            if (fragmentLevel == FragmentsLevel::InFunctionalPseudoType && !pseudoElementEffective)
> 
> Wouldn't it be enough to have:
>     if (!pseudoElementEffective)
> ?

Yes. We can do so. I'll change it :)

>> Source/WebCore/rendering/style/RenderStyle.h:239
>> +            ASSERT(pseudo != NOPSEUDO);
> 
> Is it a bit odd to compare a PseudoIdSet with a PseudoId. Maybe this should be ASSERT(pseudo)?

Yes. I'll change it :)

[Benjamin Poulain]

(In reply to comment #9)
> >> Source/WebCore/css/SelectorChecker.cpp:289
> >> +                return MatchResult::update(result, matchToNonPseudoElement);
> > 
> > "update" is a bit generic, I am not sure what would make a good name... :(
> 
> What do you think of changing it to `updateWithMatchType`?

Yep, that's better.

> >> Source/WebCore/css/SelectorChecker.cpp:693
> >> +                            // it's not necessary to check all selectors to collect pseudo element ids.
> > 
> > If I am not mistaken, we can assert that localDynamicPseudoIdSet is still 0/NOPSEUDO here?
> > 
> > If we matched a virtual pseudo element in a non-rightmost selector, something is wrong.
> 
> Previously, SelectorChecker simply ignores the virtual pseudo elements
> selectors inside non-rightmost fragments.
> So here, we may see pseudo elements. In that case, we ignore it.
> 
> But what do you think of simply making pseudo-elements inside non-rightmost
> fragment unmatched?

For virtual pseudo element, failing a match is indeed the correct thing to do.

For custom pseudo elements, they can appear anywhere in the selector (for example video::-webkit-media-controls-panel button for styling the shadow tree of <video>).

Do we not handle this correctly today? I think this code
    if (relation != CSSSelector::SubSelector) {
        // Bail-out if this selector is irrelevant for the pseudoId
        if (context.pseudoId != NOPSEUDO && context.pseudoId != dynamicPseudo)
            return SelectorFailsCompletely;
was supposed to take care of the problem.

In any case, we should make sure we have good test coverage for this.

[Yusuke Suzuki]

Created attachment 241167 [details]
Patch

rev6

[Yusuke Suzuki]

Created attachment 241169 [details]
Patch

rev7

[Yusuke Suzuki]

Comment on attachment 241169 [details]
Patch

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

Updated the patch :)

> Source/WebCore/ChangeLog:13
> +        framgnet, it is ignored. This patch changes it to unmatched.

In this patch, I've changed the pseudo element semantics.
Previously, if there's pseudo element in the non-rightmost fragment, it simply ignored.
This patch changes it to unmatched. What do you think about it?

> Source/WebCore/css/SelectorChecker.cpp:197
> +    PseudoIdSet scrollbarIdSet = { SCROLLBAR, SCROLLBAR_THUMB, SCROLLBAR_BUTTON, SCROLLBAR_TRACK, SCROLLBAR_TRACK_PIECE, SCROLLBAR_CORNER };

Constructing PseudoIdSet with initializer_list :)

> Source/WebCore/css/SelectorChecker.cpp:275
> +        // Virtual pseudo element is only effective in the rightmost fragment.

Make pseudoElementEffective false when the fragment is not rightmost fragment.

> Source/WebCore/css/SelectorChecker.cpp:693
> +                            ASSERT(!localDynamicPseudoIdSet);

Added assertion. It is now ensured.

> Source/WebCore/css/SelectorChecker.h:59
> +        static MatchResult updateWithMatchType(MatchResult result, MatchType matchType)

I've renamed it to updatedWithMatchType.

> Source/WebCore/cssjit/SelectorCompiler.cpp:966
> +        pseudoElementMatchingBehavior = PseudoElementMatchingBehavior::NeverMatch;

Make the pseudoElementMatchingBehavior NeverMatch when it is in the non-rightmost fragment.

> Source/WebCore/cssjit/SelectorCompiler.cpp:-3560
> -    }

This path is dropped, since we make the selector CannotMatchAnything when we found the pseudo element in the non-rightmost fragment.

> Source/WebCore/cssjit/SelectorCompiler.cpp:3562
> +    ASSERT_WITH_MESSAGE(fragmentMatchesTheRightmostElement(m_selectorContext, fragment), "Virtual pseudo elements handling is only effective in the rightmost fragment. If the current fragment is not rightmost fragment, CSS JIT compiler makes it CannotMatchAnything in fragment construction phase, so never reach here.");

Added assertion.

> Source/WebCore/rendering/style/RenderStyleConstants.h:85
> +class PseudoIdSet {

Introduce strongly typed PseudoIdSet.
It supports several operations, "has", "add", "merge", "&", "|".

> Source/WebCore/rendering/style/RenderStyleConstants.h:128
> +    explicit operator bool() const

Leverage C++11 explicit bool conversion operator.
It allows
    if (pseudoIdSet) ...;
    ASSERT(pseudoIdSet);
But it makes the following compile error :)
    unsigned value = pseudoIdSet;
    PseudoId pseudoId = pseudoIdSet;

> LayoutTests/fast/selectors/pseudo-element-inside-matches-expected.html:71
> +    <p id="target14"><p>non-rightmost pseudo element has no effect.</p></p>

Added :matches(non-rightmost pseudo elements) cases.

[Yusuke Suzuki]

Created attachment 241174 [details]
Patch

rev8, fix unused parameter warnings

[Yusuke Suzuki]

Rev8: Use ASSERT_UNUSED / ASSERT_WITH_MESSAGE_UNUSED to suppress unused parameter warnings.

[Yusuke Suzuki]

(In reply to comment #10)
> > >> Source/WebCore/css/SelectorChecker.cpp:693
> > >> +                            // it's not necessary to check all selectors to collect pseudo element ids.
> > > 
> > > If I am not mistaken, we can assert that localDynamicPseudoIdSet is still 0/NOPSEUDO here?
> > > 
> > > If we matched a virtual pseudo element in a non-rightmost selector, something is wrong.
> > 
> > Previously, SelectorChecker simply ignores the virtual pseudo elements
> > selectors inside non-rightmost fragments.
> > So here, we may see pseudo elements. In that case, we ignore it.
> > 
> > But what do you think of simply making pseudo-elements inside non-rightmost
> > fragment unmatched?
> 
> For virtual pseudo element, failing a match is indeed the correct thing to
> do.

Agreed. I'll take this semantics.

> For custom pseudo elements, they can appear anywhere in the selector (for
> example video::-webkit-media-controls-panel button for styling the shadow
> tree of <video>).

Yup. Need to handle it.
And I'll add comment to SelectorCompiler to clarify handling custom pseudo elements.

> Do we not handle this correctly today? I think this code
>     if (relation != CSSSelector::SubSelector) {
>         // Bail-out if this selector is irrelevant for the pseudoId
>         if (context.pseudoId != NOPSEUDO && context.pseudoId !=
> dynamicPseudo)
>             return SelectorFailsCompletely;

I think it's currently handled in the code,

    if ((!context.elementStyle && context.resolvingMode == Mode::ResolvingStyle) || context.resolvingMode == Mode::QueryingRules)
        return MatchResult::fails(Match::SelectorFailsLocally);

In this code, context.elementStyle becomes nullptr if the context is in a non-rightmost fragment.
When resolvingMode is ResolvingStyle or QueryingRules, virtual pseudo elements in a non-rightmost fragment are ignored.
So when viewing the html,

<!DOCTYPE HTML>
<html lang="en">
<head>
	<meta charset="UTF-8">
	<title></title>
    <style>
    .OK::after span {
        background-color: red;
    }
    </style>
</head>
<body>
<p class="OK"><span>Hello World</span></p>
</body>
</html>

Collecting rules for span collects `.OK::after span` rule(We can see it in the inspector). Is it an expected behavior?

And I think that `if (context.pseudoId != NOPSEUDO && context.pseudoId != dynamicPseudo)` meets when "span::selection::before" selector case.
Is it correct? Need to investigate more...


> 
> In any case, we should make sure we have good test coverage for this.

Oops. I need to care it.

[Yusuke Suzuki]

Created attachment 241183 [details]
Patch

rev9, fix care for custom pseudo elements. Need to add tests.

[Benjamin Poulain]

(In reply to comment #16)
> > Do we not handle this correctly today? I think this code
> >     if (relation != CSSSelector::SubSelector) {
> >         // Bail-out if this selector is irrelevant for the pseudoId
> >         if (context.pseudoId != NOPSEUDO && context.pseudoId !=
> > dynamicPseudo)
> >             return SelectorFailsCompletely;
> 
> I think it's currently handled in the code,
> 
>     if ((!context.elementStyle && context.resolvingMode ==
> Mode::ResolvingStyle) || context.resolvingMode == Mode::QueryingRules)
>         return MatchResult::fails(Match::SelectorFailsLocally);

Right.

> In this code, context.elementStyle becomes nullptr if the context is in a
> non-rightmost fragment.
> When resolvingMode is ResolvingStyle or QueryingRules, virtual pseudo
> elements in a non-rightmost fragment are ignored.
> So when viewing the html,
> 
> <!DOCTYPE HTML>
> <html lang="en">
> <head>
> 	<meta charset="UTF-8">
> 	<title></title>
>     <style>
>     .OK::after span {
>         background-color: red;
>     }
>     </style>
> </head>
> <body>
> <p class="OK"><span>Hello World</span></p>
> </body>
> </html>
> 
> Collecting rules for span collects `.OK::after span` rule(We can see it in
> the inspector). Is it an expected behavior?

Damn, that looks like a bug :(
Matching such rule makes no sense.

The bug should not affect styling because the selector would never match when resolving style.

The problem are
-The rule is considered in the ruleset for no reason.
-It will be shown incorrectly in the inspector.

> And I think that `if (context.pseudoId != NOPSEUDO && context.pseudoId !=
> dynamicPseudo)` meets when "span::selection::before" selector case.
> Is it correct? Need to investigate more...

I was thinking of style resolution in my comment.

When resolving the style for ::after, context.pseudoId would be set and ".OK::after span" would fail.
But when collecting rules, context.pseudoId must NOPSEUDO and nothing protects the invalid case.

[Yusuke Suzuki]

Created attachment 241231 [details]
Patch

rev10

[Yusuke Suzuki]

Comment on attachment 241231 [details]
Patch

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

Thank you for your comment, Benjamin.
I've updated the patch. In this patch,
+ I take care for custom pseudo elements.
+ Disabling virtual pseudo elements inside non-rightmost fragments completely (even if collecting rules)

> Source/WebCore/css/SelectorChecker.cpp:231
>          if (context.selector->isCustomPseudoElement()) {

We take care for custom pseudo elements inside non-rightmost fragment; custom pseudo element inside non-rightmost fragment is effective.
But I cannot come into my mind about any good test cases for this...
What do you think about this, Benjamin?
It seems that selectors such as "video::-webkit-media-controls-panel > *" don't work well now.

> Source/WebCore/css/SelectorChecker.cpp:234
> +                return MatchResult::fails(Match::SelectorFailsCompletely);

Inside functional pseudo class, disable custom pseudo elements in the meantime.

> Source/WebCore/css/SelectorChecker.cpp:244
> +            if (!context.pseudoElementEffective)

Disable virtual pseudo element matching completely when pseudoElementEffective is false.

> Source/WebCore/css/SelectorChecker.cpp:279
> +        nextContext.pseudoElementEffective = false;

Make pseudoElementEffective false when evaluating non-rightmost fragments.

> Source/WebCore/rendering/style/RenderStyleConstants.h:85
> +class PseudoIdSet {

Introduce strongly typed PseudoIdSet.

> Source/WebCore/rendering/style/RenderStyleConstants.h:128
> +    explicit operator bool() const

Use C++11 explicit bool conversion operator.

> LayoutTests/fast/selectors/ignore-pseudo-element-inside-non-rightmost-fragments.html:31
> +shouldBe("window.getMatchedCSSRules(document.getElementById('testcase2'))", "null");

Ensure the selector that has virtual pseudo element inside non-rightmost fragment doesn't match when collecting rules.

[Yusuke Suzuki]

Created attachment 241271 [details]
Patch

rev11, fix merge conflicts

[Yusuke Suzuki]

(In reply to comment #18)
> I was thinking of style resolution in my comment.
> 
> When resolving the style for ::after, context.pseudoId would be set and
> ".OK::after span" would fail.
> But when collecting rules, context.pseudoId must NOPSEUDO and nothing
> protects the invalid case.

Ah, I've misunderstood.
I considered that selector matching with pseudo element request only occurs when the target node is marked with the requested pseudo elements.
But we can explicitly query selector matching with pseudo element request even if the target node doesn't have pseudo elements.

[Benjamin Poulain]

(In reply to comment #20)
> Comment on attachment 241231 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241231&action=review
> 
> Thank you for your comment, Benjamin.
> I've updated the patch. In this patch,
> + I take care for custom pseudo elements.
> + Disabling virtual pseudo elements inside non-rightmost fragments
> completely (even if collecting rules)
> 
> > Source/WebCore/css/SelectorChecker.cpp:231
> >          if (context.selector->isCustomPseudoElement()) {
> 
> We take care for custom pseudo elements inside non-rightmost fragment;
> custom pseudo element inside non-rightmost fragment is effective.
> But I cannot come into my mind about any good test cases for this...
> What do you think about this, Benjamin?
> It seems that selectors such as "video::-webkit-media-controls-panel > *"
> don't work well now.

Hum, yeah. The problem is those pseudo elements inside the shadow DOM can only be styled through the user agent stylesheet.

I don't think we have the right infrastructure to test that. On the other hand, the impact of a mistake is much lower than for the other cases.

[Benjamin Poulain]

Comment on attachment 241271 [details]
Patch

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

Some minor issues below.

My only real concern is "if (!context.inFunctionalPseudoClass)" in the isCustomPseudoElement() branch.

> Source/WebCore/ChangeLog:12
> +        Previously when pseudo element selector appear in the non-rightmost

appear -> appeared

> Source/WebCore/ChangeLog:13
> +        framgnet, it is ignored. This patch changes it to unmatched.

typo: framgnet

"is ignored" -> "was ignored"

> Source/WebCore/css/SelectorChecker.cpp:236
> +            if (!context.inFunctionalPseudoClass)
> +                return MatchResult::fails(Match::SelectorFailsCompletely);

This does not look right. This always fail if not in a functional pseudo class.

Shouldn't it be "if (context.inFunctionalPseudoClass)"?

At some point we should just drop this condition entirely for :matches(). Sure, that's just for us internally but people love :matches().

> Source/WebCore/css/SelectorChecker.cpp:291
> +            MatchResult result = matchRecursively(nextContext, ignoreDynamicPseudo);

Should this assert !ignoreDynamicPseudo? We should get a failure if we have one right?

> Source/WebCore/css/SelectorChecker.cpp:305
> +            MatchResult result = matchRecursively(nextContext, ignoreDynamicPseudo);

ditto

> Source/WebCore/css/SelectorChecker.cpp:325
> +            return MatchResult::updateWithMatchType(matchRecursively(nextContext, ignoreDynamicPseudo), matchType);

ditto

> Source/WebCore/css/SelectorChecker.cpp:338
> +            MatchResult result = matchRecursively(nextContext, ignoreDynamicPseudo);

ditto

> Source/WebCore/rendering/style/RenderStyleConstants.h:85
> +class PseudoIdSet {

Very neat little helper.

> Source/WebCore/rendering/style/RenderStyleConstants.h:89
> +    { }

For functions, we generally use
    {
    }
instead of {}.

> Source/WebCore/rendering/style/RenderStyleConstants.h:110
> +        m_data |= (1 << pseudoId);

We should probably assert (1 << pseudoId) does not overflow m_data's size.

> Source/WebCore/rendering/style/RenderStyleConstants.h:137
> +    { }

Style:
    {
    }

> LayoutTests/fast/selectors/ignore-pseudo-element-inside-non-rightmost-fragments.html:31
> +shouldBe("window.getMatchedCSSRules(document.getElementById('testcase1'))", "null");
> +shouldBe("window.getMatchedCSSRules(document.getElementById('testcase2'))", "null");

This could be extended to test getMatchedCSSRules() with the pseudo elements "before", "after", etc.

> LayoutTests/fast/selectors/pseudo-element-inside-matches.html:68
> +#target14:matches(::after) > p{

Missing space before the curly bracket.

> LayoutTests/fast/selectors/pseudo-element-inside-matches.html:72
> +#target15:matches(::after > p) > p{

ditto

[Yusuke Suzuki]

Comment on attachment 241271 [details]
Patch

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

Thank you for your great review! I've fixed and updated soon :)

>> Source/WebCore/ChangeLog:12
>> +        Previously when pseudo element selector appear in the non-rightmost
> 
> appear -> appeared

Oops. Thank you!

>> Source/WebCore/ChangeLog:13
>> +        framgnet, it is ignored. This patch changes it to unmatched.
> 
> typo: framgnet
> 
> "is ignored" -> "was ignored"

done.

>> Source/WebCore/css/SelectorChecker.cpp:236
>> +                return MatchResult::fails(Match::SelectorFailsCompletely);
> 
> This does not look right. This always fail if not in a functional pseudo class.
> 
> Shouldn't it be "if (context.inFunctionalPseudoClass)"?
> 
> At some point we should just drop this condition entirely for :matches(). Sure, that's just for us internally but people love :matches().

Right! Thank you. I've mis-coded.

And yes,it should be just dropped. I've added FIXME comment here.

>> Source/WebCore/css/SelectorChecker.cpp:291
>> +            MatchResult result = matchRecursively(nextContext, ignoreDynamicPseudo);
> 
> Should this assert !ignoreDynamicPseudo? We should get a failure if we have one right?

Yes. When reaching here, `nextContext.pseudoElementEffective` should be `false`.
So `ignoreDynamicPseudo` should be empty.
I've inserted `ASSERT(!nextContext.pseudoElementEffective && !ignoreDynamicPseudo)`.

>> Source/WebCore/css/SelectorChecker.cpp:305
>> +            MatchResult result = matchRecursively(nextContext, ignoreDynamicPseudo);
> 
> ditto

done.

>> Source/WebCore/css/SelectorChecker.cpp:325
>> +            return MatchResult::updateWithMatchType(matchRecursively(nextContext, ignoreDynamicPseudo), matchType);
> 
> ditto

done.

>> Source/WebCore/css/SelectorChecker.cpp:338
>> +            MatchResult result = matchRecursively(nextContext, ignoreDynamicPseudo);
> 
> ditto

done.

>> Source/WebCore/rendering/style/RenderStyleConstants.h:89
>> +    { }
> 
> For functions, we generally use
>     {
>     }
> instead of {}.

Thanks. fixed :)

>> Source/WebCore/rendering/style/RenderStyleConstants.h:110
>> +        m_data |= (1 << pseudoId);
> 
> We should probably assert (1 << pseudoId) does not overflow m_data's size.

Sounds nice.
I've inserted `ASSERT((sizeof(m_data) * 8) > pseudoId)`

>> Source/WebCore/rendering/style/RenderStyleConstants.h:137
>> +    { }
> 
> Style:
>     {
>     }

done.

>> LayoutTests/fast/selectors/ignore-pseudo-element-inside-non-rightmost-fragments.html:31
>> +shouldBe("window.getMatchedCSSRules(document.getElementById('testcase2'))", "null");
> 
> This could be extended to test getMatchedCSSRules() with the pseudo elements "before", "after", etc.

Sounds very nice! I've added them :)

>> LayoutTests/fast/selectors/pseudo-element-inside-matches.html:68
>> +#target14:matches(::after) > p{
> 
> Missing space before the curly bracket.

done.

>> LayoutTests/fast/selectors/pseudo-element-inside-matches.html:72
>> +#target15:matches(::after > p) > p{
> 
> ditto

done.

[Yusuke Suzuki]

Created attachment 241342 [details]
Patch

rev12

[Benjamin Poulain]

Comment on attachment 241342 [details]
Patch

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

Only minor comments. Let's do it!

> Source/WebCore/css/SelectorChecker.cpp:199
> +    PseudoIdSet scrollbarIdSet = { SCROLLBAR, SCROLLBAR_THUMB, SCROLLBAR_BUTTON, SCROLLBAR_TRACK, SCROLLBAR_TRACK_PIECE, SCROLLBAR_CORNER };

This is just incredibly elegant. It is really neat what we can do with C++11.

> Source/WebCore/css/SelectorChecker.cpp:570
> +                // Since :not cannot contain pseudo elements, there's no effect on matchedToNonPseudoElement.
> +                MatchType ignoreMatchType = MatchType::Element;

Looks like stale code.

The comment mention "matchedToNonPseudoElement". "ignoreMatchType" is not used.

> Source/WebCore/css/SelectorChecker.cpp:703
> +                            ASSERT(!localDynamicPseudoIdSet);
> +                            return true;

Shouldn't you reset matchType to MatchType::Element in this case?

Otherwise wouldn't it make "return context.resolvingMode == Mode::CollectingRulesIgnoringVirtualPseudoElements || result.matchType == MatchType::Element;" fail?
If that's the case, let's add a test for it :)

[Yusuke Suzuki]

Comment on attachment 241342 [details]
Patch

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

Great! Thank you for your detailed review. I'll fix and land it :)

>> Source/WebCore/css/SelectorChecker.cpp:570
>> +                MatchType ignoreMatchType = MatchType::Element;
> 
> Looks like stale code.
> 
> The comment mention "matchedToNonPseudoElement". "ignoreMatchType" is not used.

Oops. I've missed to take care of !CSS_SELECTOR_LEVEL4 case.
The comment should be, "Since :not cannot contain pseudo elements, there's no effect on matchType".
And I've replaced ignoreMatchedToNonPseudoElement to ignoreMatchType.

>> Source/WebCore/css/SelectorChecker.cpp:703
>> +                            return true;
> 
> Shouldn't you reset matchType to MatchType::Element in this case?
> 
> Otherwise wouldn't it make "return context.resolvingMode == Mode::CollectingRulesIgnoringVirtualPseudoElements || result.matchType == MatchType::Element;" fail?
> If that's the case, let's add a test for it :)

Oh thank you! You're completely right.
To make the flow clearly, I've introduced localMatchType and assign localMatchType to matchType only when the result becomes true.
And I've introduced tests for this.

[Yusuke Suzuki]

Committed r175889: <http://trac.webkit.org/changeset/175889>

[Elliott Sprehn]

This patch violates the spec, pseudo elements are not supposed to be valid in :matches:
http://dev.w3.org/csswg/selectors-4/#matches