j.j. Reporter
	Description • 7 years ago

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

A gradient like

 background-image: radial-gradient(circle gold,red);

works in Gecko. Must not work because of missing comma after "circle". It doesn't work in Chrome (and most likely other browsers).

	j.j. Reporter
	Comment 2 • 7 years ago

Hmm, works as expected with Stylo enabled, so should I close this bug?

	Cameron McCormack (:heycam)
	Comment 3 • 7 years ago

Let's track this as a stylo behavior change.

	Cameron McCormack (:heycam)
	Comment 4 • 7 years ago

Xidorn, is it worth fixing Gecko here?

	Xidorn Quan [:xidorn] UTC+11
	Comment 5 • 7 years ago

I don't think it's worth fixing Gecko given that we accept and only document the correct syntax, and it doesn't work in other browsers.

Let's just track it as a stylo behavior change.

	Chris Mills [:cmills]
	Comment 6 • 7 years ago

I've reported this difference in the MDN reference pages, as I think it is worth knowing about. See 

https://developer.mozilla.org/en-US/docs/Web/CSS/radial-gradient#Stylo_notes
https://developer.mozilla.org/en-US/Firefox/Releases/57#Stylo_notes

	Kuba Niewiarowski
	Comment 7 • 7 years ago

So fixed with Stylo, right?

	Chris Mills [:cmills]
	Comment 8 • 7 years ago

(In reply to Kuba Niewiarowski from comment #7)
> So fixed with Stylo, right?

Yes.

	Daniel Holbert [:dholbert] Assignee
	Comment 10 • 7 years ago

Given that Stylo fixes this, I don't think we should bother trying to fix this in Gecko (since the Gecko fix likely wouldn't ship until Gecko's style system is unsupported anyway).

I've posted a patch that just adds this bug's testcase as a mochitest (with the expectation that it's rejected by Stylo, but accepted by Gecko).

	Xidorn Quan [:xidorn] UTC+11
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8912432 [details]
Bug 1383323: Add a property_database.js testcase to verify that we reject radial-gradient() expressions that lack comma between shape and first color (iff stylo is enabled).

https://reviewboard.mozilla.org/r/183758/#review188962

::: layout/style/test/property_database.js:510
(Diff revision 1)
> +// (Note: this isn't technically a "property-enabling pref", so we're slightly
> +// abusing "IsCSSPropertyPrefEnabled" here.  But it's OK; the point is that
> +// we have different expectations depending on whether the pref is enabled.
> +// *And*, we want to be alerted if the pref is removed entirely.)
> +if (IsCSSPropertyPrefEnabled("layout.css.servo.enabled")) {

Probably just use `SpecialPowers.DOMWindowUtils.isStyledByServo` instead.

	Daniel Holbert [:dholbert] Assignee
	Comment 12 • 7 years ago
mozreview-review-reply

Comment on attachment 8912432 [details]
Bug 1383323: Add a property_database.js testcase to verify that we reject radial-gradient() expressions that lack comma between shape and first color (iff stylo is enabled).

https://reviewboard.mozilla.org/r/183758/#review188962

> Probably just use `SpecialPowers.DOMWindowUtils.isStyledByServo` instead.

I think I prefer IsCSSPropertyPrefEnabled, because it gives us the benefit that it's guaranteed to trigger a test failure as soon as the pref is removed (which will remind us to merge this list into the actual main list, which we should do at that point if not sooner).

Maybe SpecialPowers.DOMWindowUtils.isStyledByServo would also trigger a test failure at that point in time (e.g. if that API were ripped out simultaneously to the pref being removed), but I don't know that for sure.

Is that OK with you?  (Maybe SpecialPowers.DOMWindowUtils.isStyledByServo is better for reasons I'm unaware of?)

	Xidorn Quan [:xidorn] UTC+11
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8912432 [details]
Bug 1383323: Add a property_database.js testcase to verify that we reject radial-gradient() expressions that lack comma between shape and first color (iff stylo is enabled).

https://reviewboard.mozilla.org/r/183758/#review188966

::: layout/style/test/property_database.js:515
(Diff revision 1)
> +  Array.prototype.push.apply(invalidGradientAndElementValues,
> +                             gradientsNewlyRejectedInStylo);

It is probably clearer to do
```javascript
invalidGradientAndElementValues.push(...gradientsNewlyRejectedInStylo);
```

	Daniel Holbert [:dholbert] Assignee
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8912432 [details]
Bug 1383323: Add a property_database.js testcase to verify that we reject radial-gradient() expressions that lack comma between shape and first color (iff stylo is enabled).

https://reviewboard.mozilla.org/r/183758/#review188968

::: layout/style/test/property_database.js:515
(Diff revision 1)
> +  Array.prototype.push.apply(invalidGradientAndElementValues,
> +                             gradientsNewlyRejectedInStylo);

Thanks, I'll do this. Clearly my modern-JS knowledge is lacking. :)

(I cribbed this code from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/push#Merging_two_arrays , BTW).

	Xidorn Quan [:xidorn] UTC+11
	Comment 15 • 7 years ago
mozreview-review-reply

Comment on attachment 8912432 [details]
Bug 1383323: Add a property_database.js testcase to verify that we reject radial-gradient() expressions that lack comma between shape and first color (iff stylo is enabled).

https://reviewboard.mozilla.org/r/183758/#review188962

> I think I prefer IsCSSPropertyPrefEnabled, because it gives us the benefit that it's guaranteed to trigger a test failure as soon as the pref is removed (which will remind us to merge this list into the actual main list, which we should do at that point if not sooner).
> 
> Maybe SpecialPowers.DOMWindowUtils.isStyledByServo would also trigger a test failure at that point in time (e.g. if that API were ripped out simultaneously to the pref being removed), but I don't know that for sure.
> 
> Is that OK with you?  (Maybe SpecialPowers.DOMWindowUtils.isStyledByServo is better for reasons I'm unaware of?)

`isStyledByServo` detects the actual backend used in the document, which has both pros and cons. There are cases where a document can be styled by gecko backend even when the pref of stylo is enabled, which means checking pref can lead to wrong result, but it also means that if stylo is silently disabled internally, no one would notice.

In my mind, `isStyledByServo` should be removed at the same time when we remove the old style system (and the pref), so I don't think that is a problem.

I would probably prefer we have one unified mechanism to check stylo backend rather than check pref at some places while using the internal property at other places.

	Xidorn Quan [:xidorn] UTC+11
	Comment 16 • 7 years ago
mozreview-review-reply

Comment on attachment 8912432 [details]
Bug 1383323: Add a property_database.js testcase to verify that we reject radial-gradient() expressions that lack comma between shape and first color (iff stylo is enabled).

https://reviewboard.mozilla.org/r/183758/#review188968

> Thanks, I'll do this. Clearly my modern-JS knowledge is lacking. :)
> 
> (I cribbed this code from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/push#Merging_two_arrays , BTW).

I'm not super familiar with that either... I thought there should be some modern syntax for that, but it also took me a while to figure out what that is.

	Daniel Holbert [:dholbert] Assignee
	Comment 18 • 7 years ago

Try run with updated patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f02be77627d

	Pulsebot
	Comment 19 • 7 years ago

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd98557b7d2a
Add a property_database.js testcase to verify that we reject radial-gradient() expressions that lack comma between shape and first color (iff stylo is enabled). r=xidorn

	Daniel Holbert [:dholbert] Assignee
	Comment 20 • 7 years ago

Resolving as fixed by stylo. (Can't actually put stylo in depends-on field because that'd create a cycle, so I'll put it in the blocks field, so it's connected at least.)

	Phil Ringnalda (:philor)
	Comment 21 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fd98557b7d2a