Closed Bug 1367274 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: xidorn, Assigned: ferjm)

References

Details

Attachments

(1 file, 2 obsolete files)

Bug 1367274 - Update test expectations for test_computed_style.html.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
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.
Priority: -- → P2
Assignee: nobody → ferjmoreno
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 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+
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+
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+
(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!
Attachment #8885620 - Attachment is obsolete: true
Attachment #8885621 - Attachment is obsolete: true
Pushed by ferjmoreno@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8e3ae84d8141
Update test expectations for test_computed_style.html. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/8e3ae84d8141
Status: NEW → RESOLVED
Closed: 7 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: