Closed Bug 1465307 Opened 6 years ago Closed 6 years ago

Extend StyleComplexColor to support other blends besides linear interpolation.

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: u480271, Assigned: u480271)

References

Details

Attachments

(4 files)

Bug 1465307 - P1: Extend StyleComplexColor to support additive blending.
59 bytes, text/x-review-board-request
hiro
: review+
xidorn
: review+
Details
Bug 1465307 - P2: Fix nsStyleBorder::mBorderColor for GCC.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1465307 - P3: Extract {animated,computed}::Color common parts.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1465307 - P4: Enable disabled dom/smil lighting_color tests.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
Bug 1457353 exposed a change in color computation that causes SMIL <animate from="rgb(10,20,30)" by="currentColor" ... /> to fail.
Assignee: nobody → dglastonbury
Priority: -- → P3
> ::: servo/components/style/values/animated/color.rs:105
> (Diff revision 2)
> > +    Complex {
> > +        color: RGBA,
> > +        // ratios representing the contribution of color and
> > +        // currentColor to the final color, where:
> > +        // `(bgRatio, fgRatio) = self.ratios`.
> > +        ratios: (f32, f32)
> 
> Should we split this tuple into two different variables?  Actually during
> reviewing this patch I don't see which one is for bg or fg.

I've replaced the tuple with a struct with named components. I hope this makes it clean which ratio is related to what color.
> ::: servo/components/style/values/animated/color.rs:230
> (Diff revision 2)
> > -                self.foreground_ratio
> > -                    .compute_squared_distance(&other.foreground_ratio)?)
> > +                   self_ratios.0.compute_squared_distance(&other_ratios.0)? +
> > +                   self_ratios.1.compute_squared_distance(&other_ratios.1)?)
> > +            },
> 
> Oh, I didn't know that we calcurate foreground_ratio  as if it's in the same
> distance space for color.

Because it's possible to have the same `color` and `foreground_ratio` but different `background_ratio` I extended it. The `foreground_ratio` addition was already in the code and I can't comment on whether this is valid or not.
> ::: layout/style/StyleComplexColor.h:66
> (Diff revision 2)
> > -  bool IsNumericColor() const { return mForegroundRatio == 0; }
> > -  bool IsCurrentColor() const { return mForegroundRatio == 255; }
> > +  bool IsAuto() const { return mTag == eAuto; }
> > +  bool IsCurrentColor() const { return mTag == eCurrent; }
> >  
> >    bool operator==(const StyleComplexColor& aOther) const {
> > -    return mForegroundRatio == aOther.mForegroundRatio &&
> > -           (IsCurrentColor() || mColor == aOther.mColor) &&
> > +    if (mTag != aOther.mTag) {
> > +      return false;
>
> Maybe assert in the case when one is complex and the other is numeric or foreground, and they don't happen to have equal value?

In discussion on IRC with :xidorn, decided to put assert into construction of `StyleComplexColor`s instead.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #32)
> Comment on attachment 8980195 [details]
> Bug 1457353 - P3: Disable failing SMIL tests.
> 
> https://reviewboard.mozilla.org/r/246350/#review253892
> 
> ::: servo/components/style/values/animated/color.rs:97
> (Diff revision 2)
> > -pub struct Color {
> > -    pub color: RGBA,
> > -    pub foreground_ratio: f32,
> > +pub enum Color {
> > +    Numeric { color: RGBA },
> > +    Current,
> > +    Complex {
> > +        color: RGBA,
> > +        // ratios representing the contribution of color and
> > +        // currentColor to the final color, where:
> > +        // `(bgRatio, fgRatio) = self.ratios`.
> > +        ratios: (f32, f32)
> > +    },
> >  }
> 
> Given you are bascially duplicating the enum in `animated` and `computed`,
> you should move this enum into `generics` mod and make it generic over
> `RGBA`, then alias it in the two mods. That would allow you to avoid lots of
> duplicates below as well.
> 
> Actually, I'm suspecting that we probably should move `RGBA` into `generics`
> as well and make it generic over the field type.

I moved the `Color` enum into `generics` and removed and common code from `animated::Color` and `computed::Color`.

I didn't move `RGBA` because there's only one definition in this crate.  The other `RGBA` comes from the separate `cssparser` crate.
Comment on attachment 8983304 [details]
Bug 1465307 - P1: Extend StyleComplexColor to support additive blending.

https://reviewboard.mozilla.org/r/249204/#review255362

Looks good to me other than From trait for ComplexColorRatios.  Thanks!

::: servo/components/style/values/animated/color.rs:170
(Diff revision 1)
> -                    foreground_ratio: self.foreground_ratio,
> -                })
> -            }
> -        } else if self.is_currentcolor() && other.is_numeric() {
> -            Ok(Color {
> -                color: other.color,
> +                color: c1.animate(&c2, procedure)?,
> +            }),
> +            // Combinations of numeric color and currentColor
> +            (Color::Foreground, Color::Numeric { color }, _) => Ok(Color::Complex {
> +                color: color.clone(),
> +                ratios: (other_weight, this_weight).into(),

Unfortunately, using From trait here makes this tuple unclear, which one is for bg or fg.  So we should directly use ComplexColorRatios { bg: xx, fg: xx }?

::: servo/components/style/values/computed/color.rs:30
(Diff revision 1)
> +impl From<(f32, f32)> for ComplexColorRatios {
> +    fn from(x: (f32, f32)) -> Self {
> +        ComplexColorRatios { bg: x.0, fg: x.1 }
> +    }
> +}
>  
> -    /// The ratio of currentcolor in complex color.
> -    pub foreground_ratio: u8,
> +impl From<(f64, f64)> for ComplexColorRatios {
> +    fn from(x: (f64, f64)) -> Self {
> +        ComplexColorRatios {
> +            bg: x.0 as f32,
> +            fg: x.1 as f32,
> +        }
> +    }
> +}

I think these Trait shouldn't be used.
Attachment #8983304 - Flags: review?(hikezoe) → review+
Comment on attachment 8983304 [details]
Bug 1465307 - P1: Extend StyleComplexColor to support additive blending.

https://reviewboard.mozilla.org/r/249204/#review255664

Alright, looks mostly fine. Some small issues below.

::: layout/style/StyleComplexColor.h:29
(Diff revision 3)
> - * the actual algorithm.
>   *
>   * It can also represent an "auto" value, which is valid for some
> - * properties. See comment of mIsAuto.
> + * properties. See comment of `Tag::eAuto`.
>   */
> -struct StyleComplexColor
> +class StyleComplexColor

This can probably be marked `final`.

::: layout/style/StyleComplexColor.h:32
(Diff revision 3)
> - * properties. See comment of mIsAuto.
> + * properties. See comment of `Tag::eAuto`.
>   */
> -struct StyleComplexColor
> +class StyleComplexColor
>  {
> -  nscolor mColor;
> -  uint8_t mForegroundRatio;
> +public:
> +  enum Tag : uint8_t {

It seems to me nothing related to `Tag` is exposed, so maybe you can have this enum private as well.

::: layout/style/StyleComplexColor.h:33
(Diff revision 3)
>   */
> -struct StyleComplexColor
> +class StyleComplexColor
>  {
> -  nscolor mColor;
> -  uint8_t mForegroundRatio;
> +public:
> +  enum Tag : uint8_t {
> -  // Whether the complex color represents a computed-value time auto
> +    // Whether the complex color represents a computed-value time auto

Since it is no longer a flag, it probably makes more sense to remove "Whether" from this comment and below. Something like "This represents a ..." is probably enough.

::: layout/style/StyleComplexColor.h:106
(Diff revision 3)
>     * the foreground color from aFrame's ComputedStyle.
>     */
>    nscolor CalcColor(const nsIFrame* aFrame) const;
> -};
>  
> +protected:

For a final class (which cannot be inherited by anything), `protected` section doesn't make much sense. Please just move it to `private`.

::: servo/components/style/gecko_bindings/sugar/style_complex_color.rs:9
(Diff revision 3)
> +use gecko_bindings::structs::{StyleComplexColor_Tag_eAuto, StyleComplexColor_Tag_eComplex,
> +                              StyleComplexColor_Tag_eForeground, StyleComplexColor_Tag_eNumeric};

I think you can add `StyleComplexColor_Tag` into `rusty-enums` in `ServoBindings.toml`, then you can just do
```rust
use gecko_bindings::structs::StyleComplexColor_Tag as Tag;
```
here, and code below can be much simplified.

Maybe this would also avoid the warning you are working around below.

::: servo/components/style/gecko_bindings/sugar/style_complex_color.rs:52
(Diff revision 3)
>  }
>  
>  impl From<ComputedColor> for StyleComplexColor {
>      fn from(other: ComputedColor) -> Self {
> +        match other {
> +            ComputedColor::Numeric { color } => convert_rgba_to_nscolor(&color).into(),

Maybe it would be better to just `impl From<RGBA> for StyleComplexColor`.

::: servo/components/style/gecko_bindings/sugar/style_complex_color.rs:95
(Diff revision 3)
> +                        bg: other.mBgRatio,
> +                        fg: other.mFgRatio,
> +                    },
> +                }
> +            }
> +            x => panic!("Unsupport StyleComplexColor with tag {}", x),

I would prefer `unreachable` in this case over `panic`.

Also with `StyleComplexColor_Tag` being a rusty enum, you should be able to explicitly list `Tag::eAuto` here instead.

::: servo/components/style/values/animated/color.rs:109
(Diff revision 3)
> -    pub color: RGBA,
> -    pub foreground_ratio: f32,
> +    Numeric {
> +        color: RGBA,
> +    },
> +    Foreground,
> +    Complex {
> +        color: RGBA,
> +        ratios: ComplexColorRatios,
> +    },

I guess you can remove the names here like:
```rust
pub enum Color {
    Numeric(RGBA),
    Foreground,
    Complex(RGBA, ComplexColorRatios),
}
```

The types themselves should already be pretty clear what they are in this case, and thus the additional names don't seem to add much value.

::: servo/components/style/values/animated/color.rs:223
(Diff revision 3)
>  
>  impl ComputeSquaredDistance for Color {
>      #[inline]
>      fn compute_squared_distance(&self, other: &Self) -> Result<SquaredDistance, ()> {
>          // All comments from the Animate impl also applies here.
> -        if self.foreground_ratio == other.foreground_ratio {
> +        match (*self, *other) {

Since you are returning `Ok` everywhere, you can probably have it wrap the `match` expression at the top level.

It's not a big deal, so up to which way you would prefer.

::: servo/components/style/values/animated/color.rs:228
(Diff revision 3)
> -            }
> -        } else if self.is_currentcolor() && other.is_numeric() {
> -            Ok(
> -                RGBA::transparent().compute_squared_distance(&other.color)? +
> -                    SquaredDistance::from_sqrt(1.),
> -            )
> +            (Color::Foreground, Color::Numeric { color }) => Ok(RGBA::transparent()
> +                .compute_squared_distance(&color)?
> +                + SquaredDistance::from_sqrt(1.)),
> +            (Color::Numeric { color }, Color::Foreground) => Ok(color
> +                .compute_squared_distance(&RGBA::transparent())?
> +                + SquaredDistance::from_sqrt(1.)),

These two branches can be merged together. `computed_squared_distance` should be symmetric.

::: servo/components/style/values/computed/color.rs:35
(Diff revision 3)
> +    Numeric {
> +        /// The color
> +        color: RGBA,
> +    },
> +
> +    /// The current foreground color.
> +    Foreground,
> +
> +    /// A linear combination of numeric color and currentColor.
> +    /// The formula is: `color * bg_ratio + currentColor * fg_ratio`.
> +    Complex {
> +        /// The background color
> +        color: RGBA,
> +        /// The ratios of background and foreground colors in color.
> +        /// `(bg_ratio, fg_ratio) = ratios`
> +        ratios: ComplexColorRatios,
> +    },

As I mentioned above, just using tuples in this enum should be enough.

(I still don't get why you cannot move it into generics, but I guess there are reasons. I can probably try it myself afterwards and see why that's going to be a problem.)

::: servo/components/style/values/computed/color.rs:78
(Diff revision 3)
> -        self.foreground_ratio == 0
> +        match *self {
> +            Color::Numeric { .. } => true,
> +            _ => false,
> +        }

`matches!(*self, Color::Numeric { .. })`

::: servo/components/style/values/computed/color.rs:86
(Diff revision 3)
> -        self.foreground_ratio == u8::max_value()
> +        match *self {
> +            Color::Foreground => true,
> +            _ => false,
> +        }

`matches!(*self, Color::Foreground)`

::: servo/components/style/values/computed/color.rs:125
(Diff revision 3)
>  
>          let a = p1 * a1 + p2 * a2;
> -        if a == 0.0 {
> +        if a <= 0. {
>              return RGBA::transparent();
>          }
> +        let a = if a > 1. { 1. } else { a };

Maybe `f32::min(a, 1.)`.
Attachment #8983304 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8983305 [details]
Bug 1465307 - P2: Fix nsStyleBorder::mBorderColor for GCC.

https://reviewboard.mozilla.org/r/249206/#review255672

Ideally this should be moved to the first patch so that all builds work even between the patches. But it's not a big deal.
Attachment #8983305 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8983304 [details]
Bug 1465307 - P1: Extend StyleComplexColor to support additive blending.

https://reviewboard.mozilla.org/r/249204/#review255664

> As I mentioned above, just using tuples in this enum should be enough.
> 
> (I still don't get why you cannot move it into generics, but I guess there are reasons. I can probably try it myself afterwards and see why that's going to be a problem.)

Oh, it seems you are moving `Color` to `generics` in a later patch.
Comment on attachment 8983306 [details]
Bug 1465307 - P3: Extract {animated,computed}::Color common parts.

https://reviewboard.mozilla.org/r/249208/#review255676

There are several issues in part 1 you may want to address first, and this has some issues as well, so canceling review for now.

::: servo/components/style/values/computed/color.rs:12
(Diff revision 3)
>  use cssparser::{Color as CSSParserColor, RGBA};
>  use std::fmt;
>  use style_traits::{CssWriter, ToCss};
>  use values::animated::ToAnimatedValue;
> -use values::animated::color::{Color as AnimatedColor, RGBA as AnimatedRGBA};
> -
> +use values::animated::color::RGBA as AnimatedRGBA;
> +use values::generics::color::{Color as GenericColor, Complex, Foreground, Numeric, Transparent};

You can do
```rust
use values::generics::color as generics;
```
then in the rest of places use `generics::Color`.

::: servo/components/style/values/generics/color.rs:9
(Diff revision 3)
> +/// A trait to obtain the value of the transparent color for a color representation.
> +pub trait Transparent {
> +    /// Obtain the transparent color.
> +    fn transparent() -> Self;
> +}

I don't think this is necessary.

It seems that it is only used in the `transparent` function below, not anything else.

I think you can just implement the `transparent` functions for the corresponding concrete color types, rather than adding a trait and having it in the generic type.

::: servo/components/style/values/generics/color.rs:32
(Diff revision 3)
> +    pub const NUMERIC: ComplexColorRatios = ComplexColorRatios { bg: 1., fg: 0. };
> +    /// Ratios representing the `Foreground` color.
> +    pub const FOREGROUND: ComplexColorRatios = ComplexColorRatios { bg: 0., fg: 1. };
> +}
> +
> +pub use self::Color::*;

I don't think we general do this at top mod level.

Please avoid it.

::: servo/components/style/values/generics/color.rs:110
(Diff revision 3)
> +impl<T> ToAnimatedValue for Color<T>
> +where
> +    T: ToAnimatedValue,

I don't see why you need to implement this manually. This seems pretty deriveable.

If it complains about `ComplexColorRatios`, I think you can derive `ToAnimatedValue` for that as well.
Attachment #8983306 - Flags: review?(xidorn+moz)
Comment on attachment 8983307 [details]
Bug 1465307 - P4: Enable disabled dom/smil lighting_color tests.

https://reviewboard.mozilla.org/r/249210/#review255678
Attachment #8983307 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8983304 [details]
Bug 1465307 - P1: Extend StyleComplexColor to support additive blending.

https://reviewboard.mozilla.org/r/249204/#review255664

> I think you can add `StyleComplexColor_Tag` into `rusty-enums` in `ServoBindings.toml`, then you can just do
> ```rust
> use gecko_bindings::structs::StyleComplexColor_Tag as Tag;
> ```
> here, and code below can be much simplified.
> 
> Maybe this would also avoid the warning you are working around below.

Thanks. I didn't about that feature in ServoBindings.toml.
Comment on attachment 8983304 [details]
Bug 1465307 - P1: Extend StyleComplexColor to support additive blending.

https://reviewboard.mozilla.org/r/249204/#review255664

> `matches!(*self, Color::Numeric { .. })`

Thanks. I didn't know about that macro.
Comment on attachment 8983306 [details]
Bug 1465307 - P3: Extract {animated,computed}::Color common parts.

https://reviewboard.mozilla.org/r/249208/#review255676

> I don't think this is necessary.
> 
> It seems that it is only used in the `transparent` function below, not anything else.
> 
> I think you can just implement the `transparent` functions for the corresponding concrete color types, rather than adding a trait and having it in the generic type.

I added that when I started making RGBA generic and then forgot to remove when I abandoned that.

> I don't think we general do this at top mod level.
> 
> Please avoid it.

I was trying to get around the weird behaviour of rust where the pattern matching refers to the generic type name:
```rust
impl From<ComputedColor> for StyleComplexColor {
    fn from(other: ComputedColor) -> Self {
        match other {
            // Matching a ComputedColor but have to specify GenericColor
            GenericColor::Numeric(color) => color.into(),
            GenericColor::Foreground => Self::current_color(),
            GenericColor::Complex(color, ratios) => {
                ...
            }
        }
    }
}```

If I import the generic names using `use values::generics::color as generics`, as you suggest, it would become:
```rust
impl From<ComputedColor> for StyleComplexColor {
    fn from(other: ComputedColor) -> Self {
        match other {
            // Matching a ComputedColor but have to specify generics::Color
            generics::Color::Numeric(color) => color.into(),
            generics::Color::Foreground => Self::current_color(),
            generics::Color::Complex(color, ratios) => {
                ...
            }
        }
    }
}```

which is gross.

Would you be okay with:
```rust
impl From<ComputedColor> for StyleComplexColor {
    fn from(other: ComputedColor) -> Self {
        use GenericColor::*;
        match other {
            Numeric(color) => color.into(),
            Foreground => Self::current_color(),
            Complex(color, ratios) => {
                ...
            }
        }
    }
}```
Comment on attachment 8983306 [details]
Bug 1465307 - P3: Extract {animated,computed}::Color common parts.

https://reviewboard.mozilla.org/r/249208/#review255676

> I was trying to get around the weird behaviour of rust where the pattern matching refers to the generic type name:
> ```rust
> impl From<ComputedColor> for StyleComplexColor {
>     fn from(other: ComputedColor) -> Self {
>         match other {
>             // Matching a ComputedColor but have to specify GenericColor
>             GenericColor::Numeric(color) => color.into(),
>             GenericColor::Foreground => Self::current_color(),
>             GenericColor::Complex(color, ratios) => {
>                 ...
>             }
>         }
>     }
> }```
> 
> If I import the generic names using `use values::generics::color as generics`, as you suggest, it would become:
> ```rust
> impl From<ComputedColor> for StyleComplexColor {
>     fn from(other: ComputedColor) -> Self {
>         match other {
>             // Matching a ComputedColor but have to specify generics::Color
>             generics::Color::Numeric(color) => color.into(),
>             generics::Color::Foreground => Self::current_color(),
>             generics::Color::Complex(color, ratios) => {
>                 ...
>             }
>         }
>     }
> }```
> 
> which is gross.
> 
> Would you be okay with:
> ```rust
> impl From<ComputedColor> for StyleComplexColor {
>     fn from(other: ComputedColor) -> Self {
>         use GenericColor::*;
>         match other {
>             Numeric(color) => color.into(),
>             Foreground => Self::current_color(),
>             Complex(color, ratios) => {
>                 ...
>             }
>         }
>     }
> }```

https://github.com/rust-lang/rust/issues/26264
Comment on attachment 8983306 [details]
Bug 1465307 - P3: Extract {animated,computed}::Color common parts.

https://reviewboard.mozilla.org/r/249208/#review255676

> You can do
> ```rust
> use values::generics::color as generics;
> ```
> then in the rest of places use `generics::Color`.

Decided not to do this as you'll see in the adjustments due to other comments.
Comment on attachment 8983306 [details]
Bug 1465307 - P3: Extract {animated,computed}::Color common parts.

https://reviewboard.mozilla.org/r/249208/#review256030

::: servo/components/style/values/animated/color.rs:105
(Diff revision 4)
>  
>          Ok(ComplexColorRatios { bg, fg })
>      }
>  }
>  
>  #[allow(missing_docs)]

It'd better have a document here. It can just be something like
```
/// An animated value for `<color>`
```

::: servo/components/style/values/computed/color.rs:20
(Diff revision 4)
> -impl Color {
> -    /// Returns a numeric color representing the given RGBA value.
> +/// This enum represents a combined color from a numeric color and
> +/// the current foreground color (currentcolor keyword).

This is no longer an enum, just an alias now. Probably something like
```
/// A computed value for `<color>`
```
would be good enough.
Attachment #8983306 - Flags: review?(xidorn+moz) → review+
Pushed by dglastonbury@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76d098691ed0
P1: Extend StyleComplexColor to support additive blending. r=hiro,xidorn
https://hg.mozilla.org/integration/autoland/rev/31c82145a8e7
P2: Fix nsStyleBorder::mBorderColor for GCC. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/85784a996d8b
P3: Extract {animated,computed}::Color common parts. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/b34fa4440625
P4: Enable disabled dom/smil lighting_color tests. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/76d098691ed0
https://hg.mozilla.org/mozilla-central/rev/31c82145a8e7
https://hg.mozilla.org/mozilla-central/rev/85784a996d8b
https://hg.mozilla.org/mozilla-central/rev/b34fa4440625
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: