stylo: Serialization of computed value of gradient functions is different between Stylo and Gecko

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: xidorn, Assigned: ferjm)

Tracking

(Blocks: 2 bugs)

53 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 months ago
There are several failures in test_computed_style.html:

> linear-gradient(red, blue)
> gecko: linear-gradient(rgb(255, 0, 0), rgb(0, 0, 255))
> stylo: linear-gradient(180deg, rgb(255, 0, 0), rgb(0, 0, 255))

> linear-gradient(to bottom, red, blue)
> gecko: linear-gradient(180deg, rgb(255, 0, 0), rgb(0, 0, 255))
> stylo: linear-gradient(rgb(255, 0, 0), rgb(0, 0, 255))

> linear-gradient(to right, red, blue)
> gecko: linear-gradient(90deg, rgb(255, 0, 0), rgb(0, 0, 255))
> stylo: linear-gradient(to right, rgb(255, 0, 0), rgb(0, 0, 255))

There are two issues:
1. Servo doesn't preserve keyword values for gradient angle in computed style, which spec doesn't say clearly what should happen
2. Gecko doesn't strip 180deg in nsComputedDOMStyle, which is wrong per spec

We probably want to change Servo code to preserve keyword values, because all browsers currently does so, and doing so would fix all test failures here.

Fixing the Gecko issue is also desirable, but it would leave some failures unchanged, so I think we should probably just change Servo code.

Updated

9 months ago
Priority: -- → P2
(Assignee)

Updated

8 months ago
Assignee: nobody → ferjmoreno
Comment hidden (mozreview-request)
(Reporter)

Comment 2

8 months ago
mozreview-review
Comment on attachment 8885379 [details]
Bug 1367274 - Update test expectations for test_computed_style.html.

https://reviewboard.mozilla.org/r/156230/#review161524

::: servo/components/style/gecko/conversions.rs:219
(Diff revision 1)
>                                           stop_count as u32)
>                  };
>  
>                  match direction {
>                      LineDirection::Angle(angle) => {
> +                        if angle.radians() != PI {

Why do we need this here? Could you add some comment to explain?

::: servo/components/style/values/computed/image.rs:139
(Diff revision 1)
> -        #[cfg(feature = "servo")]
> -        modern_angle
> -    }
> -
>      /// Manually derived to_computed_value
> -    fn to_computed_value(&self, context: &Context, compat_mode: CompatMode) -> LineDirection {
> +    fn to_computed_value(&self, context: &Context, _: CompatMode) -> LineDirection {

If we no longer need that conversion, probably we can remove the `CompatMode` parameter, and make this a normal impl of `ToComputedValue` trait?

It seems to me some callsites of this function would be able to drop the additional parameter as well.

This should probably happen in a separate patch, though.
Attachment #8885379 - Flags: review?(xidorn+moz)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 6

8 months ago
mozreview-review
Comment on attachment 8885379 [details]
Bug 1367274 - Update test expectations for test_computed_style.html.

https://reviewboard.mozilla.org/r/156230/#review161844

::: servo/components/style/gecko/conversions.rs:219
(Diff revision 2)
>                                           stop_count as u32)
>                  };
>  
>                  match direction {
>                      LineDirection::Angle(angle) => {
> +                        // PI radians (180 degrees) is ignored because this is the default value.

I'm fine with this. But in this case, should we also avoid setting anything if it is `LineDirection::Vertical(Y::Bottom)`?

This also makes me think that all the `set_value(CoordDataValue::None)`s below are all useless. Probably we can remove them in a followup.
Attachment #8885379 - Flags: review?(xidorn+moz) → review+
(Reporter)

Comment 7

8 months ago
mozreview-review
Comment on attachment 8885620 [details]
Bug 1367274 - Part 2: Implement ToComputedValue for SpecifiedLineDirection and SpecifiedGradientKind.

https://reviewboard.mozilla.org/r/156466/#review161850

I guess we should be able to derive `ToComputedValue` for `SpecifiedGradient`... but currently we cannot.
Attachment #8885620 - Flags: review?(xidorn+moz) → review+
(Reporter)

Comment 8

8 months ago
mozreview-review
Comment on attachment 8885621 [details]
Bug 1367274 - Update test expectations for test_computed_style.html.

https://reviewboard.mozilla.org/r/156468/#review161856
Attachment #8885621 - Flags: review?(xidorn+moz) → review+
(Assignee)

Comment 9

8 months ago
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #6)
> Comment on attachment 8885379 [details]
> Bug 1367274 - Part 1: preserve linear-gradient keyword values.
> 
> https://reviewboard.mozilla.org/r/156230/#review161844
> 
> ::: servo/components/style/gecko/conversions.rs:219
> (Diff revision 2)
> >                                           stop_count as u32)
> >                  };
> >  
> >                  match direction {
> >                      LineDirection::Angle(angle) => {
> > +                        // PI radians (180 degrees) is ignored because this is the default value.
> 
> I'm fine with this. But in this case, should we also avoid setting anything
> if it is `LineDirection::Vertical(Y::Bottom)`?

Yes, I added this to the patch.

> 
> This also makes me think that all the `set_value(CoordDataValue::None)`s
> below are all useless. Probably we can remove them in a followup.

I took the opportunity to address this comment as well.

Thank you Xidorn!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8885620 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8885621 - Attachment is obsolete: true

Comment 14

8 months ago
Pushed by ferjmoreno@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8e3ae84d8141
Update test expectations for test_computed_style.html. r=xidorn

Comment 15

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8e3ae84d8141
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.