Bug 172906 – CSS.supports(one_string) should accept paren-less declaration

• Component: CSS
• Status: RESOLVED FIXED

[Emilio Cobos Álvarez]

Patch incoming.

[Emilio Cobos Álvarez]

Created attachment 311962 [details]
Patch

[Emilio Cobos Álvarez]

Hi Dean, David, looking at the blame log you've reviewed/written most of the CSS parser changes, mind taking a look at this patch?

Thanks!

[Build Bot]

Comment on attachment 311962 [details]
Patch

Attachment 311962 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3871480

New failing tests:
imported/w3c/web-platform-tests/cssom/CSS.html

[Build Bot]

Created attachment 311963 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Emilio Cobos Álvarez]

Created attachment 311964 [details]
Patch

[Darin Adler]

Comment on attachment 311964 [details]
Patch

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

> Source/WebCore/css/parser/CSSSupportsParser.cpp:39
> +    bool implicitParentheses = mode == ForWindowCSS;

It’s not so great to name a local variable "implicit parentheses" when it really means "add implicit parentheses" or "allow implicit parentheses" or something like that.

> Source/WebCore/css/parser/CSSSupportsParser.cpp:46
> +    return range.peek().type() == IdentToken && parser.supportsDeclaration(range) ? Supported : Unsupported;

Assuming that this were correct, it’s annoying that this code matches the last line of consumeConditionInParenthesis but doesn’t share a helper function. Even though it’s a one liner, I think maybe there should be a function so we can share this.

But it doesn’t look correct to me. There are quite a few checks missing here that the other functions have, such as a check for atEnd, check for trailing whitespace, etc. I think we need lots of invalid test cases without parentheses, with things like trailing garbage to show us that this is not correct.

Here are a few test cases we should add. I think these are the correct expected results:

shouldBeFalse('CSS.supports("display: deadbeef")');
shouldBeTrue('CSS.supports("(display: none) and (display: block) or (display: inline)")');
shouldBeTrue('CSS.supports("not (display: deadbeef) and (display: block)")');
shouldBeFalse('CSS.supports("!important")');
shouldBeTrue('CSS.supports("top: -webkit-calc(80% - 20px)")');
shouldBeTrue('CSS.supports("background-color: rgb(0, 128, 0)")');
shouldBeTrue('CSS.supports("background: url(\'/blah\')"');
shouldBeFalse('CSS.supports("background: invalid(\'/blah\')")');

shouldBeFalse('CSS.supports("display: none;")');
shouldBeFalse('CSS.supports("display: none; garbage")');

In addition to these, generally speaking I don’t think there are enough tests cases for trailing garbage after an otherwise correct supports query.

[Emilio Cobos Álvarez]

(In reply to Darin Adler from comment #6)
> Comment on attachment 311964 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311964&action=review
> 
> > Source/WebCore/css/parser/CSSSupportsParser.cpp:39
> > +    bool implicitParentheses = mode == ForWindowCSS;
> 
> It’s not so great to name a local variable "implicit parentheses" when it
> really means "add implicit parentheses" or "allow implicit parentheses" or
> something like that.

Fair enough, I just inlined the check.

> > Source/WebCore/css/parser/CSSSupportsParser.cpp:46
> > +    return range.peek().type() == IdentToken && parser.supportsDeclaration(range) ? Supported : Unsupported;
> 
> Assuming that this were correct, it’s annoying that this code matches the
> last line of consumeConditionInParenthesis but doesn’t share a helper
> function. Even though it’s a one liner, I think maybe there should be a
> function so we can share this.
> 
> But it doesn’t look correct to me. There are quite a few checks missing here
> that the other functions have, such as a check for atEnd, check for trailing
> whitespace, etc. I think we need lots of invalid test cases without
> parentheses, with things like trailing garbage to show us that this is not
> correct.

How not? Trailing whitespace is ignored regardless, and junk is handled by CSSParserImpl::supportsDeclaration.

> Here are a few test cases we should add. I think these are the correct
> expected results:
> 
> shouldBeFalse('CSS.supports("display: deadbeef")');
> shouldBeTrue('CSS.supports("(display: none) and (display: block) or
> (display: inline)")');

This one is invalid (need parenthesis to disambiguate between and and or).

> shouldBeTrue('CSS.supports("not (display: deadbeef) and (display: block)")');

Ditto (to disambiguate between not and and).

> shouldBeFalse('CSS.supports("!important")');
> shouldBeTrue('CSS.supports("top: -webkit-calc(80% - 20px)")');
> shouldBeTrue('CSS.supports("background-color: rgb(0, 128, 0)")');
> shouldBeTrue('CSS.supports("background: url(\'/blah\')"');

Missing the closing paren, so this throws, but with it all these tests pass with this patch.

> shouldBeFalse('CSS.supports("background: invalid(\'/blah\')")');
> 
> shouldBeFalse('CSS.supports("display: none;")');
> shouldBeFalse('CSS.supports("display: none; garbage")');

Ditto about this, these pass with this patch, I'll add it.

In particular, this works because the only thing that parses in parens and doesn't in a normal condition per the grammar is the declaration condition: https://drafts.csswg.org/css-conditional-3/#supports_declaration_condition

Which is what we check here. If you're worried about the function syntax (which currently always evaluates to false) evaluating to something else in the future, I can put both conditions in a helper function.

I can also write something more complex like https://gist.github.com/emilio/812eed04fa62e1d696c7ba1f0223f16c, but I think it's unnecessary.

[Emilio Cobos Álvarez]

(In reply to Emilio Cobos Álvarez from comment #7)
> (In reply to Darin Adler from comment #6)
> > Comment on attachment 311964 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=311964&action=review
> > 
> > > Source/WebCore/css/parser/CSSSupportsParser.cpp:39
> > > +    bool implicitParentheses = mode == ForWindowCSS;
> > 
> > It’s not so great to name a local variable "implicit parentheses" when it
> > really means "add implicit parentheses" or "allow implicit parentheses" or
> > something like that.
> 
> Fair enough, I just inlined the check.
> 
> > > Source/WebCore/css/parser/CSSSupportsParser.cpp:46
> > > +    return range.peek().type() == IdentToken && parser.supportsDeclaration(range) ? Supported : Unsupported;
> > 
> > Assuming that this were correct, it’s annoying that this code matches the
> > last line of consumeConditionInParenthesis but doesn’t share a helper
> > function. Even though it’s a one liner, I think maybe there should be a
> > function so we can share this.
> > 
> > But it doesn’t look correct to me. There are quite a few checks missing here
> > that the other functions have, such as a check for atEnd, check for trailing
> > whitespace, etc. I think we need lots of invalid test cases without
> > parentheses, with things like trailing garbage to show us that this is not
> > correct.
> 
> How not? Trailing whitespace is ignored regardless, and junk is handled by
> CSSParserImpl::supportsDeclaration.
> 
> > Here are a few test cases we should add. I think these are the correct
> > expected results:
> > 
> > shouldBeFalse('CSS.supports("display: deadbeef")');
> > shouldBeTrue('CSS.supports("(display: none) and (display: block) or
> > (display: inline)")');
> 
> This one is invalid (need parenthesis to disambiguate between and and or).
> 
> > shouldBeTrue('CSS.supports("not (display: deadbeef) and (display: block)")');
> 
> Ditto (to disambiguate between not and and).
> 
> > shouldBeFalse('CSS.supports("!important")');
> > shouldBeTrue('CSS.supports("top: -webkit-calc(80% - 20px)")');
> > shouldBeTrue('CSS.supports("background-color: rgb(0, 128, 0)")');
> > shouldBeTrue('CSS.supports("background: url(\'/blah\')"');
> 
> Missing the closing paren, so this throws, but with it all these tests pass
> with this patch.
> 
> > shouldBeFalse('CSS.supports("background: invalid(\'/blah\')")');
> > 
> > shouldBeFalse('CSS.supports("display: none;")');
> > shouldBeFalse('CSS.supports("display: none; garbage")');
> 
> Ditto about this, these pass with this patch, I'll add it.
> 
> In particular, this works because the only thing that parses in parens and
> doesn't in a normal condition per the grammar is the declaration condition:
> https://drafts.csswg.org/css-conditional-3/#supports_declaration_condition
> 
> Which is what we check here. If you're worried about the function syntax
> (which currently always evaluates to false) evaluating to something else in
> the future, I can put both conditions in a helper function.

Note that this would have made a difference if the function syntax would start returning Supported values, in which case something like:

CSS.supports("function(foo) random garbage");

would return the function(foo) result independently of the random garbage. I guess I'll add the atEnd() check, just so the result doesn't lie and tell Unsupported instead of Invalid.

[Emilio Cobos Álvarez]

Created attachment 311973 [details]
Patch

[Emilio Cobos Álvarez]

(In reply to Emilio Cobos Álvarez from comment #8)
> I guess I'll add the atEnd() check, just so the result doesn't lie and tell
> Unsupported instead of Invalid.

I couldn't do this in the end because CSSParserImpl::consumeDeclaration actually takes the range by value, so it isn't consumed.

Anyway, I don't think it's observable per my above comments, and we already don't check whether innerRange is consumed, so if you think it deserves a fix, I'd prefer to move it to another bug.

[Darin Adler]

Comment on attachment 311973 [details]
Patch

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

> LayoutTests/css3/supports-dom-api.html:47
>      shouldBeFalse('CSS.supports("(display: none) and (display: block) or (display: inline)")');
>      shouldBeFalse('CSS.supports("not (display: deadbeef) and (display: block)")');
> +    shouldBeFalse('CSS.supports("(display: none) and (display: block) or (display: inline)")');
> +    shouldBeFalse('CSS.supports("not (display: deadbeef) and (display: block)")');

Looks like these got doubled accidentally.

[Emilio Cobos Álvarez]

Created attachment 311975 [details]
Patch

[Emilio Cobos Álvarez]

(In reply to Darin Adler from comment #11)
> Comment on attachment 311973 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311973&action=review
> 
> > LayoutTests/css3/supports-dom-api.html:47
> >      shouldBeFalse('CSS.supports("(display: none) and (display: block) or (display: inline)")');
> >      shouldBeFalse('CSS.supports("not (display: deadbeef) and (display: block)")');
> > +    shouldBeFalse('CSS.supports("(display: none) and (display: block) or (display: inline)")');
> > +    shouldBeFalse('CSS.supports("not (display: deadbeef) and (display: block)")');
> 
> Looks like these got doubled accidentally.

Thanks, good catch :)

[WebKit Commit Bot]

Comment on attachment 311975 [details]
Patch

Clearing flags on attachment: 311975

Committed r217844: <http://trac.webkit.org/changeset/217844>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.