Bug 1431031: Make the font-size calc() code do what it means to do. draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 17 Jan 2018 17:20:20 +0100
changeset 721790 14c188daf498834f8d25fe27c42f75fec734fa54
parent 721789 d22e3b436114f2d0c1c7af4b1157881cba6e42c0
child 722161 c89d6f0e3bb0b1870f28b3a1520416c7bee88d63
push id95947
push userbmo:emilio@crisal.io
push dateWed, 17 Jan 2018 21:20:18 +0000
bugs1431031
milestone59.0a1
Bug 1431031: Make the font-size calc() code do what it means to do. It makes no sense to pass a custom base size of zero in presence of rem, ex, or ch units. MozReview-Commit-ID: 7ZZwRzQKREX
servo/components/style/properties/gecko.mako.rs
servo/components/style/values/specified/font.rs
servo/components/style/values/specified/length.rs
testing/web-platform/meta/MANIFEST.json
testing/web-platform/tests/css/css-values/calc-ch-ex-lang-ref.html
testing/web-platform/tests/css/css-values/calc-ch-ex-lang.html
testing/web-platform/tests/css/css-values/calc-rem-lang-ref.html
testing/web-platform/tests/css/css-values/calc-rem-lang.html
--- 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) {
                         context.rule_cache_conditions.borrow_mut()
                             .set_font_size_dependency(
                                 reference_font_size.into()
                             );
                     }
                 }
-                (reference_font_size, length)
+
+                if matches!(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)
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -146378,16 +146378,28 @@
       [
        "/css/css-values/reference/200-200-green.html",
        "=="
       ]
      ],
      {}
     ]
    ],
+   "css/css-values/calc-ch-ex-lang.html": [
+    [
+     "/css/css-values/calc-ch-ex-lang.html",
+     [
+      [
+       "/css/css-values/calc-ch-ex-lang-ref.html",
+       "=="
+      ]
+     ],
+     {}
+    ]
+   ],
    "css/css-values/calc-in-calc.html": [
     [
      "/css/css-values/calc-in-calc.html",
      [
       [
        "/css/css-values/reference/all-green.html",
        "=="
       ]
@@ -146438,16 +146450,28 @@
       [
        "/css/css-values/reference/all-green.html",
        "=="
       ]
      ],
      {}
     ]
    ],
+   "css/css-values/calc-rem-lang.html": [
+    [
+     "/css/css-values/calc-rem-lang.html",
+     [
+      [
+       "/css/css-values/calc-rem-lang-ref.html",
+       "=="
+      ]
+     ],
+     {}
+    ]
+   ],
    "css/css-values/ch-unit-001.html": [
     [
      "/css/css-values/ch-unit-001.html",
      [
       [
        "/css/css-values/reference/ch-unit-001-ref.html",
        "=="
       ]
@@ -253663,16 +253687,26 @@
      {}
     ]
    ],
    "css/css-values/OWNERS": [
     [
      {}
     ]
    ],
+   "css/css-values/calc-ch-ex-lang-ref.html": [
+    [
+     {}
+    ]
+   ],
+   "css/css-values/calc-rem-lang-ref.html": [
+    [
+     {}
+    ]
+   ],
    "css/css-values/ex-calc-expression-001-ref.html": [
     [
      {}
     ]
    ],
    "css/css-values/reference/200-200-green.html": [
     [
      {}
@@ -510085,16 +510119,24 @@
   "css/css-values/attr-px-invalid-fallback.html": [
    "855fa8131c6fcf734982d427b0e46ab2be90ae80",
    "reftest"
   ],
   "css/css-values/attr-px-valid.html": [
    "70627dbbfc5af5fb859fdf1362f0004b38c64e34",
    "reftest"
   ],
+  "css/css-values/calc-ch-ex-lang-ref.html": [
+   "218e1cb6782b554de35aad70913cddb8e7d71da2",
+   "support"
+  ],
+  "css/css-values/calc-ch-ex-lang.html": [
+   "d15f42e237b281793ac808e06af92217d4f9593e",
+   "reftest"
+  ],
   "css/css-values/calc-in-calc.html": [
    "be08a1510714e8b4fbc4d35582db5708924d06b2",
    "reftest"
   ],
   "css/css-values/calc-in-media-queries-001.html": [
    "f8fdd8373eaca7a03d6a089b4faf71825c8bfaf2",
    "reftest"
   ],
@@ -510105,16 +510147,24 @@
   "css/css-values/calc-invalid-range-clamping.html": [
    "3352575826aa7e335ef41b9e4f3386deda4a54b0",
    "reftest"
   ],
   "css/css-values/calc-parenthesis-stack.html": [
    "080551c1bee3d7bf54dda2c3d5b7e5a9fbd8aed6",
    "reftest"
   ],
+  "css/css-values/calc-rem-lang-ref.html": [
+   "08bbc95f3078421a489e1e93cc7a4f035af40d5b",
+   "support"
+  ],
+  "css/css-values/calc-rem-lang.html": [
+   "6fa668d2bcaf01f5c4680e3e14a0e86160d1b5d5",
+   "reftest"
+  ],
   "css/css-values/calc-unit-analysis.html": [
    "c5fd567b4fa257ce53c48ebf8c444bf382459fec",
    "testharness"
   ],
   "css/css-values/ch-unit-001.html": [
    "6a9d4e97823a3c1b2babf5dcc98595a4697c5eb4",
    "reftest"
   ],
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-values/calc-ch-ex-lang-ref.html
@@ -0,0 +1,12 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>CSS Test Reference</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<style>
+div {
+  width: calc(1ex + 1ch + 1em);
+  height: calc(1ex + 1ch + 1em);
+  background: green;
+}
+</style>
+<div></div>
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-values/calc-ch-ex-lang.html
@@ -0,0 +1,16 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>CSS Test: Calc in font-size with ch / ex units across lang changes</title>
+<link rel="help" href="https://drafts.csswg.org/css-values/#ch">
+<link rel="help" href="https://drafts.csswg.org/css-values/#ex">
+<link rel="help" href="https://drafts.csswg.org/css-values/#funcdef-calc">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1431031">
+<link rel="match" href="calc-ch-ex-lang-ref.html">
+<style>
+div[lang] {
+  font-size: calc(1ex + 1ch + 1em);
+}
+</style>
+<div lang="en">
+  <div style="width: 1em; height: 1em; background: green;"></div>
+</div>
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-values/calc-rem-lang-ref.html
@@ -0,0 +1,6 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>CSS Test Reference</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<p>You should see a green box twice-the-initial-font-size wide.</p>
+<div style="width: 2em; height: 2em; background: green;"></div>
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-values/calc-rem-lang.html
@@ -0,0 +1,17 @@
+<!doctype html>
+<html lang="en"><!-- The lang is important! -->
+<meta charset="utf-8">
+<title>CSS Test: Calc with rem and relative units on the root element</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="help" href="https://drafts.csswg.org/css-values/#rem">
+<link rel="help" href="https://drafts.csswg.org/css-values/#funcdef-calc">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1431031">
+<link rel="match" href="calc-rem-lang-ref.html">
+<style>
+  html {
+    font-size: calc(1rem + 1em);
+  }
+</style>
+<p style="font-size: initial">You should see a green box twice-the-initial-font-size wide.</p>
+<div style="width: 1em; height: 1em; background: green;"></div>
+</html>