Closed
Bug 1239153
Opened 9 years ago
Closed 9 years ago
Firefox's CSS parser rejects "-webkit-linear-gradient(0,pink,teal)"
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(2 files)
201 bytes,
text/html
|
Details | |
2.97 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
Chrome renders a gradient in the attached testcase. We do not.
The expression in question is:
-webkit-linear-gradient(0,pink,teal);
Assignee | ||
Comment 1•9 years ago
|
||
(It's the leading "0," that makes us reject the gradient. I'm not sure offhand what Chrome interprets that "0" as meaning, but it does seem to make the gradient horizontal -- if I remove it, the gradient becomes vertical.)
Presumably this is because they allow unitless 0 for angles, whereas we don't. See also VARIANT_ZERO_ANGLE in nsCSSParser.cpp.
Assignee | ||
Comment 3•9 years ago
|
||
Ah, yes -- that's it. If I replace "0" with "0deg", then we match Chrome and render the gradient.
Assignee | ||
Comment 4•9 years ago
|
||
So -- I think every single VARIANT_ANGLE usage in ParseLinearGradient wants to have VARIANT_ZERO_ANGLE included as well, *if* we're parsing a -webkit-linear-gradient expression.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Flags: in-testsuite?
Assignee | ||
Comment 5•9 years ago
|
||
Actually (correcting myself), there's only one VARIANT_ANGLE usage in ParseLinearGradient which is applicable for webkit-prefixed gradients.
This patch fixes that one, and adds two mochitest cases to property_database.js to make sure it works.
Attachment #8707267 -
Flags: review?(cam)
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8707267 [details] [diff] [review]
fix v1
Note:
>- // if we got an angle, we might now have a comma, ending the gradient-line
>+ // If we got an angle, we might now have a comma, ending the gradient-line.
(My only changes to this contextual comment were capitalizing the first letter and adding a "." at the end.)
Comment 8•9 years ago
|
||
Comment on attachment 8707267 [details] [diff] [review]
fix v1
It would be better to add a test to ensure that unprefixed gradients do NOT accept unitless angles.
Comment 9•9 years ago
|
||
Comment on attachment 8707267 [details] [diff] [review]
fix v1
Review of attachment 8707267 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/style/nsCSSParser.cpp
@@ +9894,5 @@
> if (haveGradientLine) {
> // Parse a <legacy-gradient-line>
> cssGradient->mIsLegacySyntax = true;
> + // In -webkit-linear-gradient expressions (handled below), we need to accept
> + // unitless 0 for angles, to match webkit/blink.
Nit: "WebKit/Blink"
@@ +9903,2 @@
> bool haveAngle =
> + ParseSingleTokenVariant(cssGradient->mAngle, angleFlags, nullptr);
What if we have for example:
-webkit-radial-gradient(0 20px, cover circle, red, blue)
Won't we think we got an angle, when we should have treated it as a zero length?
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #8)
> It would be better to add a test to ensure that unprefixed gradients do NOT
> accept unitless angles.
Thanks! I thought about that, and naively assumed that such tests must already be present. But it looks like they actually aren't, so now I've added some locally.
> Won't we think we got an angle, when we should have treated it as a zero length?
Excellent question -- I hadn't thought of that, but I should've. I think we're OK, though.
First: This patch only affects our handling of webkit *linear* gradients, so it doesn't affect your sample expression (a radial gradient). And as noted in bug 1210575 comment 16, -webkit-radial-gradient doesn't support angles. So we only need to worry about linear gradients here.
Secondly: -webkit-linear-gradient only supports "box position" keywords (top,right, etc) for positioning -- not lengths -- as noted in bug 1210575 comment 23, section (c). So that means we don't have to worry about accidentally parsing a length "0" as an angle, because a length 0 would've been invalid anyway.
Comment 11•9 years ago
|
||
Thanks. When thinking about the angle ambiguity, I then went on to check radial gradient syntax for lengths-in-first-position, rather than linear gradients. Good to see we're fine here.
Updated•9 years ago
|
Attachment #8707267 -
Flags: review?(cam) → review+
Comment 12•9 years ago
|
||
Comment 13•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•