From bf07f65e931d45cdb56d8615f57ab4765b56d210 Mon Sep 17 00:00:00 2001 From: Jonathan Kew Date: Thu, 19 Jan 2023 16:17:29 +0000 Subject: [PATCH] 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 --- .../components/style/values/animated/font.rs | 146 +++--------------- .../components/style/values/computed/font.rs | 67 +++++++- .../components/style/values/generics/font.rs | 30 ++-- 3 files changed, 106 insertions(+), 137 deletions(-) diff --git a/servo/components/style/values/animated/font.rs b/servo/components/style/values/animated/font.rs index f890a3b2bd3d..63a0abf17943 100644 --- a/servo/components/style/values/animated/font.rs +++ b/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; /// +/// +/// 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 { - FontSettingTagIter::new(self, other)? - .map(|r| r.and_then(|(st, ot)| st.animate(&ot, procedure))) - .collect::, ()>>() - .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::, ()>>() + .map(|v| GenericFontSettings(v.into_boxed_slice())) + } else { + Err(()) + } } } impl ComputeSquaredDistance for FontVariationSettings { #[inline] fn compute_squared_distance(&self, other: &Self) -> Result { - 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; - -// 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, ()> { - 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> { - 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. - } - } -} diff --git a/servo/components/style/values/computed/font.rs b/servo/components/style/values/computed/font.rs index c792cd97cfb5..abc36e61373c 100644 --- a/servo/components/style/values/computed/font.rs +++ b/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>; /// The computed value for font-variation-settings. pub type FontVariationSettings = FontSettings>; +// 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(settings_list: &mut Vec) +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 ToComputedValue for FontSettings +where + T: ToComputedValue, + ::ComputedValue: TaggedFontValue, +{ + type ComputedValue = FontSettings; + + fn to_computed_value(&self, context: &Context) -> Self::ComputedValue { + let mut v = self + .0 + .iter() + .map(|item| item.to_computed_value(context)) + .collect::>(); + 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::>() + .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()) - }, + } + }, } } } diff --git a/servo/components/style/values/generics/font.rs b/servo/components/style/values/generics/font.rs index a88df7be1732..09ed542e97a0 100644 --- a/servo/components/style/values/generics/font.rs +++ b/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 { pub value: Integer, } +impl TaggedFontValue for FeatureTagValue { + fn tag(&self) -> FontTag { + self.tag + } +} + impl ToCss for FeatureTagValue where Integer: One + ToCss + PartialEq, @@ -76,18 +89,15 @@ pub struct VariationValue { pub value: Number, } +impl TaggedFontValue for VariationValue { + 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(#[css(if_empty = "normal", iterable)] pub Box<[T]>);