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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Chrome renders a gradient in the attached testcase. We do not.

The expression in question is:
  -webkit-linear-gradient(0,pink,teal);

Comment 1

[Daniel Holbert [:dholbert]]

(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.)

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Presumably this is because they allow unitless 0 for angles, whereas we don't.  See also VARIANT_ZERO_ANGLE in nsCSSParser.cpp.

Comment 3

[Daniel Holbert [:dholbert]]

Ah, yes -- that's it. If I replace "0" with "0deg", then we match Chrome and render the gradient.

Comment 4

[Daniel Holbert [:dholbert]]

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.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v1

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.

Comment 6

[Daniel Holbert [:dholbert]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c1442b67e04

Comment 7

[Daniel Holbert [:dholbert]]

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

[Masatoshi Kimura [:emk]]

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

[Cameron McCormack (:heycam)]

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?

Comment 10

[Daniel Holbert [:dholbert]]

(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

[Cameron McCormack (:heycam)]

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.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9944efaa00d3

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9944efaa00d3