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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
9 months ago
5 months ago

People

(Reporter: SimonSapin, Assigned: manishearth)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox54 wontfix, firefox55 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

9 months 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

5 months ago
Assignee: nobody → manishearth
Blocks: 1321197
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
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 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 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)
(Assignee)

Comment 9

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

Comment 10

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

Comment 11

5 months 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: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
54 RC build is released. Mark 54 won't fix.
status-firefox54: affected → wontfix
You need to log in before you can comment on or make changes to this bug.