1338486 - CSS.supports(one_string) should accept paren-less declaration

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Simon Sapin (:SimonSapin)]

Test case

    [CSS.supports("(color: green)"), CSS.supports("color: green")]

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4881

Expected result: [ true, true ]

Actual result in Firefox Release 51.0.1, Firefox Nightly 54.0a1 (2017-02-09), and Chromium 56.0.2924.87: [ true, false ]

Relevant spec: https://drafts.csswg.org/css-conditional/#the-css-interface

> When invoked with a single conditionText argument, it must return true
> if conditionText, when either parsed and evaluated as a supports_condition
> or parsed as a declaration, wrapped in implied parentheses, and evaluated
> as a supports_condition, would return true. Otherwise, it must return 
> false.

Note "wrapped in implied parentheses".

Comment 1

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1338486: Support implied parentheses in CSS.supports;

Review commit: https://reviewboard.mozilla.org/r/145628/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145628/

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874194 [details]
Bug 1338486: Support implied parentheses in CSS.supports;

https://reviewboard.mozilla.org/r/145628/#review149522

::: layout/style/CSS.cpp:97
(Diff revision 1)
>    }
>  
>    nsCSSParser parser;
>    return parser.EvaluateSupportsCondition(aCondition, info.mDocURI,
> -                                          info.mBaseURI, info.mPrincipal);
> +                                          info.mBaseURI, info.mPrincipal,
> +                                          /* impliedParentheses */ true);

Probably better using an `enum class` rather than a boolean?

::: layout/style/nsCSSParser.h:264
(Diff revision 1)
>     */
>    bool EvaluateSupportsCondition(const nsAString& aCondition,
>                                   nsIURI* aDocURL,
>                                   nsIURI* aBaseURL,
> -                                 nsIPrincipal* aDocPrincipal);
> +                                 nsIPrincipal* aDocPrincipal,
> +                                 bool impliedParentheses = false);

Params should prefixed by "a", e.g. `aImpliedParentheses`.

Comment 3

[Daniel Holbert [:dholbert]]

Without having looked at the patch yet, I agree with xidorn's suggestions -- I'll just redirect the review to him since he's already taken a look & I trust his judgement.

Comment 4

[Manish Goregaokar [:manishearth]]

Comment on attachment 8874194 [details]
Bug 1338486: Support implied parentheses in CSS.supports;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145628/diff/1-2/

Comment 5

[Manish Goregaokar [:manishearth]]

Comment on attachment 8874194 [details]
Bug 1338486: Support implied parentheses in CSS.supports;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145628/diff/2-3/

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874194 [details]
Bug 1338486: Support implied parentheses in CSS.supports;

https://reviewboard.mozilla.org/r/145628/#review149548

::: layout/style/nsCSSParser.h:270
(Diff revision 2)
> +                                 mozilla::css::SupportsParsingSettings aImpliedParentheses
> +                                  = mozilla::css::SupportsParsingSettings::Normal);

I don't like this kind of long name... but I don't have a good idea either... so probably fine.

::: layout/style/nsCSSParser.cpp:2435
(Diff revision 2)
> +  if (aImpliedParentheses == SupportsParsingSettings::ImpliedParentheses) {
> +    parsedOK = ParseSupportsConditionInParensInsideParens(conditionMet) && !GetToken(true);
> +  } else {
> +    parsedOK = ParseSupportsCondition(conditionMet) && !GetToken(true);
> +  }
> +  

Redundant line and whitespaces.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874194 [details]
Bug 1338486: Support implied parentheses in CSS.supports;

https://reviewboard.mozilla.org/r/145628/#review149550

::: layout/style/nsCSSParser.h:270
(Diff revision 3)
>     */
>    bool EvaluateSupportsCondition(const nsAString& aCondition,
>                                   nsIURI* aDocURL,
>                                   nsIURI* aBaseURL,
> -                                 nsIPrincipal* aDocPrincipal);
> +                                 nsIPrincipal* aDocPrincipal,
> +                                 mozilla::css::SupportsParsingSettings aImpliedParentheses

Oh, if the type is "Settings", probably just call the parameter "aSettings" or "aOptions" something like that?

Comment 8

[Manish Goregaokar [:manishearth]]

Comment on attachment 8874194 [details]
Bug 1338486: Support implied parentheses in CSS.supports;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145628/diff/3-4/

Comment 9

[Manish Goregaokar [:manishearth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=541ffb5494484679a4f566d55679f60f6dccd86b

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5c037e4dbcc
Support implied parentheses in CSS.supports; r=xidorn

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50e5014bb919
Update wpt expectations for /cssom/CSS.html.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e5c037e4dbcc
https://hg.mozilla.org/mozilla-central/rev/50e5014bb919

Comment 13

[Gerry Chang [:gchang]]

54 RC build is released. Mark 54 won't fix.