Bug 137348 – CSS Selectors Level 4: Add parsing for :matches

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

CSS Selectors Level 4: Add parsing for :matches

[Yusuke Suzuki]

ref:
http://dev.w3.org/csswg/selectors4/#matches
http://dev.w3.org/csswg/selectors4/#selector-list

:matches(selector list)

[Yusuke Suzuki]

Created attachment 239133 [details]
Patch

[Yusuke Suzuki]

Created attachment 239139 [details]
Patch

[Benjamin Poulain]

Comment on attachment 239139 [details]
Patch

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

Exciting!

> Source/WebCore/ChangeLog:11
> +        Curretnly :not doesn't accept any functional pseudo classes, :not(:matches(...)) is rejected.

Typo: Curretnly

> Source/WebCore/ChangeLog:12
> +        And currently, :matches(:visited, :link) is allowed in the parsing phase.

That's okay, our specialized behavior can be at runtime.

> Source/WebCore/css/CSSSelector.cpp:450
> +                const CSSSelector* firstSubSelector = cs->selectorList()->first();
> +                for (const CSSSelector* subSelector = firstSubSelector; subSelector; subSelector = CSSSelectorList::next(subSelector)) {
> +                    if (subSelector != firstSubSelector)
> +                        str.appendLiteral(", ");
> +                    str.append(subSelector->selectorText());
> +                }

It would be a good idea to move this into a static function. The code of :nth-child(of) does more or less the same thing, both could use the same function.

> LayoutTests/ChangeLog:7
> +

Wow, that's some serious testing going on here! :)

> LayoutTests/ChangeLog:32
> +

Maybe I missed it, but I did not see any test for canonicalization of the selector when accessed through CSS OM.

For example, testing that :matches(a,b,c) returns :matches(a, b, c).

> LayoutTests/fast/css/parsing-css-matches-4.html:29
> +    // "*",

I guess you commented some for efficiency? Better remove them entirely, someone could be confused by the commented ones.

> LayoutTests/fast/css/parsing-css-matches-5.html:22
> +var invalidSelectors = [

Let's add unbalanced parenthesis to this set:
-:not(
-:matches(
-:nth-child(2n+1 of .foo

[Yusuke Suzuki]

Comment on attachment 239139 [details]
Patch

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

>> Source/WebCore/css/CSSSelector.cpp:450
>> +                }
> 
> It would be a good idea to move this into a static function. The code of :nth-child(of) does more or less the same thing, both could use the same function.

Yes! I've extracted it to `appendSelectorList` static function.

>> LayoutTests/ChangeLog:32
>> +
> 
> Maybe I missed it, but I did not see any test for canonicalization of the selector when accessed through CSS OM.
> 
> For example, testing that :matches(a,b,c) returns :matches(a, b, c).

Ah! Thanks for your pointing.
I missed it. I'll add it :)

>> LayoutTests/fast/css/parsing-css-matches-4.html:29
>> +    // "*",
> 
> I guess you commented some for efficiency? Better remove them entirely, someone could be confused by the commented ones.

Ah! OK, I'll remove them.
Yup. In this test, we generate the selector list composed of 3 selectors.
So there are `n! where n = validSelectors.length` selectors to be tested.
It takes too much time and easily exceeds the time limit.
To avoid this, I reduced the number of validSelectors to 1/2. And the comment outed 1/2 are tested in the parsing-css-matches-3.html.

>> LayoutTests/fast/css/parsing-css-matches-5.html:22
>> +var invalidSelectors = [
> 
> Let's add unbalanced parenthesis to this set:
> -:not(
> -:matches(
> -:nth-child(2n+1 of .foo

Looks very nice. I'll add them.

[Yusuke Suzuki]

Created attachment 239177 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 239139 [details]
Patch

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

>>> LayoutTests/fast/css/parsing-css-matches-4.html:29
>>> +    // "*",
>> 
>> I guess you commented some for efficiency? Better remove them entirely, someone could be confused by the commented ones.
> 
> Ah! OK, I'll remove them.
> Yup. In this test, we generate the selector list composed of 3 selectors.
> So there are `n! where n = validSelectors.length` selectors to be tested.
> It takes too much time and easily exceeds the time limit.
> To avoid this, I reduced the number of validSelectors to 1/2. And the comment outed 1/2 are tested in the parsing-css-matches-3.html.

s/`n! where n = validSelectors.length`/`nC3 where n = validSelectors.length`/

[Benjamin Poulain]

Comment on attachment 239177 [details]
Patch

Looks good to me.

[WebKit Commit Bot]

Comment on attachment 239177 [details]
Patch

Clearing flags on attachment: 239177

Committed r174259: <http://trac.webkit.org/changeset/174259>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.