1391432 - stylo: -moz-linear-gradient's direction parsing doesn't always work

Status: VERIFIED FIXED

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

Description

[Josh Matthews [:jdm]]

Attachment: testcase

Stylo reports the testcase as a CSS error while Gecko deals with it just fine.

Comment 1

[Josh Matthews [:jdm]]

-webkit-linear-gradient works fine. This makes me suspect that https://dxr.mozilla.org/servo/rev/fbabcaf614662a52eace21a64f27802c7eff4304/components/style/values/specified/image.rs#196 is not recognized for some reason.

Comment 2

[Darkspirit]

Beta 56.0b3 x64 20170815141045 + Nightly 57 x64 20170817100132 @ Debian Testing:
Grey-to-Black horizontal background gradient with Gecko
vs. just a white background with Stylo.

Comment 3

[Josh Matthews [:jdm]]

Ok, it's more complicated than that. Stylo apparently does not like those arguments for the -moz-linear-gradient; a basic `-moz-linear-gradient(red, yellow)` works fine.

Comment 4

[Bobby Holley (:bholley)]

Hm, Nazim, didn't we implement -moz-linear-gradient in bug 1366544?

Comment 5

[Josh Matthews [:jdm]]

Working:
-moz-linear-gradient(top,rgba(0,0,0,0.3) 0,#000 100%)
Broken:
-moz-linear-gradient(left,rgba(0,0,0,0.3) 0,#000 100%)

Something about the parsing for LegacyPosition seems to be incorrect.

Comment 6

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1391432 - stylo: Add test cases for -moz-linear-gradient with single keyword

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

Comment 7

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Oh, interestingly we have tons of tests about -moz- prefixed gradients but we don't have a test for this. There should be another return branch in the LegacyPosition parsing code. Sorry about that.

Comment 8

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8898495 [details]
Bug 1391432 - stylo: Add test cases for -moz-linear-gradient with single keyword

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

Comment 9

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

This looks better.

Comment 10

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Oh, try run of this patch also showed a bug in gecko. Computed value of `-moz-linear-gradient(bottom, red, blue)` is wrong. See bug 1391534. Changing the test case because of that.

Comment 11

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8898495 [details]
Bug 1391432 - stylo: Add test cases for -moz-linear-gradient with single keyword

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

Comment 12

[Josh Matthews [:jdm]]

I don't know the details of position/gradient parsing well enough to feel comfortable reviewing this.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8898495 [details]
Bug 1391432 - stylo: Add test cases for -moz-linear-gradient with single keyword

https://reviewboard.mozilla.org/r/169850/#review175458

Heh, I had already taken a look at this patch yesterday... r=me, thanks for fixing!

Should we also add a reftest for this? Or do you think parsing tests are enough?

::: servo/components/style/values/specified/position.rs:332
(Diff revision 3)
>                  }
>                  let x_pos = OriginComponent::Side(x_keyword);
>                  if let Ok(y_lop) = input.try(|i| LengthOrPercentage::parse_quirky(context, i, allow_quirks)) {
>                      return Ok(Self::new(x_pos, OriginComponent::Length(y_lop)))
>                  }
> +                let _ = input.try(|i| i.expect_ident_matching("center"));

Maybe use the same structure as the block below:

```
let y_pos = OriginComponent::Center;
let _ = input.try(|i| i.expect_ident_matching("center"));
```

Comment 14

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8898495 [details]
Bug 1391432 - stylo: Add test cases for -moz-linear-gradient with single keyword

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169850/diff/3-4/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7314d7897f58
stylo: Add test cases for -moz-linear-gradient with single keyword  r=emilio

Comment 16

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Servo side: https://github.com/servo/servo/pull/18144

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7314d7897f58

Comment 18

[Darkspirit]

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #2)
> Beta 56.0b3 x64 20170815141045 + Nightly 57 x64 20170817100132 @ Debian Testing:
> Grey-to-Black horizontal background gradient with Gecko
> vs. just a white background with Stylo.

Verified fixed in Nightly 57 x64 20170820100343 @ Debian Testing.