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

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: SimonSapin, Assigned: manishearth)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 wontfix, firefox55 fixed)

Details

()

Attachments

(1 attachment)

Reporter

Description

2 years ago
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".
Assignee

Updated

2 years ago
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
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`.
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.
Attachment #8874194 - Flags: review?(dholbert)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
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.
Attachment #8874194 - Flags: review?(xidorn+moz) → review+

Comment 7

2 years ago
mozreview-review
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 hidden (mozreview-request)

Comment 10

2 years ago
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e5c037e4dbcc
Support implied parentheses in CSS.supports; r=xidorn

Comment 11

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/50e5014bb919
Update wpt expectations for /cssom/CSS.html.
https://hg.mozilla.org/mozilla-central/rev/e5c037e4dbcc
https://hg.mozilla.org/mozilla-central/rev/50e5014bb919
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
54 RC build is released. Mark 54 won't fix.
You need to log in before you can comment on or make changes to this bug.