stylo: implement interpolation for currentColor

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
5 months ago
2 months ago

People

(Reporter: bholley, Assigned: xidorn)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment, 11 obsolete attachments)

59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
See the discussion at bug 760345, which may impact how we implement it.
Xidorn, are there outstanding spec issues, or is this something we can assign to somebody to work on?
Flags: needinfo?(xidorn+moz)
(Assignee)

Comment 2

3 months ago
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).
Flags: needinfo?(xidorn+moz)

Updated

3 months ago
Summary: stylo: implement currentColor → stylo: implement interpolation for currentColor
(Assignee)

Comment 3

2 months ago
I'll look into this... soon.
Assignee: nobody → xidorn+moz
(Assignee)

Updated

2 months ago
Flags: needinfo?(xidorn+moz)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Flags: needinfo?(xidorn+moz)
(Assignee)

Comment 6

2 months ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 months ago
Hmmm, looks like we really should not serialize color to its authored form in general.
(Assignee)

Comment 10

2 months ago
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 hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8874652 - Attachment is obsolete: true
Attachment #8874652 - Flags: review?(manishearth)
(Assignee)

Comment 12

2 months ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

2 months ago
mozreview-review
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

2 months ago
mozreview-review
Comment on attachment 8874704 [details]
Bug 1345709 - Lowercase color keyword in specified value.

https://reviewboard.mozilla.org/r/146076/#review149954
Attachment #8874704 - Flags: review?(cam) → review+
(Assignee)

Comment 17

2 months ago
mozreview-review-reply
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

2 months ago
mozreview-review-reply
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).
(Assignee)

Comment 19

2 months ago
mozreview-review-reply
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

2 months ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Comment 22

2 months ago
mozreview-review-reply
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

2 months ago
mozreview-review
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?)
Attachment #8874651 - Flags: review?(bbirtles) → review+
(Assignee)

Updated

2 months ago
Attachment #8874651 - Flags: review?(manishearth)
(Assignee)

Comment 24

2 months ago
Ok, I guess I can split it a bit, and that would also make the changes clearer to myself.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 36

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=808468df8d007dbb5d8dd6e3b86d32382262e1b3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 48

2 months ago
mozreview-review
Comment on attachment 8875134 [details]
Remove unused CSSRGBA.

https://reviewboard.mozilla.org/r/146500/#review150966
Attachment #8875134 - Flags: review?(manishearth) → review+

Comment 49

2 months ago
mozreview-review
Comment on attachment 8875135 [details]
Move specified color types into specified::color mod.

https://reviewboard.mozilla.org/r/146502/#review150972
Attachment #8875135 - Flags: review?(manishearth) → review+

Comment 50

2 months ago
mozreview-review
Comment on attachment 8875136 [details]
Move ToComputedValue impl of color types into specified::color.

https://reviewboard.mozilla.org/r/146504/#review150974
Attachment #8875136 - Flags: review?(manishearth) → review+

Comment 51

2 months ago
mozreview-review
Comment on attachment 8875137 [details]
Factor out Gecko-specific color keywords.

https://reviewboard.mozilla.org/r/146506/#review150978
Attachment #8875137 - Flags: review?(manishearth) → review+

Comment 52

2 months ago
mozreview-review
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()`?
Attachment #8875138 - Flags: review?(manishearth) → review+

Comment 53

2 months ago
mozreview-review
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?
Attachment #8875139 - Flags: review?(manishearth) → review+

Comment 54

2 months ago
mozreview-review
Comment on attachment 8875140 [details]
Merge CSSColor into Color.

https://reviewboard.mozilla.org/r/146512/#review151016
Attachment #8875140 - Flags: review?(manishearth) → review+

Comment 55

2 months ago
mozreview-review
Comment on attachment 8875141 [details]
Remove complex_color parameter.

https://reviewboard.mozilla.org/r/146514/#review151018
Attachment #8875141 - Flags: review?(manishearth) → review+

Comment 56

2 months ago
mozreview-review
Comment on attachment 8875142 [details]
Simplify caret-color conversion.

https://reviewboard.mozilla.org/r/146516/#review151022
Attachment #8875142 - Flags: review?(manishearth) → review+

Comment 57

2 months ago
mozreview-review
Comment on attachment 8874651 [details]
Add separate computed Color value.

https://reviewboard.mozilla.org/r/146000/#review151034
Attachment #8874651 - Flags: review?(manishearth) → review+
(Assignee)

Comment 58

2 months ago
Servo side: servo/servo#17219
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8875134 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8875135 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8875136 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8875137 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8875138 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8875139 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8875140 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8875141 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8874651 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8875142 - Attachment is obsolete: true

Comment 60

2 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1880a72e41e
Lowercase color keyword in specified value. r=heycam

Comment 61

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