Bug 1572738 - Don't clamp font-size calc() factors too early. r=manishearth

These two bugs (bug 1572738 and bug 1572451) are stylo regressions.

When font-family changes, we try to recompute the font-size with a length /
percentage combinations in case the generic family changes, so the user
preferences are kept.

When calc() is involved, we clamp to non-negative too early, via
NonNegativeLength::scale_by.

I think we should generally dump this "try to track font-size across calc()"
thingie, as as various comments note it is not quite perfect, and it's not clear
how it should work in presence of min()/max().

This patch fixes the issue and simplifies code a bit, I may consider removing
this altogether in a follow-up.

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

--HG--
extra : moz-landing-system : lando

servo/components/style/properties/gecko.mako.rs

@@ -1146,7 +1146,7 @@ fn static_assert() {
     }
 
     pub fn set_font_size(&mut self, v: FontSize) {
-        use crate::values::generics::font::KeywordSize;
+        use crate::values::specified::font::KeywordSize;
 
         let size = v.size();
         self.gecko.mScriptUnconstrainedSize = size.0;
@@ -1167,7 +1167,7 @@ fn static_assert() {
                 KeywordSize::XXXLarge => structs::NS_STYLE_FONT_SIZE_XXXLARGE,
             } as u8;
             self.gecko.mFontSizeFactor = info.factor;
-            self.gecko.mFontSizeOffset = info.offset.0.to_i32_au();
+            self.gecko.mFontSizeOffset = info.offset.to_i32_au();
         } else {
             self.gecko.mFontSizeKeyword = structs::NS_STYLE_FONT_SIZE_NO_KEYWORD as u8;
             self.gecko.mFontSizeFactor = 1.;
@@ -1176,7 +1176,7 @@ fn static_assert() {
     }
 
     pub fn clone_font_size(&self) -> FontSize {
-        use crate::values::generics::font::{KeywordInfo, KeywordSize};
+        use crate::values::specified::font::{KeywordInfo, KeywordSize};
         let size = Au(self.gecko.mSize).into();
         let kw = match self.gecko.mFontSizeKeyword as u32 {
             structs::NS_STYLE_FONT_SIZE_XXSMALL => KeywordSize::XXSmall,

servo/components/style/properties/longhands/font.mako.rs

@@ -401,7 +401,7 @@ ${helpers.predefined_type(
                         is_system_font: true,
                     },
                     font_size: FontSize {
-                        size: cx.maybe_zoom_text(Au(system.size).into()),
+                        size: NonNegative(cx.maybe_zoom_text(Au(system.size).into())),
                         keyword_info: None
                     },
                     font_weight,

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

@@ -9,11 +9,11 @@ use crate::gecko_bindings::sugar::refptr::RefPtr;
 #[cfg(feature = "gecko")]
 use crate::gecko_bindings::{bindings, structs};
 use crate::values::animated::{ToAnimatedValue, ToAnimatedZero};
-use crate::values::computed::{Angle, Context, Integer, NonNegativeLength, NonNegativePercentage};
+use crate::values::computed::{Angle, Context, Integer, Length, NonNegativeLength, NonNegativePercentage};
 use crate::values::computed::{Number, Percentage, ToComputedValue};
-use crate::values::generics::font as generics;
+use crate::values::generics::{font as generics, NonNegative};
 use crate::values::generics::font::{FeatureTagValue, FontSettings, VariationValue};
-use crate::values::specified::font::{self as specified, MAX_FONT_WEIGHT, MIN_FONT_WEIGHT};
+use crate::values::specified::font::{self as specified, KeywordInfo, MAX_FONT_WEIGHT, MIN_FONT_WEIGHT};
 use crate::values::specified::length::{FontBaseSize, NoCalcLength};
 use crate::values::CSSFloat;
 use crate::Atom;
@@ -88,9 +88,6 @@ pub struct FontSize {
     pub keyword_info: Option<KeywordInfo>,
 }
 
-/// Additional information for computed keyword-derived font sizes.
-pub type KeywordInfo = generics::KeywordInfo<NonNegativeLength>;
-
 impl FontWeight {
     /// Value for normal
     pub fn normal() -> Self {
@@ -162,17 +159,17 @@ impl FontSize {
 }
 
 impl ToAnimatedValue for FontSize {
-    type AnimatedValue = NonNegativeLength;
+    type AnimatedValue = Length;
 
     #[inline]
     fn to_animated_value(self) -> Self::AnimatedValue {
-        self.size
+        self.size.0
     }
 
     #[inline]
     fn from_animated_value(animated: Self::AnimatedValue) -> Self {
         FontSize {
-            size: animated.clamp(),
+            size: NonNegative(animated.clamp_to_non_negative()),
             keyword_info: None,
         }
     }

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

@@ -20,7 +20,7 @@ use crate::Zero;
 use app_units::Au;
 use ordered_float::NotNan;
 use std::fmt::{self, Write};
-use std::ops::{Add, Neg};
+use std::ops::{Add, Mul, Neg};
 use style_traits::values::specified::AllowedNumericType;
 use style_traits::{CssWriter, ToCss};
 
@@ -291,7 +291,7 @@ impl specified::CalcLengthPercentage {
     ) -> LengthPercentage {
         self.to_computed_value_with_zoom(
             context,
-            |abs| context.maybe_zoom_text(abs.into()).0,
+            |abs| context.maybe_zoom_text(abs.into()),
             base_size,
         )
     }
@@ -354,8 +354,8 @@ impl LengthPercentage {
         self.unclamped_length().px() == 0.0 && self.percentage.0 == 0.0
     }
 
-    // CSSFloat doesn't implement Hash, so does CSSPixelLength. Therefore, we still use Au as the
-    // hash key.
+    // CSSFloat doesn't implement Hash, so does CSSPixelLength. Therefore, we
+    // still use Au as the hash key.
     #[allow(missing_docs)]
     pub fn to_hash_key(&self) -> (Au, NotNan<f32>) {
         (
@@ -685,6 +685,15 @@ impl Neg for CSSPixelLength {
     }
 }
 
+impl Mul<CSSFloat> for CSSPixelLength {
+    type Output = Self;
+
+    #[inline]
+    fn mul(self, other: CSSFloat) -> Self {
+        Self::new(self.px() * other)
+    }
+}
+
 impl From<CSSPixelLength> for Au {
     #[inline]
     fn from(len: CSSPixelLength) -> Self {
@@ -750,14 +759,6 @@ impl NonNegativeLength {
             self
         }
     }
-
-    /// Scale this NonNegativeLength.
-    /// We scale NonNegativeLength by zero if the factor is negative because it doesn't
-    /// make sense to scale a negative factor on a non-negative length.
-    #[inline]
-    pub fn scale_by(&self, factor: f32) -> Self {
-        Self::new(self.0.px() * factor.max(0.))
-    }
 }
 
 impl From<Length> for NonNegativeLength {

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

@@ -230,7 +230,7 @@ impl<'a> Context<'a> {
 
     /// Apply text-zoom if enabled.
     #[cfg(feature = "gecko")]
-    pub fn maybe_zoom_text(&self, size: NonNegativeLength) -> NonNegativeLength {
+    pub fn maybe_zoom_text(&self, size: CSSPixelLength) -> CSSPixelLength {
         // We disable zoom for <svg:text> by unsetting the
         // -x-text-zoom property, which leads to a false value
         // in mAllowZoom
@@ -243,7 +243,7 @@ impl<'a> Context<'a> {
 
     /// (Servo doesn't do text-zoom)
     #[cfg(feature = "servo")]
-    pub fn maybe_zoom_text(&self, size: NonNegativeLength) -> NonNegativeLength {
+    pub fn maybe_zoom_text(&self, size: CSSPixelLength) -> CSSPixelLength {
         size
     }
 }

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

@@ -5,14 +5,13 @@
 //! Generic types for font stuff.
 
 use crate::parser::{Parse, ParserContext};
-use app_units::Au;
 use byteorder::{BigEndian, ReadBytesExt};
 use cssparser::Parser;
 use num_traits::One;
 use std::fmt::{self, Write};
 use std::io::Cursor;
-use style_traits::{CssWriter, KeywordsCollectFn, ParseError};
-use style_traits::{SpecifiedValueInfo, StyleParseErrorKind, ToCss};
+use style_traits::{CssWriter, ParseError};
+use style_traits::{StyleParseErrorKind, ToCss};
 
 /// https://drafts.csswg.org/css-fonts-4/#feature-tag-value
 #[derive(
@@ -172,104 +171,6 @@ impl Parse for FontTag {
     }
 }
 
-#[derive(
-    Animate,
-    Clone,
-    ComputeSquaredDistance,
-    Copy,
-    Debug,
-    MallocSizeOf,
-    PartialEq,
-    ToAnimatedValue,
-    ToAnimatedZero,
-    ToCss,
-    ToShmem,
-)]
-/// Additional information for keyword-derived font sizes.
-pub struct KeywordInfo<Length> {
-    /// The keyword used
-    pub kw: KeywordSize,
-    /// A factor to be multiplied by the computed size of the keyword
-    #[css(skip)]
-    pub factor: f32,
-    /// An additional Au offset to add to the kw*factor in the case of calcs
-    #[css(skip)]
-    pub offset: Length,
-}
-
-impl<L> KeywordInfo<L>
-where
-    Au: Into<L>,
-{
-    /// KeywordInfo value for font-size: medium
-    pub fn medium() -> Self {
-        KeywordSize::Medium.into()
-    }
-}
-
-impl<L> From<KeywordSize> for KeywordInfo<L>
-where
-    Au: Into<L>,
-{
-    fn from(x: KeywordSize) -> Self {
-        KeywordInfo {
-            kw: x,
-            factor: 1.,
-            offset: Au(0).into(),
-        }
-    }
-}
-
-impl<L> SpecifiedValueInfo for KeywordInfo<L> {
-    fn collect_completion_keywords(f: KeywordsCollectFn) {
-        <KeywordSize as SpecifiedValueInfo>::collect_completion_keywords(f);
-    }
-}
-
-/// CSS font keywords
-#[derive(
-    Animate,
-    Clone,
-    ComputeSquaredDistance,
-    Copy,
-    Debug,
-    MallocSizeOf,
-    Parse,
-    PartialEq,
-    SpecifiedValueInfo,
-    ToAnimatedValue,
-    ToAnimatedZero,
-    ToCss,
-    ToShmem,
-)]
-#[allow(missing_docs)]
-pub enum KeywordSize {
-    #[css(keyword = "xx-small")]
-    XXSmall,
-    XSmall,
-    Small,
-    Medium,
-    Large,
-    XLarge,
-    #[css(keyword = "xx-large")]
-    XXLarge,
-    #[css(keyword = "xxx-large")]
-    XXXLarge,
-}
-
-impl KeywordSize {
-    /// Convert to an HTML <font size> value
-    #[inline]
-    pub fn html_size(self) -> u8 {
-        self as u8
-    }
-}
-
-impl Default for KeywordSize {
-    fn default() -> Self {
-        KeywordSize::Medium
-    }
-}
 
 /// A generic value for the `font-style` property.
 ///

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

@@ -11,9 +11,9 @@ use crate::properties::longhands::system_font::SystemFont;
 use crate::values::computed::font::{FamilyName, FontFamilyList, FontStyleAngle, SingleFontFamily};
 use crate::values::computed::{font as computed, Length, NonNegativeLength};
 use crate::values::computed::{Angle as ComputedAngle, Percentage as ComputedPercentage};
-use crate::values::computed::{Context, ToComputedValue};
+use crate::values::computed::{CSSPixelLength, Context, ToComputedValue};
 use crate::values::generics::font::{self as generics, FeatureTagValue, FontSettings, FontTag};
-use crate::values::generics::font::{KeywordSize, VariationValue};
+use crate::values::generics::font::VariationValue;
 use crate::values::generics::NonNegative;
 use crate::values::specified::length::{FontBaseSize, AU_PER_PT, AU_PER_PX};
 use crate::values::specified::{AllowQuirks, Angle, Integer, LengthPercentage};
@@ -481,6 +481,115 @@ impl ToComputedValue for FontStretch {
     }
 }
 
+/// CSS font keywords
+#[derive(
+    Animate,
+    Clone,
+    ComputeSquaredDistance,
+    Copy,
+    Debug,
+    MallocSizeOf,
+    Parse,
+    PartialEq,
+    SpecifiedValueInfo,
+    ToAnimatedValue,
+    ToAnimatedZero,
+    ToCss,
+    ToShmem,
+)]
+#[allow(missing_docs)]
+pub enum KeywordSize {
+    #[css(keyword = "xx-small")]
+    XXSmall,
+    XSmall,
+    Small,
+    Medium,
+    Large,
+    XLarge,
+    #[css(keyword = "xx-large")]
+    XXLarge,
+    #[css(keyword = "xxx-large")]
+    XXXLarge,
+}
+
+impl KeywordSize {
+    /// Convert to an HTML <font size> value
+    #[inline]
+    pub fn html_size(self) -> u8 {
+        self as u8
+    }
+}
+
+impl Default for KeywordSize {
+    fn default() -> Self {
+        KeywordSize::Medium
+    }
+}
+
+#[derive(
+    Animate,
+    Clone,
+    ComputeSquaredDistance,
+    Copy,
+    Debug,
+    MallocSizeOf,
+    PartialEq,
+    ToAnimatedValue,
+    ToAnimatedZero,
+    ToCss,
+    ToShmem,
+)]
+/// Additional information for keyword-derived font sizes.
+pub struct KeywordInfo {
+    /// The keyword used
+    pub kw: KeywordSize,
+    /// A factor to be multiplied by the computed size of the keyword
+    #[css(skip)]
+    pub factor: f32,
+    /// An additional fixed offset to add to the kw * factor in the case of
+    /// `calc()`.
+    #[css(skip)]
+    pub offset: CSSPixelLength,
+}
+
+impl KeywordInfo {
+    /// KeywordInfo value for font-size: medium
+    pub fn medium() -> Self {
+        Self::new(KeywordSize::Medium)
+    }
+
+    fn new(kw: KeywordSize) -> Self {
+        KeywordInfo {
+            kw,
+            factor: 1.,
+            offset: CSSPixelLength::new(0.),
+        }
+    }
+
+    /// Computes the final size for this font-size keyword, accounting for
+    /// text-zoom.
+    fn to_computed_value(&self, context: &Context) -> CSSPixelLength {
+        let base = context.maybe_zoom_text(self.kw.to_computed_value(context).0);
+        base * self.factor + context.maybe_zoom_text(self.offset)
+    }
+
+    /// Given a parent keyword info (self), apply an additional factor/offset to
+    /// it.
+    pub fn compose(self, factor: f32, offset: CSSPixelLength) -> Self {
+        KeywordInfo {
+            kw: self.kw,
+            factor: self.factor * factor,
+            offset: self.offset * factor + offset,
+        }
+    }
+}
+
+impl SpecifiedValueInfo for KeywordInfo {
+    fn collect_completion_keywords(f: KeywordsCollectFn) {
+        <KeywordSize as SpecifiedValueInfo>::collect_completion_keywords(f);
+    }
+}
+
 #[derive(Clone, Debug, MallocSizeOf, PartialEq, SpecifiedValueInfo, ToCss, ToShmem)]
 /// A specified font-size value
 pub enum FontSize {
@@ -652,27 +761,6 @@ impl ToComputedValue for FontSizeAdjust {
     }
 }
 
-/// Additional information for specified keyword-derived font sizes.
-pub type KeywordInfo = generics::KeywordInfo<NonNegativeLength>;
-
-impl KeywordInfo {
-    /// Computes the final size for this font-size keyword, accounting for
-    /// text-zoom.
-    pub fn to_computed_value(&self, context: &Context) -> NonNegativeLength {
-        let base = context.maybe_zoom_text(self.kw.to_computed_value(context));
-        base.scale_by(self.factor) + context.maybe_zoom_text(self.offset)
-    }
-
-    /// Given a parent keyword info (self), apply an additional factor/offset to it
-    pub fn compose(self, factor: f32, offset: NonNegativeLength) -> Self {
-        KeywordInfo {
-            kw: self.kw,
-            factor: self.factor * factor,
-            offset: self.offset.scale_by(factor) + offset,
-        }
-    }
-}
-
 /// This is the ratio applied for font-size: larger
 /// and smaller by both Firefox and Chrome
 const LARGER_FONT_SIZE_RATIO: f32 = 1.2;
@@ -790,7 +878,7 @@ impl FontSize {
     /// <https://html.spec.whatwg.org/multipage/#rules-for-parsing-a-legacy-font-size>
     pub fn from_html_size(size: u8) -> Self {
         FontSize::Keyword(
-            match size {
+            KeywordInfo::new(match size {
                 // If value is less than 1, let it be 1.
                 0 | 1 => KeywordSize::XSmall,
                 2 => KeywordSize::Small,
@@ -800,8 +888,7 @@ impl FontSize {
                 6 => KeywordSize::XXLarge,
                 // If value is greater than 7, let it be 7.
                 _ => KeywordSize::XXXLarge,
-            }
-            .into(),
+            })
         )
     }
 
@@ -819,7 +906,7 @@ impl FontSize {
                 .get_parent_font()
                 .clone_font_size()
                 .keyword_info
-                .map(|i| i.compose(factor, Au(0).into()))
+                .map(|i| i.compose(factor, CSSPixelLength::new(0.)))
         };
         let mut info = None;
         let size = match *self {
@@ -829,16 +916,16 @@ impl FontSize {
                     // Tack the em unit onto the factor
                     info = compose_keyword(em);
                 }
-                value.to_computed_value(context, base_size).into()
+                value.to_computed_value(context, base_size)
             },
             FontSize::Length(LengthPercentage::Length(NoCalcLength::ServoCharacterWidth(
                 value,
-            ))) => value.to_computed_value(base_size.resolve(context)).into(),
+            ))) => value.to_computed_value(base_size.resolve(context)),
             FontSize::Length(LengthPercentage::Length(NoCalcLength::Absolute(ref l))) => {
-                context.maybe_zoom_text(l.to_computed_value(context).into())
+                context.maybe_zoom_text(l.to_computed_value(context))
             },
             FontSize::Length(LengthPercentage::Length(ref l)) => {
-                l.to_computed_value(context).into()
+                l.to_computed_value(context)
             },
             FontSize::Length(LengthPercentage::Percentage(pc)) => {
                 // If the parent font was keyword-derived, this is too.
@@ -871,29 +958,33 @@ impl FontSize {
                             context,
                             FontBaseSize::InheritedStyleButStripEmUnits,
                         )
-                        .length_component();
+                        .unclamped_length();
 
-                    info = parent.keyword_info.map(|i| i.compose(ratio, abs.into()));
+                    info = parent.keyword_info.map(|i| i.compose(ratio, abs));
                 }
                 let calc = calc.to_computed_value_zoomed(context, base_size);
-                calc.to_used_value(base_size.resolve(context)).into()
+                // FIXME(emilio): we _could_ use clamp_to_non_negative()
+                // everywhere, without affecting behavior in theory, since the
+                // others should reject negatives during parsing. But SMIL
+                // allows parsing negatives, and relies on us _not_ doing that
+                // clamping. That's so bonkers :(
+                CSSPixelLength::from(calc.to_used_value(base_size.resolve(context)))
+                    .clamp_to_non_negative()
             },
             FontSize::Keyword(i) => {
                 // As a specified keyword, this is keyword derived
                 info = Some(i);
-                i.to_computed_value(context)
+                i.to_computed_value(context).clamp_to_non_negative()
             },
             FontSize::Smaller => {
                 info = compose_keyword(1. / LARGER_FONT_SIZE_RATIO);
                 FontRelativeLength::Em(1. / LARGER_FONT_SIZE_RATIO)
                     .to_computed_value(context, base_size)
-                    .into()
             },
             FontSize::Larger => {
                 info = compose_keyword(LARGER_FONT_SIZE_RATIO);
                 FontRelativeLength::Em(LARGER_FONT_SIZE_RATIO)
                     .to_computed_value(context, base_size)
-                    .into()
             },
 
             FontSize::System(_) => {
@@ -903,12 +994,12 @@ impl FontSize {
                 }
                 #[cfg(feature = "gecko")]
                 {
-                    context.cached_system_font.as_ref().unwrap().font_size.size
+                    context.cached_system_font.as_ref().unwrap().font_size.size.0
                 }
             },
         };
         computed::FontSize {
-            size: size,
+            size: NonNegative(size),
             keyword_info: info,
         }
     }
@@ -952,7 +1043,7 @@ impl FontSize {
         }
 
         if let Ok(kw) = input.try(KeywordSize::parse) {
-            return Ok(FontSize::Keyword(kw.into()));
+            return Ok(FontSize::Keyword(KeywordInfo::new(kw)));
         }
 
         try_match_ident_ignore_ascii_case! { input,

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

@@ -90,8 +90,7 @@ impl ToComputedValue for LineHeight {
                 let result = match non_negative_lp.0 {
                     LengthPercentage::Length(NoCalcLength::Absolute(ref abs)) => {
                         context
-                            .maybe_zoom_text(abs.to_computed_value(context).into())
-                            .0
+                            .maybe_zoom_text(abs.to_computed_value(context))
                     },
                     LengthPercentage::Length(ref length) => length.to_computed_value(context),
                     LengthPercentage::Percentage(ref p) => FontRelativeLength::Em(p.0)

testing/web-platform/tests/css/css-fonts/font-size-relative-across-calc-ff-bug-001.html

@@ -0,0 +1,15 @@
+<!doctype html>
+<link rel="help" href="https://drafts.csswg.org/css-fonts-4/#font-size-prop">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1572738">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1572451">
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<script type="text/javascript" src="/resources/testharness.js"></script>
+<script type="text/javascript" src="/resources/testharnessreport.js"></script>
+<div style="font-family: something-non-default; font-size: calc(-100em + 1px);"></div>
+<div style="font-family: something-non-default; font-size: calc(1em - 100px);"></div>
+<script>
+test(function() {
+  for (const element of document.querySelectorAll("div"))
+    assert_equals(getComputedStyle(element).fontSize, "0px");
+}, "font-size computation isn't messed up when mixing positive and negatives when font-family changes and the parent has a keyword font-size");
+</script>