Daniel Holbert [:dholbert] Assignee
	Description • 8 years ago

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

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

	Daniel Holbert [:dholbert] Assignee
	Comment 1 • 8 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.)

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 2 • 8 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 3 • 8 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 4 • 8 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.

	Daniel Holbert [:dholbert] Assignee
	Comment 5 • 8 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.

	Daniel Holbert [:dholbert] Assignee
	Comment 6 • 8 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 7 • 8 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.)

	Masatoshi Kimura [:emk]
	Comment 8 • 8 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.

	Cameron McCormack (:heycam)
	Comment 9 • 8 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?

	Daniel Holbert [:dholbert] Assignee
	Comment 10 • 8 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.

	Cameron McCormack (:heycam)
	Comment 11 • 8 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.

	Pulsebot
	Comment 12 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 13 • 8 years ago
bugherder

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