servo: Merge #19807 - style: Make the Gecko font-size calc() code do what it means to do (from emilio:calc-font-size); r=Manishearth
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 18 Jan 2018 09:36:51 -0600
changeset 454159 110b50e49ca4064fbe78deaf0bf9fd72a45e45af
parent 454155 b06e69e9a326d0c672bf678b85a36ba9a008f1f4
child 454160 f8230d6dd54f00b3439e5a587499255e29f9ebd9
push id1648
push usermtabara@mozilla.com
push dateThu, 01 Mar 2018 12:45:47 +0000
treeherdermozilla-release@cbb9688c2eeb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersManishearth
milestone59.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
servo: Merge #19807 - style: Make the Gecko font-size calc() code do what it means to do (from emilio:calc-font-size); r=Manishearth It makes no sense to pass a custom base size of zero in presence of rem, ex, or ch units. Bug: 1431031 Reviewed-by: Manishearth MozReview-Commit-ID: 7ZZwRzQKREX Source-Repo: https://github.com/servo/servo Source-Revision: 671b69c0b77f9a4bd0c098cb2a2f73c95dacb954
servo/components/style/properties/gecko.mako.rs
servo/components/style/values/specified/font.rs
servo/components/style/values/specified/length.rs
--- a/servo/components/style/properties/gecko.mako.rs
+++ b/servo/components/style/properties/gecko.mako.rs
@@ -2500,19 +2500,22 @@ fn static_assert() {
             self.gecko.mFontSizeKeyword = structs::NS_STYLE_FONT_SIZE_NO_KEYWORD as u8;
             self.gecko.mFontSizeFactor = 1.;
             self.gecko.mFontSizeOffset = 0;
         }
     }
 
     /// Set font size, taking into account scriptminsize and scriptlevel
     /// Returns Some(size) if we have to recompute the script unconstrained size
-    pub fn apply_font_size(&mut self, v: FontSize,
-                           parent: &Self,
-                           device: &Device) -> Option<NonNegativeLength> {
+    pub fn apply_font_size(
+        &mut self,
+        v: FontSize,
+        parent: &Self,
+        device: &Device,
+    ) -> Option<NonNegativeLength> {
         let (adjusted_size, adjusted_unconstrained_size) =
             self.calculate_script_level_size(parent, device);
         // In this case, we have been unaffected by scriptminsize, ignore it
         if parent.gecko.mSize == parent.gecko.mScriptUnconstrainedSize &&
            adjusted_size == adjusted_unconstrained_size {
             self.set_font_size(v);
             self.fixup_font_min_size(device);
             None
--- a/servo/components/style/values/specified/font.rs
+++ b/servo/components/style/values/specified/font.rs
@@ -588,25 +588,35 @@ impl FontSize {
                 base_size.resolve(context).scale_by(pc.0).into()
             }
             FontSize::Length(LengthOrPercentage::Calc(ref calc)) => {
                 let parent = context.style().get_parent_font().clone_font_size();
                 // if we contain em/% units and the parent was keyword derived, this is too
                 // Extract the ratio/offset and compose it
                 if (calc.em.is_some() || calc.percentage.is_some()) && parent.keyword_info.is_some() {
                     let ratio = calc.em.unwrap_or(0.) + calc.percentage.map_or(0., |pc| pc.0);
-                    // Compute it, but shave off the font-relative part (em, %)
-                    // This will mean that other font-relative units like ex and ch will be computed against
-                    // the old font even when the font changes. There's no particular "right answer" for what
-                    // to do here -- Gecko recascades as if the font had changed, we instead track the changes
-                    // and reapply, which means that we carry over old computed ex/ch values whilst Gecko
-                    // recomputes new ones. This is enough of an edge case to not really matter.
-                    let abs = calc.to_computed_value_zoomed(context, FontBaseSize::Custom(Au(0).into()))
-                                  .length_component().into();
-                    info = parent.keyword_info.map(|i| i.compose(ratio, abs));
+                    // Compute it, but shave off the font-relative part (em, %).
+                    //
+                    // This will mean that other font-relative units like ex and
+                    // ch will be computed against the old parent font even when
+                    // the font changes.
+                    //
+                    // There's no particular "right answer" for what to do here,
+                    // Gecko recascades as if the font had changed, we instead
+                    // track the changes and reapply, which means that we carry
+                    // over old computed ex/ch values whilst Gecko recomputes
+                    // new ones.
+                    //
+                    // This is enough of an edge case to not really matter.
+                    let abs = calc.to_computed_value_zoomed(
+                        context,
+                        FontBaseSize::InheritedStyleButStripEmUnits,
+                    ).length_component();
+
+                    info = parent.keyword_info.map(|i| i.compose(ratio, abs.into()));
                 }
                 let calc = calc.to_computed_value_zoomed(context, base_size);
                 calc.to_used_value(Some(base_size.resolve(context))).unwrap().into()
             }
             FontSize::Keyword(i) => {
                 // As a specified keyword, this is keyword derived
                 info = Some(i);
                 i.to_computed_value(context)
--- a/servo/components/style/values/specified/length.rs
+++ b/servo/components/style/values/specified/length.rs
@@ -66,74 +66,93 @@ pub enum FontRelativeLength {
     /// A "rem" value: https://drafts.csswg.org/css-values/#rem
     #[css(dimension)]
     Rem(CSSFloat)
 }
 
 /// A source to resolve font-relative units against
 #[derive(Clone, Copy, Debug, PartialEq)]
 pub enum FontBaseSize {
-    /// Use the font-size of the current element
+    /// Use the font-size of the current element.
     CurrentStyle,
-    /// Use the inherited font-size
+    /// Use the inherited font-size.
     InheritedStyle,
-    /// Use a custom base size
+    /// Use the inherited font-size, but strip em units.
+    ///
+    /// FIXME(emilio): This is very complex, and should go away.
+    InheritedStyleButStripEmUnits,
+    /// Use a custom base size.
+    ///
+    /// FIXME(emilio): This is very dubious, and only used for MathML.
     Custom(Au),
 }
 
 impl FontBaseSize {
     /// Calculate the actual size for a given context
     pub fn resolve(&self, context: &Context) -> Au {
         match *self {
             FontBaseSize::Custom(size) => size,
             FontBaseSize::CurrentStyle => context.style().get_font().clone_font_size().size(),
+            FontBaseSize::InheritedStyleButStripEmUnits |
             FontBaseSize::InheritedStyle => context.style().get_parent_font().clone_font_size().size(),
         }
     }
 }
 
 impl FontRelativeLength {
     /// Computes the font-relative length.
     pub fn to_computed_value(&self, context: &Context, base_size: FontBaseSize) -> CSSPixelLength {
         use std::f32;
         let (reference_size, length) = self.reference_font_size_and_length(context, base_size);
         let pixel = (length * reference_size.to_f32_px()).min(f32::MAX).max(f32::MIN);
         CSSPixelLength::new(pixel)
     }
 
-    /// Return reference font size. We use the base_size flag to pass a different size
-    /// for computing font-size and unconstrained font-size.
-    /// This returns a pair, the first one is the reference font size, and the second one is the
-    /// unpacked relative length.
+    /// Return reference font size.
+    ///
+    /// We use the base_size flag to pass a different size for computing
+    /// font-size and unconstrained font-size.
+    ///
+    /// This returns a pair, the first one is the reference font size, and the
+    /// second one is the unpacked relative length.
     fn reference_font_size_and_length(
         &self,
         context: &Context,
         base_size: FontBaseSize,
     ) -> (Au, CSSFloat) {
-        fn query_font_metrics(context: &Context, reference_font_size: Au) -> FontMetricsQueryResult {
-            context.font_metrics_provider.query(context.style().get_font(),
-                                                reference_font_size,
-                                                context.style().writing_mode,
-                                                context.in_media_query,
-                                                context.device())
+        fn query_font_metrics(
+            context: &Context,
+            reference_font_size: Au,
+        ) -> FontMetricsQueryResult {
+            context.font_metrics_provider.query(
+                context.style().get_font(),
+                reference_font_size,
+                context.style().writing_mode,
+                context.in_media_query,
+                context.device(),
+            )
         }
 
         let reference_font_size = base_size.resolve(context);
-
         match *self {
             FontRelativeLength::Em(length) => {
                 if context.for_non_inherited_property.is_some() {
-                    if matches!(base_size, FontBaseSize::CurrentStyle) {
+                    if base_size == FontBaseSize::CurrentStyle {
                         context.rule_cache_conditions.borrow_mut()
                             .set_font_size_dependency(
                                 reference_font_size.into()
                             );
                     }
                 }
-                (reference_font_size, length)
+
+                if base_size == FontBaseSize::InheritedStyleButStripEmUnits {
+                    (Au(0), length)
+                } else {
+                    (reference_font_size, length)
+                }
             },
             FontRelativeLength::Ex(length) => {
                 if context.for_non_inherited_property.is_some() {
                     context.rule_cache_conditions.borrow_mut().set_uncacheable();
                 }
                 let reference_size = match query_font_metrics(context, reference_font_size) {
                     FontMetricsQueryResult::Available(metrics) => {
                         metrics.x_height
@@ -177,17 +196,17 @@ impl FontRelativeLength {
                     }
                 };
                 (reference_size, length)
             }
             FontRelativeLength::Rem(length) => {
                 // https://drafts.csswg.org/css-values/#rem:
                 //
                 //     When specified on the font-size property of the root
-                //     element, the rem units refer to the property’s initial
+                //     element, the rem units refer to the property's initial
                 //     value.
                 //
                 let reference_size = if context.is_root_element || context.in_media_query {
                     reference_font_size
                 } else {
                     context.device().root_font_size()
                 };
                 (reference_size, length)