stylo: linear-gradient is in wrong direction

RESOLVED FIXED

Status

()

P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: shinglyu, Assigned: shinglyu)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

a year ago
Created attachment 8877443 [details]
testcase.html

Stylo's linear gradient is in the wrong direction, the expected direction is top to bottom, but the rendered effect is left to right.

The computed style (by window.getComputedStyle()) is also different:

Gecko computed style
-moz-linear-gradient(50% 0%, rgb(245, 161, 102) 0%, rgb(242, 112, 46) 100%, rgb(242, 112, 46) 100%)

Stylo computed style
-webkit-linear-gradient(0deg, rgb(245, 161, 102) 0%, rgb(242, 112, 46) 100%, rgb(242, 112, 46) 100%)

You can find the img#searchGo element in the attached test case to reproduce this bug.

Reftest analyzer link: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/EdIYXP0ETq6fbzB5A07xOg/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1
(Assignee)

Comment 1

a year ago
The specified CSS are:

background: -moz-linear-gradient(top, #f5a166 0%, #f2702e 100%, #f2702e 100%);
background: -webkit-gradient(linear, left top, left bottom, color-stop(0%, #f5a166), color-stop(100%, #f2702e), color-stop(100%, #f2702e));
background: -webkit-linear-gradient(top, #f5a166 0%, #f2702e 100%, #f2702e 100%);
background: -o-linear-gradient(top, #f5a166 0%, #f2702e 100%, #f2702e 100%);
background: -ms-linear-gradient(top, #f5a166 0%, #f2702e 100%, #f2702e 100%);

The one that wins in stylo is 

background: -webkit-linear-gradient(top, #f5a166 0%, #f2702e 100%, #f2702e 100%);

Look like in Stylo we are handling -webkit-linear-gradient(top, ...)'s first argument incorrectly

Gecko's left, top, right, bottom corresponds to Stylo's top, left, bottom, right.
(Assignee)

Comment 2

a year ago
http://searchfox.org/mozilla-central/source/servo/components/style/values/computed/image.rs#99

Looks like servo naively convert the side keyword to angle, but in webkit prefixed version 0deg == right, but in unprefixed linear-gradient 0deg == top, so we need to take CompatMode into consideration.
(Assignee)

Comment 3

a year ago
@nox: Do you know how we can pass the CompatMode to to_computed_style()? I saw you did many things with the generic version, is that doable?
Flags: needinfo?(nox)
Yes, just don't derive ToComputedStyle and write it by hand.
Flags: needinfo?(nox)
(In reply to Shing Lyu [:shinglyu] from comment #2)
> http://searchfox.org/mozilla-central/source/servo/components/style/values/
> computed/image.rs#99
> 
> Looks like servo naively convert the side keyword to angle, but in webkit
> prefixed version 0deg == right, but in unprefixed linear-gradient 0deg ==
> top, so we need to take CompatMode into consideration.

Are you sure about that? The specification for -webkit-linear-gradient says 0deg is left.

https://compat.spec.whatwg.org/#css-gradients-webkit-linear-gradient
https://www.w3.org/TR/2011/WD-css3-images-20110217/#linear-gradients

> If a ‘left’, ‘bottom’, ‘right’, or ‘top’ is given, the used value of the
> gradient is 0deg, 90deg, 180deg, or 270 deg, respectively.
Flags: needinfo?(shing.lyu)
I've thought a bit more about this, and in my opinion we should rather just handle the computed angle according to the compat mode when converting it to a Gecko gradient, not in ToComputedStyle, which needs to preserve the compat mode anyway for serialisation.
(Assignee)

Comment 7

a year ago
The MDN document [0] says the prefixed one has 0deg == east

> ... the prefixed variants of the syntax all used an <angle> defined like polar angles, that is, with 0deg representing the East. To be coherent with the rest of CSS, the specification defines an angle with 0deg representing the North. To prevent sites using prefixed version of the property from suddenly braking, even when adapting to the otherwise forward-compatible final syntax, they kept the original angle definition (0deg = East). Browsers will switch to the correct spec when unprefixing the property. 

My test case also confirms this behavior in Gecko

I agree that we should handle the angle conversion during the Servo to Gecko conversion stage. I believe Servo should stick with the latest spec. 

[0] https://developer.mozilla.org/zh-TW/docs/Web/CSS/linear-gradient11
Flags: needinfo?(shing.lyu)
(Assignee)

Comment 8

a year ago
> Are you sure about that? The specification for -webkit-linear-gradient says
> 0deg is left.

Ah you are righ. 0deg = to east = from left
(In reply to Shing Lyu [:shinglyu] from comment #7)
> I agree that we should handle the angle conversion during the Servo to Gecko
> conversion stage. I believe Servo should stick with the latest spec. 

FYI, Xidorn recently added a mechanism in servo/servo#17316 to say that for certain properties, we pass a &Device as an additional argument to the glue setter function, which you could use here.
(Assignee)

Comment 10

a year ago
With the following patch I can convert the angle during the Servo to Gecko conversion

>                 match direction {
>                     LineDirection::Angle(angle) => {
>+                        let modern_angle = match gradient.compat_mode {
>+                            CompatMode::Modern => angle,
>+                            CompatMode::WebKit => Angle::Radian((angle.radians() * -1.0) + (PI * 3.0/2.0)),
>+                        };

But the problem is that this only works when the specified value is a [left|top|right|bottom], if the specified value is an angle (Xdeg), then we don't need the conversion. 

So maybe we need to delay the time we convert the keyword into angle? The old webkit spec said:

> To be precise, the gradient is converted to the angle form described in the previous paragraph at used-value time.

We can then convert it to angle in the layout code, but we need to pass the CompatMode into layout.

Or maybe we should complete separate the webkit prefix version from the non-prefixed version? Because their meaning of Angle is totally different.

---
(In reply to Cameron McCormack (:heycam) from comment #9)
> FYI, Xidorn recently added a mechanism in servo/servo#17316 to say that for
> certain properties, we pass a &Device as an additional argument to the glue
> setter function, which you could use here.

Thanks! I'll keep that in mind.
Flags: needinfo?(nox)
(Assignee)

Comment 11

a year ago
Created attachment 8877884 [details]
Gecko's gradient screenshot

This is how Gecko behaves. The `linear-gradien()` doesn't support [left|top|right|bottom] as its first keyword (must have "to" before the direction), so they are blank.
(In reply to Shing Lyu [:shinglyu] from comment #10)
> With the following patch I can convert the angle during the Servo to Gecko
> conversion
> 
> >                 match direction {
> >                     LineDirection::Angle(angle) => {
> >+                        let modern_angle = match gradient.compat_mode {
> >+                            CompatMode::Modern => angle,
> >+                            CompatMode::WebKit => Angle::Radian((angle.radians() * -1.0) + (PI * 3.0/2.0)),
> >+                        };
> 
> But the problem is that this only works when the specified value is a
> [left|top|right|bottom], if the specified value is an angle (Xdeg), then we
> don't need the conversion.

Oh I see! Then you do need to do it in `ToComputedValue::to_computed_value`, which thus can't be derived.
Flags: needinfo?(nox)
(Assignee)

Updated

a year ago
Assignee: nobody → shing.lyu
(Assignee)

Comment 13

a year ago
I got a working patch, but where do you suggest we put the testcase? servo/tests/wpt/mozilla? or maybe we don't test webkit prefixed stuff?
Flags: needinfo?(nox)
(Assignee)

Comment 14

a year ago
> I got a working patch, but where do you suggest we put the testcase?
> servo/tests/wpt/mozilla? or maybe we don't test webkit prefixed stuff?

heycam suggest we put it in servo/tests/wpt/web-platform-tests/compat/
Flags: needinfo?(nox)
(Assignee)

Comment 17

a year ago
Fixed by https://github.com/servo/servo/pull/17428
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.