1345709 - stylo: implement interpolation for currentColor

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

See the discussion at bug 760345, which may impact how we implement it.

Comment 1

[Bobby Holley (:bholley)]

Xidorn, are there outstanding spec issues, or is this something we can assign to somebody to work on?

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

It seems to me stylo is already able to keep currentcolor keyword in computed value, but it doesn't seem to be able to interpolate between currentcolor keyword and other values.

To support interpolation, we may need to make Servo's specified color and computed color include a new ratio field for the percentage of currentcolor, just like Gecko's eCSSUnit_ComplexColor of nsCSSValue (specified value) and StyleComplexColor struct (computed value).

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

I'll look into this... soon.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Add separate computed Color value.

This patch refactors the color types, and now we would have only four
color types in code:
* specified::Color -> computed::Color
* specified::RGBAColor -> computed::RGBAColor = cssparser::RGBA
The Color types include support for interpolating between numeric color
and currentcolor, while computed::RGBAColor is just RGBA.

The specified::RGBAColor is a simple struct wrapping Color to make code
generator happier. We want to eventually remove RGBAColor once we have
proper currentcolor support everywhere.

With this refactor, the situation of authored specified color value is
also improved slightly. Now we include an authored string for all parsed
numeric color value, and non-numeric values don't have that field since
that is not needed.

However, we eventually want to normalize the authored form, which would
rely on cssparser to return more detailed information. Some authored
string tests may be regressed because of lack of normalization, but we
can fix that later.

Another regression is that some places which had computed-value time
currentcolor support in Servo but not in Gecko (e.g. stop colors in
gradient value) now don't have that support anymore (which means
currentcolor would be resolved during computation not use time). This
should be fixed in the future as we getting rid of RGBAColor.

Review commit: https://reviewboard.mozilla.org/r/146000/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146000/

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1345709 - Add mechanism in SimpleTest to detect stylo and update test expectation.

Review commit: https://reviewboard.mozilla.org/r/146002/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146002/

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874651 [details]
Add separate computed Color value.

https://reviewboard.mozilla.org/r/146000/#review149910

::: servo/components/style/values/computed/color.rs:101
(Diff revision 1)
> +        let p1 = factor * (255 - fg_ratio) as f32;
> +        let a1 = factor * self.color.alpha as f32;
> +        let r1 = a1 * self.color.red as f32;
> +        let g1 = a1 * self.color.green as f32;
> +        let b1 = a1 * self.color.blue as f32;
> +
> +        let p2 = 1.0 - p1;
> +        let a2 = factor * fg_color.alpha as f32;
> +        let r2 = a2 * fg_color.red as f32;
> +        let g2 = a2 * fg_color.green as f32;
> +        let b2 = a2 * fg_color.blue as f32;
> +
> +        let a = p1 * a1 + p2 * a2;
> +        if a == 0.0 {
> +            return RGBA::transparent();
> +        }
> +
> +        let r = clamp_color((p1 * r1 + p2 * r2) / a);
> +        let g = clamp_color((p1 * g1 + p2 * g2) / a);
> +        let b = clamp_color((p1 * b1 + p2 * b2) / a);
> +        return RGBA::new(r, g, b, (a * 255.0).round() as u8);

Maybe I should just use `RGBA`'s `{alpha,red,green,blue}_f32` and `from_floats` instead.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874651 [details]
Add separate computed Color value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146000/diff/1-2/

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874652 [details]
Bug 1345709 - Add mechanism in SimpleTest to detect stylo and update test expectation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146002/diff/1-2/

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Hmmm, looks like we really should not serialize color to its authored form in general.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

So I guess this should not affect any test at this moment, since the only test checking interpolation of currentcolor as far as I know is test_transition_per_property.html which hasn't been enabled.

We will get better authored color string support at some point, but not with this refactor.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874651 [details]
Add separate computed Color value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146000/diff/2-3/

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874651 [details]
Add separate computed Color value.

https://reviewboard.mozilla.org/r/146000/#review149942

::: layout/style/test/stylo-failures.md:119
(Diff revision 3)
>    * test_units_time.html [1]
>  * getComputedStyle style doesn't contain custom properties bug 1336891
>    * test_variable_serialization_computed.html [35]
>    * test_variables.html `custom property name` [2]
>  * test_css_supports.html: issues around @supports syntax servo/servo#15482 [8]
> -* test_author_specified_style.html: support serializing color as author specified bug 1348165 [27]
> +* test_author_specified_style.html: support serializing color as author specified bug 1348165 [45]

This should be removed.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1345709 - Lowercase color keyword in specified value.

Review commit: https://reviewboard.mozilla.org/r/146076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146076/

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874651 [details]
Add separate computed Color value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146000/diff/3-4/

Comment 15

[Brian Birtles (:birtles)]

Comment on attachment 8874651 [details]
Add separate computed Color value.

https://reviewboard.mozilla.org/r/146000/#review149952

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2714
(Diff revision 2)
> -            // FIXME: Bug 1345709: Implement currentColor animations.
> -            _ => Err(()),
> +        } else if self.is_currentcolor() && other.is_numeric() {
> +            Ok(IntermediateColor {
> +                color: self.color,
> +                foreground_ratio: self_portion as f32,
> +            })
> +        } else if self.is_numeric() && other.is_currentcolor() {
> +            Ok(IntermediateColor {
> +                color: other.color,
> +                foreground_ratio: other_portion as f32,
> +            })
> +        } else {

I don't quite understand this part.

If self_portion is 1.0 and other_portion is 1.0, then the result should be the sum of the current color and the numeric color. It seems like IntermediateColor doesn't let us represent that, though? Perhaps we need two ratios in IntermediateColor?

Also, even for the self_portion + other_portion == 1.0 case, do we really want to use self.color in the first branch since it is transparent? Don't we want other.color?

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8874704 [details]
Bug 1345709 - Lowercase color keyword in specified value.

https://reviewboard.mozilla.org/r/146076/#review149954

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874651 [details]
Add separate computed Color value.

https://reviewboard.mozilla.org/r/146000/#review149952

> I don't quite understand this part.
> 
> If self_portion is 1.0 and other_portion is 1.0, then the result should be the sum of the current color and the numeric color. It seems like IntermediateColor doesn't let us represent that, though? Perhaps we need two ratios in IntermediateColor?
> 
> Also, even for the self_portion + other_portion == 1.0 case, do we really want to use self.color in the first branch since it is transparent? Don't we want other.color?

Ah, my fault. Yeah, the first branch should use `other.color` and the second should use `self.color`.

I'm not completely sure how this should work for additive color. If we have 100% currentcolor + 100% numeric color, what should the final color be?

If we simply clamp everything, we would get a currentcolor eventually because that is the maximum value of `foreground_ratio` means. But that doesn't make much sense, because it happens just because we use foreground ratio rather than rgba ratio. If we do the other way, we get a reverse result... So what really should happen?

Comment 18

[Brian Birtles (:birtles)]

Comment on attachment 8874651 [details]
Add separate computed Color value.

https://reviewboard.mozilla.org/r/146000/#review149952

> Ah, my fault. Yeah, the first branch should use `other.color` and the second should use `self.color`.
> 
> I'm not completely sure how this should work for additive color. If we have 100% currentcolor + 100% numeric color, what should the final color be?
> 
> If we simply clamp everything, we would get a currentcolor eventually because that is the maximum value of `foreground_ratio` means. But that doesn't make much sense, because it happens just because we use foreground ratio rather than rgba ratio. If we do the other way, we get a reverse result... So what really should happen?

If we have 100% currentcolor + 100% numeric color, we should ultimately resolve the result as the sum of the two colors. e.g. if current color resolves to rgb(255,0,0) and the numeric color is rgb(0,255,0), we should get rgb(255,255,0). So it sounds like instead of a foreground_ratio, we need two ratios?

Admittedly, this situation is pretty rare (at least until we ship additive animation in CSS / Web Animations).

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874651 [details]
Add separate computed Color value.

https://reviewboard.mozilla.org/r/146000/#review149952

> If we have 100% currentcolor + 100% numeric color, we should ultimately resolve the result as the sum of the two colors. e.g. if current color resolves to rgb(255,0,0) and the numeric color is rgb(0,255,0), we should get rgb(255,255,0). So it sounds like instead of a foreground_ratio, we need two ratios?
> 
> Admittedly, this situation is pretty rare (at least until we ship additive animation in CSS / Web Animations).

Hmmm, then Gecko doesn't support that either. Gecko's StyleComplexColor only has one ratio at the moment.

I would suggest we don't worry too much about this for now, since this is basically what Gecko is currently doing. We may want to add another ratio field to complex color types eventually at some point.

Comment 20

[Brian Birtles (:birtles)]

Comment on attachment 8874651 [details]
Add separate computed Color value.

https://reviewboard.mozilla.org/r/146000/#review149952

> Hmmm, then Gecko doesn't support that either. Gecko's StyleComplexColor only has one ratio at the moment.
> 
> I would suggest we don't worry too much about this for now, since this is basically what Gecko is currently doing. We may want to add another ratio field to complex color types eventually at some point.

Alright, then as long as we add a comment to that effect, I'm ok with it.

Obviously we still want to swap the self.color / other.color references.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874651 [details]
Add separate computed Color value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146000/diff/4-5/

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874651 [details]
Add separate computed Color value.

https://reviewboard.mozilla.org/r/146000/#review149952

> Alright, then as long as we add a comment to that effect, I'm ok with it.
> 
> Obviously we still want to swap the self.color / other.color references.

Added some comment for this situation, also swapped self.color / other.color.

Comment 23

[Brian Birtles (:birtles)]

Comment on attachment 8874651 [details]
Add separate computed Color value.

https://reviewboard.mozilla.org/r/146000/#review149966

This is a massive patch. Thank you very much for doing all this work!

That said, if at all possible it would be better to split this up in future. I've mostly only skimmed the second half of the patch since it's just too much to concentrate on and I trust you know what you're doing since as far as I can tell this lines up fairly closely with the Gecko code.

::: servo/components/style/values/computed/color.rs:28
(Diff revision 4)
> +fn blend_color_component(bg: u8, fg: u8, fg_alpha: u8) -> u8 {
> +    let bg_ratio = (u8::max_value() - fg_alpha) as u32;
> +    let fg_ratio = fg_alpha as u32;
> +    let color = bg as u32 * bg_ratio + fg as u32 * fg_ratio;
> +    // Rounding divide the number by 255
> +    ((color + 127) / 255) as u8
> +}

(For my own notes, this corresponds to BlendColorComponent in nsColor.cpp[1]

[1] http://searchfox.org/mozilla-central/rev/d441cb24482c2e5448accaf07379445059937080/gfx/src/nsColor.cpp#263)

::: servo/components/style/values/computed/color.rs:144
(Diff revision 4)
> +        if self.is_numeric() {
> +            self.color.to_css(dest)
> +        } else if self.is_currentcolor() {
> +            CSSParserColor::CurrentColor.to_css(dest)
> +        } else {
> +            Ok(())

(Should this warn if we are trying to serialize an intermediate color?)

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

Ok, I guess I can split it a bit, and that would also make the changes clearer to myself.

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Remove unused CSSRGBA.

Review commit: https://reviewboard.mozilla.org/r/146500/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146500/

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Move specified color types into specified::color mod.

Review commit: https://reviewboard.mozilla.org/r/146502/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146502/

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Move ToComputedValue impl of color types into specified::color.

Review commit: https://reviewboard.mozilla.org/r/146504/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146504/

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Factor out Gecko-specific color keywords.

Review commit: https://reviewboard.mozilla.org/r/146506/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146506/

Comment 29

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Unify specified Color type between Stylo and Servo.

Review commit: https://reviewboard.mozilla.org/r/146508/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146508/

Comment 30

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Create RGBAColor for colors compute to RGBA.

This is a transitional value type before we support computed-value time
currentcolor everywhere.

Servo would be slightly regressed for places where Gecko doesn't support
computed-value time currentcolor but Servo does, for example, stop color
in gradients. This should be fixed in the future as we getting rid of
RGBAColor.

Review commit: https://reviewboard.mozilla.org/r/146510/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146510/

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Merge CSSColor into Color.

Review commit: https://reviewboard.mozilla.org/r/146512/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146512/

Comment 32

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Remove complex_color parameter.

Review commit: https://reviewboard.mozilla.org/r/146514/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146514/

Comment 33

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Simplify caret-color conversion.

Review commit: https://reviewboard.mozilla.org/r/146516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146516/

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874704 [details]
Bug 1345709 - Lowercase color keyword in specified value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146076/diff/1-2/

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874651 [details]
Add separate computed Color value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146000/diff/5-6/

Comment 36

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=808468df8d007dbb5d8dd6e3b86d32382262e1b3

Comment 37

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874704 [details]
Bug 1345709 - Lowercase color keyword in specified value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146076/diff/2-3/

Comment 38

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8875134 [details]
Remove unused CSSRGBA.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146500/diff/1-2/

Comment 39

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8875135 [details]
Move specified color types into specified::color mod.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146502/diff/1-2/

Comment 40

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8875136 [details]
Move ToComputedValue impl of color types into specified::color.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146504/diff/1-2/

Comment 41

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8875137 [details]
Factor out Gecko-specific color keywords.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146506/diff/1-2/

Comment 42

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8875138 [details]
Unify specified Color type between Stylo and Servo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146508/diff/1-2/

Comment 43

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8875139 [details]
Create RGBAColor for colors compute to RGBA.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146510/diff/1-2/

Comment 44

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8875140 [details]
Merge CSSColor into Color.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146512/diff/1-2/

Comment 45

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8875141 [details]
Remove complex_color parameter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146514/diff/1-2/

Comment 46

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874651 [details]
Add separate computed Color value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146000/diff/6-7/

Comment 47

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8875142 [details]
Simplify caret-color conversion.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146516/diff/1-2/

Comment 48

[Manish Goregaokar [:manishearth]]

Comment on attachment 8875134 [details]
Remove unused CSSRGBA.

https://reviewboard.mozilla.org/r/146500/#review150966

Comment 49

[Manish Goregaokar [:manishearth]]

Comment on attachment 8875135 [details]
Move specified color types into specified::color mod.

https://reviewboard.mozilla.org/r/146502/#review150972

Comment 50

[Manish Goregaokar [:manishearth]]

Comment on attachment 8875136 [details]
Move ToComputedValue impl of color types into specified::color.

https://reviewboard.mozilla.org/r/146504/#review150974

Comment 51

[Manish Goregaokar [:manishearth]]

Comment on attachment 8875137 [details]
Factor out Gecko-specific color keywords.

https://reviewboard.mozilla.org/r/146506/#review150978

Comment 52

[Manish Goregaokar [:manishearth]]

Comment on attachment 8875138 [details]
Unify specified Color type between Stylo and Servo.

https://reviewboard.mozilla.org/r/146508/#review150980

::: servo/tests/unit/style/properties/serialization.rs:555
(Diff revision 2)
>              let mut properties = Vec::new();
>  
>              let width = BorderSideWidth::Length(Length::from_px(4f32));
>              let style = Either::First(Auto);
>              let color = CSSColor {
> -                parsed: ComputedColor::RGBA(RGBA::new(255, 0, 0, 255)),
> +                parsed: Color::RGBA(RGBA::new(255, 0, 0, 255)),

nit: Could we also have a `From` impl so that all of these can just be `.into()`?

Comment 53

[Manish Goregaokar [:manishearth]]

Comment on attachment 8875139 [details]
Create RGBAColor for colors compute to RGBA.

https://reviewboard.mozilla.org/r/146510/#review150984

::: servo/components/layout/display_list_builder.rs:615
(Diff revision 2)
>      radii
>  }
>  
>  fn convert_gradient_stops(gradient_items: &[GradientItem],
>                            total_length: Au,
> -                          style: &ServoComputedValues) -> Vec<GradientStop> {
> +                          _style: &ServoComputedValues) -> Vec<GradientStop> {

can the argument be removed entirely?

::: servo/components/style/properties/gecko.mako.rs:413
(Diff revision 2)
> +<%def name="impl_rgba_color(ident, gecko_ffi_name, need_clone=False)">
> +    #[allow(non_snake_case)]
> +    pub fn set_${ident}(&mut self, v: longhands::${ident}::computed_value::T) {
> +        ${set_gecko_property(gecko_ffi_name, "convert_rgba_to_nscolor(&v)")}
> +    }
> +<%call expr="impl_simple_copy(ident, gecko_ffi_name)"></%call>

nit: indent this and the following lines?

Comment 54

[Manish Goregaokar [:manishearth]]

Comment on attachment 8875140 [details]
Merge CSSColor into Color.

https://reviewboard.mozilla.org/r/146512/#review151016

Comment 55

[Manish Goregaokar [:manishearth]]

Comment on attachment 8875141 [details]
Remove complex_color parameter.

https://reviewboard.mozilla.org/r/146514/#review151018

Comment 56

[Manish Goregaokar [:manishearth]]

Comment on attachment 8875142 [details]
Simplify caret-color conversion.

https://reviewboard.mozilla.org/r/146516/#review151022

Comment 57

[Manish Goregaokar [:manishearth]]

Comment on attachment 8874651 [details]
Add separate computed Color value.

https://reviewboard.mozilla.org/r/146000/#review151034

Comment 58

[Xidorn Quan [:xidorn] UTC+10]

Servo side: servo/servo#17219

Comment 59

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874704 [details]
Bug 1345709 - Lowercase color keyword in specified value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146076/diff/3-4/

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1880a72e41e
Lowercase color keyword in specified value. r=heycam

Comment 61

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e1880a72e41e