Bug 199237 - Implement @supports selector().
Summary: Implement @supports selector().
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joonghun Park
URL:
Keywords: BrowserCompat, InRadar
: 204165 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-06-26 15:30 PDT by Emilio Cobos Álvarez (:emilio)
Modified: 2020-08-27 14:10 PDT (History)
13 users (show)

See Also:


Attachments
<WIP> (9.09 KB, patch)
2020-08-18 06:11 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Need to rebase layout test result (9.28 KB, patch)
2020-08-18 07:39 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Add annotation in CSSSupportsParser::consumeConditionInParenthesis (9.38 KB, patch)
2020-08-18 07:51 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
wpt tests has been added (20.47 KB, patch)
2020-08-19 04:23 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (23.68 KB, patch)
2020-08-26 17:06 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (19.48 KB, patch)
2020-08-26 18:52 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Fix the debug layout test crash by adding CSSSelector match check in containsUnknownWebKitPseudoElements (19.46 KB, patch)
2020-08-26 23:16 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch for landing (20.58 KB, patch)
2020-08-27 13:12 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emilio Cobos Álvarez (:emilio) 2019-06-26 15:30:49 PDT
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.
Comment 1 Radar WebKit Bug Importer 2019-08-16 16:08:50 PDT
<rdar://problem/54411727>
Comment 2 Nick Smoley 2019-11-18 12:41:39 PST
*** Bug 204165 has been marked as a duplicate of this bug. ***
Comment 3 Joonghun Park 2020-08-18 06:11:28 PDT
Created attachment 406779 [details]
<WIP>
Comment 4 Joonghun Park 2020-08-18 07:39:37 PDT
Created attachment 406781 [details]
Need to rebase layout test result
Comment 5 Joonghun Park 2020-08-18 07:51:17 PDT
Created attachment 406782 [details]
Add annotation in CSSSupportsParser::consumeConditionInParenthesis
Comment 6 Darin Adler 2020-08-18 08:06:14 PDT
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*.
Comment 7 Joonghun Park 2020-08-18 08:34:01 PDT
(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.
Comment 8 Joonghun Park 2020-08-18 18:19:55 PDT
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.
Comment 9 Joonghun Park 2020-08-19 04:15:25 PDT
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.
Comment 10 Joonghun Park 2020-08-19 04:23:28 PDT
Created attachment 406835 [details]
wpt tests has been added
Comment 11 Darin Adler 2020-08-19 08:23:58 PDT
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
Comment 12 Joonghun Park 2020-08-26 17:01:13 PDT
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.
Comment 13 Joonghun Park 2020-08-26 17:06:59 PDT
Created attachment 407359 [details]
Patch
Comment 14 Joonghun Park 2020-08-26 18:52:06 PDT
Created attachment 407366 [details]
Patch
Comment 15 Joonghun Park 2020-08-26 23:16:09 PDT
Created attachment 407378 [details]
Fix the debug layout test crash by adding CSSSelector match check in containsUnknownWebKitPseudoElements
Comment 16 Antti Koivisto 2020-08-27 09:52:11 PDT
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
> +2020-08-26  Joonghun Park  <jh718.park@samsung.com>
> +
> +        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?
Comment 17 Antti Koivisto 2020-08-27 09:53:15 PDT
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?
Comment 18 Joonghun Park 2020-08-27 12:58:29 PDT
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.
Comment 19 Joonghun Park 2020-08-27 13:06:33 PDT
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.
Comment 20 Joonghun Park 2020-08-27 13:12:43 PDT
Created attachment 407424 [details]
Patch for landing
Comment 21 Antti Koivisto 2020-08-27 13:28:01 PDT
> > 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.
Comment 22 Joonghun Park 2020-08-27 13:34:36 PDT
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.
Comment 23 EWS 2020-08-27 14:10:24 PDT
Committed r266253: <https://trac.webkit.org/changeset/266253>

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