Bug 1845693 - Make AbsoluteColor deal with legacy and modern color syntax more concise r=emilio

Now the default of a color is the modern syntax and converting to legacy
syntax is very explicit.

AbsoluteColor::new can now take u8, f32 and Option<f32> as component
arguments and the flags will be set automagically.

Differential Revision: https://phabricator.services.mozilla.com/D184685

layout/painting/nsCSSRenderingGradients.cpp

@@ -408,7 +408,7 @@ static void ResolveMidpoints(nsTArray<ColorStop>& stops) {
       const float alpha =
           srgb1.alpha + multiplier * (srgb2.alpha - srgb1.alpha);
 
-      newStop.mColor = StyleAbsoluteColor::Srgb(red, green, blue, alpha);
+      newStop.mColor = StyleAbsoluteColor::SrgbLegacy(red, green, blue, alpha);
     }
 
     stops.ReplaceElementsAt(x, 1, newStops, 9);

layout/style/StyleColorInlines.h

@@ -16,16 +16,26 @@
 namespace mozilla {
 
 inline StyleAbsoluteColor StyleAbsoluteColor::FromColor(nscolor aColor) {
-  return StyleAbsoluteColor::Srgb(
+  return StyleAbsoluteColor::SrgbLegacy(
       NS_GET_R(aColor) / 255.0f, NS_GET_G(aColor) / 255.0f,
       NS_GET_B(aColor) / 255.0f, NS_GET_A(aColor) / 255.0f);
 }
 
 // static
-inline StyleAbsoluteColor StyleAbsoluteColor::Srgb(float red, float green,
-                                                   float blue, float alpha) {
-  return StyleAbsoluteColor{StyleColorComponents{red, green, blue}, alpha,
-                            StyleColorSpace::Srgb, StyleColorFlags{0}};
+inline StyleAbsoluteColor StyleAbsoluteColor::SrgbLegacy(float red, float green,
+                                                         float blue,
+                                                         float alpha) {
+  const auto ToLegacyComponent = [](float aF) {
+    if (MOZ_UNLIKELY(!std::isfinite(aF))) {
+      return 0.0f;
+    }
+    return aF;
+  };
+
+  return StyleAbsoluteColor{
+      StyleColorComponents{ToLegacyComponent(red), ToLegacyComponent(green),
+                           ToLegacyComponent(blue)},
+      alpha, StyleColorSpace::Srgb, StyleColorFlags::IS_LEGACY_SRGB};
 }
 
 template <>

servo/components/style/color/mix.rs

@@ -169,8 +169,19 @@ pub fn mix(
         alpha_multiplier,
     );
 
-    if flags.contains(ColorMixFlags::RESULT_IN_MODERN_SYNTAX) && result.is_legacy_syntax() {
-        result.to_color_space(ColorSpace::Srgb).into_modern_syntax()
+    if flags.contains(ColorMixFlags::RESULT_IN_MODERN_SYNTAX) {
+        // If the result *MUST* be in modern syntax, then make sure it is in a
+        // color space that allows the modern syntax. So hsl and hwb will be
+        // converted to srgb.
+        if result.is_legacy_syntax() {
+            result.to_color_space(ColorSpace::Srgb)
+        } else {
+            result
+        }
+    } else if left_color.is_legacy_syntax() && right_color.is_legacy_syntax() {
+        // If both sides of the mix is legacy then convert the result back into
+        // legacy.
+        result.into_srgb_legacy()
     } else {
         result
     }

servo/components/style/color/mod.rs

@@ -18,6 +18,7 @@ pub struct ColorComponents(pub f32, pub f32, pub f32);
 
 impl ColorComponents {
     /// Apply a function to each of the 3 components of the color.
+    #[must_use]
     pub fn map(self, f: impl Fn(f32) -> f32) -> Self {
         Self(f(self.0), f(self.1), f(self.2))
     }
@@ -131,9 +132,9 @@ bitflags! {
     #[derive(Clone, Copy, Default, MallocSizeOf, PartialEq, ToShmem)]
     #[repr(C)]
     pub struct ColorFlags : u8 {
-        /// If set, serializes sRGB colors into `color(srgb ...)` instead of
-        /// `rgba(...)`.
-        const AS_COLOR_FUNCTION = 1 << 0;
+        /// Marks that this color is in the legacy color format. This flag is
+        /// only valid for the `Srgb` color space.
+        const IS_LEGACY_SRGB = 1 << 0;
         /// Whether the 1st color component is `none`.
         const C1_IS_NONE = 1 << 1;
         /// Whether the 2nd color component is `none`.
@@ -230,7 +231,7 @@ impl AbsoluteColor {
         components: ColorComponents(0.0, 0.0, 0.0),
         alpha: 0.0,
         color_space: ColorSpace::Srgb,
-        flags: ColorFlags { bits: 0 }, // cbindgen does not like ColorFlags::empty().
+        flags: ColorFlags::IS_LEGACY_SRGB,
     };
 
     /// An opaque black color in the legacy syntax.
@@ -238,7 +239,7 @@ impl AbsoluteColor {
         components: ColorComponents(0.0, 0.0, 0.0),
         alpha: 1.0,
         color_space: ColorSpace::Srgb,
-        flags: ColorFlags { bits: 0 }, // cbindgen does not like ColorFlags::empty().
+        flags: ColorFlags::IS_LEGACY_SRGB,
     };
 
     /// An opaque white color in the legacy syntax.
@@ -246,7 +247,7 @@ impl AbsoluteColor {
         components: ColorComponents(1.0, 1.0, 1.0),
         alpha: 1.0,
         color_space: ColorSpace::Srgb,
-        flags: ColorFlags { bits: 0 }, // cbindgen does not like ColorFlags::empty().
+        flags: ColorFlags::IS_LEGACY_SRGB,
     };
 
     /// Create a new [`AbsoluteColor`] with the given [`ColorSpace`] and
@@ -299,16 +300,29 @@ impl AbsoluteColor {
         }
     }
 
-    /// Convert this color to the modern color syntax.
+    /// Convert this color into the sRGB color space and set it to the legacy
+    /// syntax.
     #[inline]
-    pub fn into_modern_syntax(mut self) -> Self {
-        self.flags |= ColorFlags::AS_COLOR_FUNCTION;
-        self
+    #[must_use]
+    pub fn into_srgb_legacy(self) -> Self {
+        let mut result = if !matches!(self.color_space, ColorSpace::Srgb) {
+            self.to_color_space(ColorSpace::Srgb)
+        } else {
+            self
+        };
+
+        // Explicitly set the flags to IS_LEGACY_SRGB only to clear out the
+        // *_IS_NONE flags, because the legacy syntax doesn't allow "none".
+        result.flags = ColorFlags::IS_LEGACY_SRGB;
+
+        result
     }
 
-    /// Create a new [`AbsoluteColor`] from rgba values in the sRGB color space.
-    pub fn srgb(red: f32, green: f32, blue: f32, alpha: f32) -> Self {
-        Self::new(ColorSpace::Srgb, red, green, blue, alpha)
+    /// Create a new [`AbsoluteColor`] from rgba legacy syntax values in the sRGB color space.
+    pub fn srgb_legacy(red: u8, green: u8, blue: u8, alpha: f32) -> Self {
+        let mut result = Self::new(ColorSpace::Srgb, red, green, blue, alpha);
+        result.flags = ColorFlags::IS_LEGACY_SRGB;
+        result
     }
 
     /// Return all the components of the color in an array.  (Includes alpha)
@@ -322,7 +336,7 @@ impl AbsoluteColor {
     pub fn is_legacy_syntax(&self) -> bool {
         // rgb(), rgba(), hsl(), hsla(), hwb(), hwba()
         match self.color_space {
-            ColorSpace::Srgb => !self.flags.contains(ColorFlags::AS_COLOR_FUNCTION),
+            ColorSpace::Srgb => self.flags.contains(ColorFlags::IS_LEGACY_SRGB),
             ColorSpace::Hsl | ColorSpace::Hwb => true,
             _ => false,
         }
@@ -453,17 +467,14 @@ impl ToCss for AbsoluteColor {
         let maybe_alpha = value_or_none!(self.alpha, ALPHA_IS_NONE);
 
         match self.color_space {
-            ColorSpace::Hsl => self.to_color_space(ColorSpace::Srgb).to_css(dest),
-
-            ColorSpace::Hwb => self.to_color_space(ColorSpace::Srgb).to_css(dest),
-
-            ColorSpace::Srgb if !self.flags.contains(ColorFlags::AS_COLOR_FUNCTION) => {
+            ColorSpace::Srgb if self.flags.contains(ColorFlags::IS_LEGACY_SRGB) => {
                 // The "none" keyword is not supported in the rgb/rgba legacy syntax.
                 cssparser::ToCss::to_css(
                     &cssparser::RgbaLegacy::from_floats(self.components.0, self.components.1, self.components.2, self.alpha),
                     dest,
                 )
             },
+            ColorSpace::Hsl | ColorSpace::Hwb => self.into_srgb_legacy().to_css(dest),
             ColorSpace::Lab => cssparser::ToCss::to_css(
                 &cssparser::Lab::new(maybe_c1, maybe_c2, maybe_c3, maybe_alpha),
                 dest,
@@ -484,10 +495,9 @@ impl ToCss for AbsoluteColor {
                 let color_space = match self.color_space {
                     ColorSpace::Srgb => {
                         debug_assert!(
-                            self.flags.contains(ColorFlags::AS_COLOR_FUNCTION),
-                             "The case without this flag should be handled in the wrapping match case!!"
-                          );
-
+                            !self.flags.contains(ColorFlags::IS_LEGACY_SRGB),
+                            "legacy srgb is not a color function"
+                        );
                         cssparser::PredefinedColorSpace::Srgb
                     },
                     ColorSpace::SrgbLinear => cssparser::PredefinedColorSpace::SrgbLinear,

servo/components/style/gecko/values.rs

@@ -28,12 +28,17 @@ pub fn convert_absolute_color_to_nscolor(color: &AbsoluteColor) -> u32 {
 /// Convert a given `nscolor` to a Servo AbsoluteColor value.
 pub fn convert_nscolor_to_absolute_color(color: u32) -> AbsoluteColor {
     let [r, g, b, a] = color.to_le_bytes();
-    AbsoluteColor::srgb(
-        r as f32 / 255.0,
-        g as f32 / 255.0,
-        b as f32 / 255.0,
-        a as f32 / 255.0,
-    )
+    AbsoluteColor::srgb_legacy(r, g, b, a as f32 / 255.0)
+}
+
+#[test]
+fn convert_ns_color_to_absolute_color_should_be_in_legacy_syntax() {
+    use crate::color::ColorFlags;
+
+    let result = convert_nscolor_to_absolute_color(0x336699CC);
+    assert!(result.flags.contains(ColorFlags::IS_LEGACY_SRGB));
+
+    assert!(result.is_legacy_syntax());
 }
 
 impl CounterStyle {

servo/components/style/values/specified/color.rs

@@ -149,11 +149,7 @@ impl LightDark {
     fn compute(&self, cx: &Context) -> ComputedColor {
         let style_color_scheme = cx.style().get_inherited_ui().clone_color_scheme();
         let dark = cx.device().is_dark_color_scheme(&style_color_scheme);
-        let used = if dark {
-            &self.dark
-        } else {
-            &self.light
-        };
+        let used = if dark { &self.dark } else { &self.light };
         used.to_computed_value(cx)
     }
 
@@ -428,30 +424,13 @@ impl SystemColor {
     }
 }
 
-#[inline]
-fn new_absolute(
-    color_space: ColorSpace,
-    c1: Option<f32>,
-    c2: Option<f32>,
-    c3: Option<f32>,
-    alpha: Option<f32>,
-) -> Color {
-    Color::Absolute(Box::new(Absolute {
-        color: AbsoluteColor::new(color_space, c1, c2, c3, alpha),
-        authored: None,
-    }))
-}
-
 impl cssparser::FromParsedColor for Color {
     fn from_current_color() -> Self {
         Color::CurrentColor
     }
 
     fn from_rgba(r: u8, g: u8, b: u8, a: f32) -> Self {
-        Self::Absolute(Box::new(Absolute {
-            color: AbsoluteColor::new(ColorSpace::Srgb, r, g, b, a),
-            authored: None,
-        }))
+        AbsoluteColor::srgb_legacy(r, g, b, a).into()
     }
 
     fn from_hsl(
@@ -460,7 +439,7 @@ impl cssparser::FromParsedColor for Color {
         lightness: Option<f32>,
         alpha: Option<f32>,
     ) -> Self {
-        new_absolute(ColorSpace::Hsl, hue, saturation, lightness, alpha)
+        AbsoluteColor::new(ColorSpace::Hsl, hue, saturation, lightness, alpha).into()
     }
 
     fn from_hwb(
@@ -469,7 +448,7 @@ impl cssparser::FromParsedColor for Color {
         blackness: Option<f32>,
         alpha: Option<f32>,
     ) -> Self {
-        new_absolute(ColorSpace::Hwb, hue, whiteness, blackness, alpha)
+        AbsoluteColor::new(ColorSpace::Hwb, hue, whiteness, blackness, alpha).into()
     }
 
     fn from_lab(
@@ -478,7 +457,7 @@ impl cssparser::FromParsedColor for Color {
         b: Option<f32>,
         alpha: Option<f32>,
     ) -> Self {
-        new_absolute(ColorSpace::Lab, lightness, a, b, alpha)
+        AbsoluteColor::new(ColorSpace::Lab, lightness, a, b, alpha).into()
     }
 
     fn from_lch(
@@ -487,7 +466,7 @@ impl cssparser::FromParsedColor for Color {
         hue: Option<f32>,
         alpha: Option<f32>,
     ) -> Self {
-        new_absolute(ColorSpace::Lch, lightness, chroma, hue, alpha)
+        AbsoluteColor::new(ColorSpace::Lch, lightness, chroma, hue, alpha).into()
     }
 
     fn from_oklab(
@@ -496,7 +475,7 @@ impl cssparser::FromParsedColor for Color {
         b: Option<f32>,
         alpha: Option<f32>,
     ) -> Self {
-        new_absolute(ColorSpace::Oklab, lightness, a, b, alpha)
+        AbsoluteColor::new(ColorSpace::Oklab, lightness, a, b, alpha).into()
     }
 
     fn from_oklch(
@@ -505,7 +484,7 @@ impl cssparser::FromParsedColor for Color {
         hue: Option<f32>,
         alpha: Option<f32>,
     ) -> Self {
-        new_absolute(ColorSpace::Oklch, lightness, chroma, hue, alpha)
+        AbsoluteColor::new(ColorSpace::Oklch, lightness, chroma, hue, alpha).into()
     }
 
     fn from_color_function(
@@ -515,13 +494,7 @@ impl cssparser::FromParsedColor for Color {
         c3: Option<f32>,
         alpha: Option<f32>,
     ) -> Self {
-        let mut result = new_absolute(color_space.into(), c1, c2, c3, alpha);
-        if let Color::Absolute(ref mut absolute) = result {
-            if matches!(absolute.color.color_space, ColorSpace::Srgb) {
-                absolute.color.flags |= ColorFlags::AS_COLOR_FUNCTION;
-            }
-        }
-        result
+        AbsoluteColor::new(color_space.into(), c1, c2, c3, alpha).into()
     }
 }
 
@@ -636,7 +609,7 @@ impl Color {
                             ColorSpace::Srgb | ColorSpace::Hsl
                         );
                         let is_color_function =
-                            absolute.color.flags.contains(ColorFlags::AS_COLOR_FUNCTION);
+                            !absolute.color.flags.contains(ColorFlags::IS_LEGACY_SRGB);
                         let pref_enabled = static_prefs::pref!("layout.css.more_color_4.enabled");
 
                         (is_legacy_color && !is_color_function) || pref_enabled

servo/ports/geckolib/cbindgen.toml

@@ -581,9 +581,9 @@ renaming_overrides_prefixing = true
 
 "AbsoluteColor" = """
   /**
-   * Create a new AbsoluteColor in the sRGB color space.
+   * Create a new AbsoluteColor in the sRGB color space in legacy color syntax.
    */
-  static inline StyleAbsoluteColor Srgb(float red, float green, float blue, float alpha);
+  static inline StyleAbsoluteColor SrgbLegacy(float red, float green, float blue, float alpha);
 
   static inline StyleAbsoluteColor FromColor(nscolor);