1383323 - radial-gradient(circle gold,red) must not work (missing comma)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[j.j.]

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

Comment 1

[j.j.]

Attachment: testcase

Comment 2

[j.j.]

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

Comment 3

[Cameron McCormack (:heycam)]

Let's track this as a stylo behavior change.

Comment 4

[Cameron McCormack (:heycam)]

Xidorn, is it worth fixing Gecko here?

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 6

[Chris Mills [:cmills]]

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

Comment 7

[Kuba Niewiarowski]

So fixed with Stylo, right?

Comment 8

[Chris Mills [:cmills]]

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

Yes.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: 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).

As noted in my code-comment here: Gecko's CSS parser incorrectly accepts this
CSS -- wherease Stylo correctly rejects it. Rather than trying to fix this in
Gecko, I'm just adding it in such a way that our expectation changes depending
on which CSS engine we're using.  This lets us regression-test this in our
now-default stylo configuration, and still detect accidental/unexpected
behavior-changes in Gecko.

Review commit: https://reviewboard.mozilla.org/r/183758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183758/

Comment 10

[Daniel Holbert [:dholbert]]

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

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 12

[Daniel Holbert [:dholbert]]

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?)

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

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);
```

Comment 14

[Daniel Holbert [:dholbert]]

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

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 17

[Daniel Holbert [:dholbert]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183758/diff/1-2/

Comment 18

[Daniel Holbert [:dholbert]]

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Daniel Holbert [:dholbert]]

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

Comment 21

[Phil Ringnalda (:philor)]

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