Closed
Bug 1372821
Opened 7 years ago
Closed 7 years ago
stylo: linear-gradient is in wrong direction
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Tracking
()
RESOLVED
FIXED
People
(Reporter: shinglyu, Assigned: shinglyu)
References
Details
Attachments
(2 files)
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•7 years 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•7 years 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•7 years 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)
Comment 4•7 years ago
|
||
Yes, just don't derive ToComputedStyle and write it by hand.
Flags: needinfo?(nox)
Comment 5•7 years ago
|
||
(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)
Comment 6•7 years ago
|
||
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•7 years 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•7 years 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
Comment 9•7 years ago
|
||
(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•7 years 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•7 years ago
|
||
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.
Comment 12•7 years ago
|
||
(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•7 years ago
|
Assignee: nobody → shing.lyu
Assignee | ||
Comment 13•7 years 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•7 years 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 15•7 years ago
|
||
No regression so far on Gecko reftest https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d77dc39db35879d83300535c0ea2be96834f03a
Assignee | ||
Comment 16•7 years ago
|
||
https://github.com/servo/servo/pull/17428
Assignee | ||
Comment 17•7 years ago
|
||
Fixed by https://github.com/servo/servo/pull/17428
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•