Bug 199237 – Implement @supports selector().

• Component: CSS
• Status: RESOLVED FIXED

[Emilio Cobos Álvarez (:emilio)]

This is a simple selector query function that allows to query whether an UA supports a selector, which .

Spec lives at https://drafts.csswg.org/css-conditional-4/#at-supports-ext.

It was discussed https://github.com/w3c/csswg-drafts/issues/3207.

I plan to enable it on Firefox on a couple releases, it'd be great if WebKit implemented this as well so authors can use it, and should be pretty straight-forward.

[Radar WebKit Bug Importer]

<rdar://problem/54411727>

[Smoley]

*** Bug 204165 has been marked as a duplicate of this bug. ***

[Joonghun Park]

Created attachment 406779 [details]
<WIP>

[Joonghun Park]

Created attachment 406781 [details]
Need to rebase layout test result

[Joonghun Park]

Created attachment 406782 [details]
Add annotation in CSSSupportsParser::consumeConditionInParenthesis

[Darin Adler]

Comment on attachment 406782 [details]
Add annotation in CSSSupportsParser::consumeConditionInParenthesis

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

> Source/WebCore/ChangeLog:3
> +        Implement @supports selector().

Need to include tests in this.

> Source/WebCore/css/parser/CSSSelectorParser.cpp:141
> +// static

While there are a few static instances of this in WebKit code, we normally don’t include these comments. I can see how some people might like them,, but I don’t think this is the function we should start this new tradition with.

> Source/WebCore/css/parser/CSSSelectorParser.cpp:153
> +    if (containsUnknownWebKitPseudoElements(*complexSelector))
> +        return false;
> +    return true;

This is OK, but could also just use a single return statement here.

> Source/WebCore/css/parser/CSSSelectorParser.cpp:975
> +    for (const CSSSelector* current = &complexSelector; current; current = current->tagHistory()) {

I suggest using auto here instead of const CSSSelector*.

[Joonghun Park]

(In reply to Darin Adler from comment #6)
> Comment on attachment 406782 [details]
> Add annotation in CSSSupportsParser::consumeConditionInParenthesis
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406782&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Implement @supports selector().
> 
> Need to include tests in this.
> 
> > Source/WebCore/css/parser/CSSSelectorParser.cpp:141
> > +// static
> 
> While there are a few static instances of this in WebKit code, we normally
> don’t include these comments. I can see how some people might like them,,
> but I don’t think this is the function we should start this new tradition
> with.
> 
> > Source/WebCore/css/parser/CSSSelectorParser.cpp:153
> > +    if (containsUnknownWebKitPseudoElements(*complexSelector))
> > +        return false;
> > +    return true;
> 
> This is OK, but could also just use a single return statement here.
> 
> > Source/WebCore/css/parser/CSSSelectorParser.cpp:975
> > +    for (const CSSSelector* current = &complexSelector; current; current = current->tagHistory()) {
> 
> I suggest using auto here instead of const CSSSelector*.

Thank you for your review, Darin. I will apply your comments in the next patchset.

[Joonghun Park]

If I put some printf statements like this,
 
bool CSSSelectorParser::supportsComplexSelector(CSSParserTokenRange range, const CSSParserContext& context)
{
    range.consumeWhitespace();
    CSSSelectorParser parser(context, nullptr);
    printf("CSSSelectorParser::supportsComplexSelector: %s\n", range.serialize().utf8().data());
    auto parserSelector = parser.consumeComplexSelector(range);
    if (parser.m_failedParsing || !range.atEnd() || !parserSelector)
        return false;
    auto complexSelector = parserSelector->releaseSelector();
    printf("complexSelector: %s\n", complexSelector->value().string().utf8().data());
    ASSERT(complexSelector);
    if (containsUnknownWebKitPseudoElements(*complexSelector))
        return false;
    return true;
}

below logs are printed.

CSSSelectorParser::supportsComplexSelector: A | .B
complexSelector: B

So, it seems that |CSSSelectorParser::consumeComplexSelector| doesn't address the unknown combinator correctly.

I will make a separate bug to address this bug separately and leave a FIXME comment here.

[Joonghun Park]

Comment on attachment 406782 [details]
Add annotation in CSSSupportsParser::consumeConditionInParenthesis

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

>>> Source/WebCore/ChangeLog:3
>>> +        Implement @supports selector().
>> 
>> Need to include tests in this.
> 
> Thank you for your review, Darin. I will apply your comments in the next patchset.

Tests were added in the next patchset.

>> Source/WebCore/css/parser/CSSSelectorParser.cpp:141
>> +// static
> 
> While there are a few static instances of this in WebKit code, we normally don’t include these comments. I can see how some people might like them,, but I don’t think this is the function we should start this new tradition with.

Removed.

>> Source/WebCore/css/parser/CSSSelectorParser.cpp:153
>> +    return true;
> 
> This is OK, but could also just use a single return statement here.

I changed this with a single return statement.

>> Source/WebCore/css/parser/CSSSelectorParser.cpp:975
>> +    for (const CSSSelector* current = &complexSelector; current; current = current->tagHistory()) {
> 
> I suggest using auto here instead of const CSSSelector*.

Done.

[Joonghun Park]

Created attachment 406835 [details]
wpt tests has been added

[Darin Adler]

Comment on attachment 406835 [details]
wpt tests has been added

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

> Source/WebCore/css/parser/CSSSelectorParser.cpp:155
> +    return containsUnknownWebKitPseudoElements(*complexSelector) ? false : true;

Typically we would use "!" here rather than "? false : true".

> Source/WebCore/css/parser/CSSSelectorParser.cpp:977
> +    for (const auto* current = &complexSelector; current; current = current->tagHistory()) {

When I suggested auto, I meant "auto", not "const auto*".

> Source/WebCore/css/parser/CSSSupportsParser.cpp:47
> +    bool isSupportsSelector = false;
> +
> +    if (range.peek().type() == FunctionToken && range.peek().functionId() == CSSValueSelector)
> +        isSupportsSelector = true;

This boolean expression should be written in a straightforward way, not with an if statement:

    bool isSupportsSelector = range.peek().type() == FunctionToken && range.peek().functionId() == CSSValueSelector;

> Source/WebCore/css/parser/CSSSupportsParser.cpp:72
> +        SupportsResult nextResult = consumeConditionInParenthesis(range);

auto

> Source/WebCore/css/parser/CSSSupportsParser.cpp:112
> +    SupportsResult result = consumeConditionInParenthesis(range);

auto

> Source/WebCore/css/parser/CSSSupportsParser.cpp:138
> +    if (range.peek().type() != FunctionToken)
> +        return Invalid;
> +    if (range.peek().functionId() != CSSValueSelector)
>          return Invalid;

Why not use || and a single if statement?

> Source/WebCore/css/parser/CSSSupportsParser.cpp:144
> +    if (CSSSelectorParser::supportsComplexSelector(block, m_parser.context()))
> +        return Supported;
>  
> -    CSSParserTokenRange innerRange = range.consumeBlock();
> -    innerRange.consumeWhitespace();
> -    SupportsResult result = consumeCondition(innerRange);
> -    if (result != Invalid)
> -        return result;
> -    return consumeDeclarationConditionOrGeneralEnclosed(innerRange);
> +    return Unsupported;

return CSSSelectorParser::supportsComplexSelector(block, m_parser.context()) ? Supported : Unsupported;

> Source/WebCore/css/parser/CSSSupportsParser.cpp:150
> +    CSSParserTokenRange innerRange = range;

auto

> Source/WebCore/css/parser/CSSSupportsParser.cpp:155
> +        SupportsResult result = consumeCondition(innerRange);

auto

[Joonghun Park]

Comment on attachment 406835 [details]
wpt tests has been added

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

I'm sorry for the late update, Darin.
I updated all the comments you left in the previous patchset. Could you please review this change again?

>> Source/WebCore/css/parser/CSSSelectorParser.cpp:155
>> +    return containsUnknownWebKitPseudoElements(*complexSelector) ? false : true;
> 
> Typically we would use "!" here rather than "? false : true".

Ok, I will change this in the next patchset.

>> Source/WebCore/css/parser/CSSSelectorParser.cpp:977
>> +    for (const auto* current = &complexSelector; current; current = current->tagHistory()) {
> 
> When I suggested auto, I meant "auto", not "const auto*".

Done.

>> Source/WebCore/css/parser/CSSSupportsParser.cpp:47
>> +        isSupportsSelector = true;
> 
> This boolean expression should be written in a straightforward way, not with an if statement:
> 
>     bool isSupportsSelector = range.peek().type() == FunctionToken && range.peek().functionId() == CSSValueSelector;

I rewrote the patch accoring to the spec, so I found that this bool variable was no longer needed here.

>> Source/WebCore/css/parser/CSSSupportsParser.cpp:72
>> +        SupportsResult nextResult = consumeConditionInParenthesis(range);
> 
> auto

Done.

>> Source/WebCore/css/parser/CSSSupportsParser.cpp:112
>> +    SupportsResult result = consumeConditionInParenthesis(range);
> 
> auto

Done.

>> Source/WebCore/css/parser/CSSSupportsParser.cpp:138
>>          return Invalid;
> 
> Why not use || and a single if statement?

Ok, I will change this in the next patchset.

>> Source/WebCore/css/parser/CSSSupportsParser.cpp:144
>> +    return Unsupported;
> 
> return CSSSelectorParser::supportsComplexSelector(block, m_parser.context()) ? Supported : Unsupported;

Done.

>> Source/WebCore/css/parser/CSSSupportsParser.cpp:150
>> +    CSSParserTokenRange innerRange = range;
> 
> auto

Done.

>> Source/WebCore/css/parser/CSSSupportsParser.cpp:155
>> +        SupportsResult result = consumeCondition(innerRange);
> 
> auto

Done.

[Joonghun Park]

Created attachment 407359 [details]
Patch

[Joonghun Park]

Created attachment 407366 [details]
Patch

[Joonghun Park]

Created attachment 407378 [details]
Fix the debug layout test crash by adding CSSSelector match check in containsUnknownWebKitPseudoElements

[Antti Koivisto]

Comment on attachment 407378 [details]
Fix the debug layout test crash by adding CSSSelector match check in containsUnknownWebKitPseudoElements

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

> Source/WebCore/ChangeLog:26
> [phone]  Joonghun Park  <[e-mail]>
> +
> +        Implement @supports selector().
> +        https://bugs.webkit.org/show_bug.cgi?id=199237
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Tests: css3/conditional/w3c/at-supports-040.html
> +               css3/conditional/w3c/at-supports-041.html
> +               css3/conditional/w3c/at-supports-042.html
> +
> +        * css/CSSValueKeywords.in:
> +        * css/parser/CSSParserImpl.h:
> +        (WebCore::CSSParserImpl::context const):
> +        * css/parser/CSSSelectorParser.cpp:
> +        (WebCore::CSSSelectorParser::supportsComplexSelector):
> +        (WebCore::CSSSelectorParser::containsUnknownWebKitPseudoElements):
> +        * css/parser/CSSSupportsParser.cpp:
> +        (WebCore::CSSSupportsParser::supportsCondition):
> +        (WebCore::CSSSupportsParser::consumeCondition):
> +        (WebCore::CSSSupportsParser::consumeNegation):
> +        (WebCore::CSSSupportsParser::consumeSupportsFeatureOrGeneralEnclosed):
> +        (WebCore::CSSSupportsParser::consumeSupportsSelectorFn):
> +        (WebCore::CSSSupportsParser::consumeConditionInParenthesis):
> +        (WebCore::CSSSupportsParser::consumeDeclarationConditionOrGeneralEnclosed): Deleted.
> +        * css/parser/CSSSupportsParser.h:

You should type some words about this patch and link to the spec.

> Source/WebCore/css/parser/CSSSelectorParser.cpp:155
> +    return !containsUnknownWebKitPseudoElements(*complexSelector) ? true : false;

Just

return !containsUnknownWebKitPseudoElements(*complexSelector);

or maybe

if (containsUnknownWebKitPseudoElements(*complexSelector))
    return false;

return true;

to emphasize that false is a rare special case.

> Source/WebCore/css/parser/CSSSelectorParser.cpp:983
> +bool CSSSelectorParser::containsUnknownWebKitPseudoElements(const CSSSelector& complexSelector)
> +{
> +    for (auto current = &complexSelector; current; current = current->tagHistory()) {
> +        if (current->match() == CSSSelector::PseudoElement && current->pseudoElementType() == CSSSelector::PseudoElementWebKitCustom)
> +            return true;
> +    }
> +
> +    return false;
> +}

Why are PseudoElementWebKitCustom excluded? (seems ok but is there some specific reason?)

> Source/WebCore/css/parser/CSSSupportsParser.cpp:132
> +CSSSupportsParser::SupportsResult CSSSupportsParser::consumeSupportsSelectorFn(CSSParserTokenRange& range)

WebKit coding style generally uses full words so this should be consumeSupportsSelectorFunction. Unless there is some spec or other good reason for this naming?

[Antti Koivisto]

Comment on attachment 407378 [details]
Fix the debug layout test crash by adding CSSSelector match check in containsUnknownWebKitPseudoElements

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

> LayoutTests/ChangeLog:13
> +        * css3/conditional/w3c/at-supports-040-expected.html: Added.
> +        * css3/conditional/w3c/at-supports-040.html: Added.
> +        * css3/conditional/w3c/at-supports-041-expected.html: Added.
> +        * css3/conditional/w3c/at-supports-041.html: Added.
> +        * css3/conditional/w3c/at-supports-042-expected.html: Added.
> +        * css3/conditional/w3c/at-supports-042.html: Added.

Aren't there WPT tests to enable or import for this?

[Joonghun Park]

Comment on attachment 407378 [details]
Fix the debug layout test crash by adding CSSSelector match check in containsUnknownWebKitPseudoElements

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

Thank you for your review, Antti.

>> Source/WebCore/ChangeLog:26
>> +        * css/parser/CSSSupportsParser.h:
> 
> You should type some words about this patch and link to the spec.

Ok, I will add patch description for this change as below.

This feature allows authors to test if the browser supports the tested selector syntax.
The corresponding spec is https://drafts.csswg.org/css-conditional-4/#at-supports-ext.

And unknown -webkit- pseudo elements are not supported according to the spec, https://drafts.csswg.org/css-conditional-4/#support-definition-ext.

>> Source/WebCore/css/parser/CSSSelectorParser.cpp:155
>> +    return !containsUnknownWebKitPseudoElements(*complexSelector) ? true : false;
> 
> Just
> 
> return !containsUnknownWebKitPseudoElements(*complexSelector);
> 
> or maybe
> 
> if (containsUnknownWebKitPseudoElements(*complexSelector))
>     return false;
> 
> return true;
> 
> to emphasize that false is a rare special case.

Done with return !containsUnknownWebKitPseudoElements(*complexSelector);.

>> Source/WebCore/css/parser/CSSSelectorParser.cpp:983
>> +}
> 
> Why are PseudoElementWebKitCustom excluded? (seems ok but is there some specific reason?)

This behavior is specified in the spec, https://drafts.csswg.org/css-conditional-4/#support-definition-ext.
I will leave a comment in the patch description about this.

>> Source/WebCore/css/parser/CSSSupportsParser.cpp:132
>> +CSSSupportsParser::SupportsResult CSSSupportsParser::consumeSupportsSelectorFn(CSSParserTokenRange& range)
> 
> WebKit coding style generally uses full words so this should be consumeSupportsSelectorFunction. Unless there is some spec or other good reason for this naming?

This naming was following https://drafts.csswg.org/css-conditional-4/#at-supports-ext's term <supports-selector-fn>.
But as you suggested, consumeSupportsSelectorFunction would be ok too.
I will rename it as you suggested in the next patchset.

>> LayoutTests/ChangeLog:13
>> +        * css3/conditional/w3c/at-supports-042.html: Added.
> 
> Aren't there WPT tests to enable or import for this?

When Emilio shipped this feature in Firefox, he enabled this feature with these 3 wpts, and I heard from him that these are enough tests to enable this feature.
And AFAIK these are all of the tests about css supports selector() in wpt.

[Joonghun Park]

Comment on attachment 407378 [details]
Fix the debug layout test crash by adding CSSSelector match check in containsUnknownWebKitPseudoElements

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

>>> LayoutTests/ChangeLog:13
>>> +        * css3/conditional/w3c/at-supports-042.html: Added.
>> 
>> Aren't there WPT tests to enable or import for this?
> 
> When Emilio shipped this feature in Firefox, he enabled this feature with these 3 wpts, and I heard from him that these are enough tests to enable this feature.
> And AFAIK these are all of the tests about css supports selector() in wpt.

FYI, the Forefox link for this feature implementation is https://hg.mozilla.org/mozilla-central/rev/631545ef7925.

[Joonghun Park]

Created attachment 407424 [details]
Patch for landing

[Antti Koivisto]

> > Aren't there WPT tests to enable or import for this?
> 
> When Emilio shipped this feature in Firefox, he enabled this feature with
> these 3 wpts, and I heard from him that these are enough tests to enable
> this feature.
> And AFAIK these are all of the tests about css supports selector() in wpt.

I mean we should probably be importing the whole https://github.com/web-platform-tests/wpt/tree/master/css/css-conditional directory under LayoutTests/imported/w3c/web-platform-tests/ instead of adding the tests manually.

[Joonghun Park]

Ah, I see. Then I will import (In reply to Antti Koivisto from comment #21)
> > > Aren't there WPT tests to enable or import for this?
> > 
> > When Emilio shipped this feature in Firefox, he enabled this feature with
> > these 3 wpts, and I heard from him that these are enough tests to enable
> > this feature.
> > And AFAIK these are all of the tests about css supports selector() in wpt.
> 
> I mean we should probably be importing the whole
> https://github.com/web-platform-tests/wpt/tree/master/css/css-conditional
> directory under LayoutTests/imported/w3c/web-platform-tests/ instead of
> adding the tests manually.

Ah, I see. Then I will import LayoutTests/imported/w3c/web-platform-tests/
and remove the correspond existing LayoutTests/ test cases in the separate gardening patch.

[EWS]

Committed r266253: <https://trac.webkit.org/changeset/266253>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407424 [details].