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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(2 files)

Attached file testcase 1
Chrome renders a gradient in the attached testcase. We do not. The expression in question is: -webkit-linear-gradient(0,pink,teal);
(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.
Ah, yes -- that's it. If I replace "0" with "0deg", then we match Chrome and render the gradient.
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: nobody → dholbert
Status: NEW → ASSIGNED
Flags: in-testsuite?
Attached patch fix v1Splinter Review
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)
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 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 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?
(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.
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.
Attachment #8707267 - Flags: review?(cam) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Blocks: 1259345
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: