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.
<rdar://problem/54411727>
*** Bug 204165 has been marked as a duplicate of this bug. ***
Created attachment 406779 [details] <WIP>
Created attachment 406781 [details] Need to rebase layout test result
Created attachment 406782 [details] Add annotation in CSSSupportsParser::consumeConditionInParenthesis
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*.
(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.
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 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.
Created attachment 406835 [details] wpt tests has been added
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 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.
Created attachment 407359 [details] Patch
Created attachment 407366 [details] Patch
Created attachment 407378 [details] Fix the debug layout test crash by adding CSSSelector match check in containsUnknownWebKitPseudoElements
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 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 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 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.
Created attachment 407424 [details] Patch for landing
> > 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 (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.
Committed r266253: <https://trac.webkit.org/changeset/266253> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407424 [details].