Closed
Bug 1338486
Opened 7 years ago
Closed 7 years ago
CSS.supports(one_string) should accept paren-less declaration
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: SimonSapin, Assigned: manishearth)
References
()
Details
Attachments
(1 file)
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•7 years ago
|
Comment hidden (mozreview-request) |
Comment 2•7 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`.
Comment 3•7 years ago
|
||
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.
Updated•7 years ago
|
Attachment #8874194 -
Flags: review?(dholbert)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•7 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•7 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) |
Assignee | ||
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=541ffb5494484679a4f566d55679f60f6dccd86b
Comment 10•7 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•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/50e5014bb919 Update wpt expectations for /cssom/CSS.html.
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e5c037e4dbcc https://hg.mozilla.org/mozilla-central/rev/50e5014bb919
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 13•7 years ago
|
||
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.
Description
•