Closed Bug 1239153 Opened 4 years ago Closed 4 years ago

Firefox's CSS parser rejects "-webkit-linear-gradient(0,pink,teal)"

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

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+
https://hg.mozilla.org/mozilla-central/rev/9944efaa00d3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.