Closed Bug 1391432 Opened 3 years ago Closed 3 years ago

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

Categories

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

Unspecified
All
defect

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- disabled
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: jdm, Assigned: canova)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Attached file testcase
Stylo reports the testcase as a CSS error while Gecko deals with it just fine.
Attachment #8898483 - Attachment filename: file_1391432.txt → file_1391432.html
Attachment #8898483 - Attachment mime type: text/plain → text/html
-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.
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.
Has STR: --- → yes
OS: Unspecified → All
Version: unspecified → Trunk
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.
Hm, Nazim, didn't we implement -moz-linear-gradient in bug 1366544?
Flags: needinfo?(canaltinova)
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.
Summary: stylo: vendor-prefixed linear gradients are not recognized → stylo: -moz-linear-gradient's direction parsing doesn't always work
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.
Flags: needinfo?(canaltinova)
Assignee: nobody → canaltinova
Status: NEW → ASSIGNED
Priority: -- → P2
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.
I don't know the details of position/gradient parsing well enough to feel comfortable reviewing this.
Attachment #8898495 - Flags: review?(josh) → review?(emilio+bugs)
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"));
```
Attachment #8898495 - Flags: review?(emilio+bugs) → review+
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7314d7897f58
stylo: Add test cases for -moz-linear-gradient with single keyword  r=emilio
https://hg.mozilla.org/mozilla-central/rev/7314d7897f58
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(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.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1392894
You need to log in before you can comment on or make changes to this bug.