Bug 1810717 - De-dup items with the same tag in the computed value of font-feature-settings and font-variation-settings. r=emilio

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

servo/components/style/values/animated/font.rs

@@ -6,27 +6,42 @@
 
 use super::{Animate, Procedure, ToAnimatedZero};
 use crate::values::computed::font::FontVariationSettings;
-use crate::values::computed::Number;
 use crate::values::distance::{ComputeSquaredDistance, SquaredDistance};
-use crate::values::generics::font::{FontSettings as GenericFontSettings, FontTag, VariationValue};
+use crate::values::generics::font::FontSettings as GenericFontSettings;
 
 /// <https://drafts.csswg.org/css-fonts-4/#font-variation-settings-def>
+///
+/// Note that the ComputedValue implementation will already have sorted and de-dup'd
+/// the lists of settings, so we can just iterate over the two lists together and
+/// animate their individual values.
 impl Animate for FontVariationSettings {
     #[inline]
     fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
-        FontSettingTagIter::new(self, other)?
-            .map(|r| r.and_then(|(st, ot)| st.animate(&ot, procedure)))
-            .collect::<Result<Vec<ComputedVariationValue>, ()>>()
-            .map(|v| GenericFontSettings(v.into_boxed_slice()))
+        if self.0.len() == other.0.len() {
+            self.0
+                .iter()
+                .zip(other.0.iter())
+                .map(|(st, ot)| st.animate(&ot, procedure))
+                .collect::<Result<Vec<_>, ()>>()
+                .map(|v| GenericFontSettings(v.into_boxed_slice()))
+        } else {
+            Err(())
+        }
     }
 }
 
 impl ComputeSquaredDistance for FontVariationSettings {
     #[inline]
     fn compute_squared_distance(&self, other: &Self) -> Result<SquaredDistance, ()> {
-        FontSettingTagIter::new(self, other)?
-            .map(|r| r.and_then(|(st, ot)| st.compute_squared_distance(&ot)))
-            .sum()
+        if self.0.len() == other.0.len() {
+            self.0
+                .iter()
+                .zip(other.0.iter())
+                .map(|(st, ot)| st.compute_squared_distance(&ot))
+                .sum()
+        } else {
+            Err(())
+        }
     }
 }
 
@@ -36,116 +51,3 @@ impl ToAnimatedZero for FontVariationSettings {
         Err(())
     }
 }
-
-type ComputedVariationValue = VariationValue<Number>;
-
-// FIXME: Could do a rename, this is only used for font variations.
-struct FontSettingTagIterState<'a> {
-    tags: Vec<&'a ComputedVariationValue>,
-    index: usize,
-    prev_tag: FontTag,
-}
-
-impl<'a> FontSettingTagIterState<'a> {
-    fn new(tags: Vec<&'a ComputedVariationValue>) -> FontSettingTagIterState<'a> {
-        FontSettingTagIterState {
-            index: tags.len(),
-            tags,
-            prev_tag: FontTag(0),
-        }
-    }
-}
-
-/// Iterator for font-variation-settings tag lists
-///
-/// [CSS fonts level 4](https://drafts.csswg.org/css-fonts-4/#descdef-font-face-font-variation-settings)
-/// defines the animation of font-variation-settings as follows:
-///
-///   Two declarations of font-feature-settings[sic] can be animated between if
-///   they are "like".  "Like" declarations are ones where the same set of
-///   properties appear (in any order).  Because succesive[sic] duplicate
-///   properties are applied instead of prior duplicate properties, two
-///   declarations can be "like" even if they have differing number of
-///   properties. If two declarations are "like" then animation occurs pairwise
-///   between corresponding values in the declarations.
-///
-/// In other words if we have the following lists:
-///
-///   "wght" 1.4, "wdth" 5, "wght" 2
-///   "wdth" 8, "wght" 4, "wdth" 10
-///
-/// We should animate between:
-///
-///   "wdth" 5, "wght" 2
-///   "wght" 4, "wdth" 10
-///
-/// This iterator supports this by sorting the two lists, then iterating them in
-/// reverse, and skipping entries with repeated tag names. It will return
-/// Some(Err()) if it reaches the end of one list before the other, or if the
-/// tag names do not match.
-///
-/// For the above example, this iterator would return:
-///
-///   Some(Ok("wght" 2, "wght" 4))
-///   Some(Ok("wdth" 5, "wdth" 10))
-///   None
-///
-struct FontSettingTagIter<'a> {
-    a_state: FontSettingTagIterState<'a>,
-    b_state: FontSettingTagIterState<'a>,
-}
-
-impl<'a> FontSettingTagIter<'a> {
-    fn new(
-        a_settings: &'a FontVariationSettings,
-        b_settings: &'a FontVariationSettings,
-    ) -> Result<FontSettingTagIter<'a>, ()> {
-        if a_settings.0.is_empty() || b_settings.0.is_empty() {
-            return Err(());
-        }
-
-        fn as_new_sorted_tags(tags: &[ComputedVariationValue]) -> Vec<&ComputedVariationValue> {
-            use std::iter::FromIterator;
-            let mut sorted_tags = Vec::from_iter(tags.iter());
-            sorted_tags.sort_by_key(|k| k.tag.0);
-            sorted_tags
-        }
-
-        Ok(FontSettingTagIter {
-            a_state: FontSettingTagIterState::new(as_new_sorted_tags(&a_settings.0)),
-            b_state: FontSettingTagIterState::new(as_new_sorted_tags(&b_settings.0)),
-        })
-    }
-
-    fn next_tag(state: &mut FontSettingTagIterState<'a>) -> Option<&'a ComputedVariationValue> {
-        if state.index == 0 {
-            return None;
-        }
-
-        state.index -= 1;
-        let tag = state.tags[state.index];
-        if tag.tag == state.prev_tag {
-            FontSettingTagIter::next_tag(state)
-        } else {
-            state.prev_tag = tag.tag;
-            Some(tag)
-        }
-    }
-}
-
-impl<'a> Iterator for FontSettingTagIter<'a> {
-    type Item = Result<(&'a ComputedVariationValue, &'a ComputedVariationValue), ()>;
-
-    fn next(
-        &mut self,
-    ) -> Option<Result<(&'a ComputedVariationValue, &'a ComputedVariationValue), ()>> {
-        match (
-            FontSettingTagIter::next_tag(&mut self.a_state),
-            FontSettingTagIter::next_tag(&mut self.b_state),
-        ) {
-            (Some(at), Some(bt)) if at.tag == bt.tag => Some(Ok((at, bt))),
-            (None, None) => None,
-            _ => Some(Err(())), // Mismatch number of unique tags or tag names.
-        }
-    }
-}

servo/components/style/values/computed/font.rs

@@ -10,7 +10,9 @@ use crate::values::computed::{
     Angle, Context, Integer, Length, NonNegativeLength, NonNegativeNumber, Number, Percentage,
     ToComputedValue,
 };
-use crate::values::generics::font::{FeatureTagValue, FontSettings, VariationValue};
+use crate::values::generics::font::{
+    FeatureTagValue, FontSettings, TaggedFontValue, VariationValue,
+};
 use crate::values::generics::{font as generics, NonNegative};
 use crate::values::specified::font::{
     self as specified, KeywordInfo, MAX_FONT_WEIGHT, MIN_FONT_WEIGHT,
@@ -26,8 +28,12 @@ use style_traits::{CssWriter, ParseError, ToCss};
 pub use crate::values::computed::Length as MozScriptMinSize;
 pub use crate::values::specified::font::FontPalette;
 pub use crate::values::specified::font::{FontSynthesis, MozScriptSizeMultiplier};
-pub use crate::values::specified::font::{FontVariantAlternates, FontVariantEastAsian, FontVariantLigatures, FontVariantNumeric, XLang, XTextZoom};
+pub use crate::values::specified::font::{
+    FontVariantAlternates, FontVariantEastAsian, FontVariantLigatures, FontVariantNumeric, XLang,
+    XTextZoom,
+};
 pub use crate::values::specified::Integer as SpecifiedInteger;
+pub use crate::values::specified::Number as SpecifiedNumber;
 
 /// Generic template for font property type classes that use a fixed-point
 /// internal representation with `FRACTION_BITS` for the fractional part.
@@ -718,6 +724,56 @@ pub type FontFeatureSettings = FontSettings<FeatureTagValue<Integer>>;
 /// The computed value for font-variation-settings.
 pub type FontVariationSettings = FontSettings<VariationValue<Number>>;
 
+// The computed value of font-{feature,variation}-settings discards values
+// with duplicate tags, keeping only the last occurrence of each tag.
+fn dedup_font_settings<T>(settings_list: &mut Vec<T>)
+where
+    T: TaggedFontValue,
+{
+    if settings_list.len() > 1 {
+        settings_list.sort_by_key(|k| k.tag().0);
+        // dedup() keeps the first of any duplicates, but we want the last,
+        // so we implement it manually here.
+        let mut prev_tag = settings_list.last().unwrap().tag();
+        for i in (0..settings_list.len() - 1).rev() {
+            let cur_tag = settings_list[i].tag();
+            if cur_tag == prev_tag {
+                settings_list.remove(i);
+            }
+            prev_tag = cur_tag;
+        }
+    }
+}
+
+impl<T> ToComputedValue for FontSettings<T>
+where
+    T: ToComputedValue,
+    <T as ToComputedValue>::ComputedValue: TaggedFontValue,
+{
+    type ComputedValue = FontSettings<T::ComputedValue>;
+
+    fn to_computed_value(&self, context: &Context) -> Self::ComputedValue {
+        let mut v = self
+            .0
+            .iter()
+            .map(|item| item.to_computed_value(context))
+            .collect::<Vec<_>>();
+        dedup_font_settings(&mut v);
+        FontSettings(v.into_boxed_slice())
+    }
+
+    fn from_computed_value(computed: &Self::ComputedValue) -> Self {
+        Self(
+            computed
+                .0
+                .iter()
+                .map(T::from_computed_value)
+                .collect::<Vec<_>>()
+                .into_boxed_slice(),
+        )
+    }
+}
+
 /// font-language-override can only have a single three-letter
 /// OpenType "language system" tag, so we should be able to compute
 /// it and store it as a 32-bit integer
@@ -963,7 +1019,7 @@ impl ToAnimatedValue for FontStyle {
             // This allows us to animate between normal and oblique values. Per spec,
             // https://drafts.csswg.org/css-fonts-4/#font-style-prop:
             //   Animation type: by computed value type; 'normal' animates as 'oblique 0deg'
-            return generics::FontStyle::Oblique(Angle::from_degrees(0.0))
+            return generics::FontStyle::Oblique(Angle::from_degrees(0.0));
         }
         if self == Self::ITALIC {
             return generics::FontStyle::Italic;
@@ -976,13 +1032,14 @@ impl ToAnimatedValue for FontStyle {
         match animated {
             generics::FontStyle::Normal => Self::NORMAL,
             generics::FontStyle::Italic => Self::ITALIC,
-            generics::FontStyle::Oblique(ref angle) =>
+            generics::FontStyle::Oblique(ref angle) => {
                 if angle.degrees() == 0.0 {
                     // Reverse the conversion done in to_animated_value()
                     Self::NORMAL
                 } else {
                     Self::oblique(angle.degrees())
-                },
+                }
+            },
         }
     }
 }

servo/components/style/values/generics/font.rs

@@ -13,6 +13,13 @@ use std::io::Cursor;
 use style_traits::{CssWriter, ParseError};
 use style_traits::{StyleParseErrorKind, ToCss};
 
+/// A trait for values that are labelled with a FontTag (for feature and
+/// variation settings).
+pub trait TaggedFontValue {
+    /// The value's tag.
+    fn tag(&self) -> FontTag;
+}
+
 /// https://drafts.csswg.org/css-fonts-4/#feature-tag-value
 #[derive(
     Clone,
@@ -32,6 +39,12 @@ pub struct FeatureTagValue<Integer> {
     pub value: Integer,
 }
 
+impl<T> TaggedFontValue for FeatureTagValue<T> {
+    fn tag(&self) -> FontTag {
+        self.tag
+    }
+}
+
 impl<Integer> ToCss for FeatureTagValue<Integer>
 where
     Integer: One + ToCss + PartialEq,
@@ -76,18 +89,15 @@ pub struct VariationValue<Number> {
     pub value: Number,
 }
 
+impl<T> TaggedFontValue for VariationValue<T> {
+    fn tag(&self) -> FontTag {
+        self.tag
+    }
+}
+
 /// A value both for font-variation-settings and font-feature-settings.
 #[derive(
-    Clone,
-    Debug,
-    Eq,
-    MallocSizeOf,
-    PartialEq,
-    SpecifiedValueInfo,
-    ToComputedValue,
-    ToCss,
-    ToResolvedValue,
-    ToShmem,
+    Clone, Debug, Eq, MallocSizeOf, PartialEq, SpecifiedValueInfo, ToCss, ToResolvedValue, ToShmem,
 )]
 #[css(comma)]
 pub struct FontSettings<T>(#[css(if_empty = "normal", iterable)] pub Box<[T]>);