style: Try to bring some more sanity into our font code.

It's not very easy to understand on its current state, and it causes subtle bugs
like bug 1533654.

It could be simpler if we centralized where the interactions between properties
are handled. This patch does this.

This patch also changes how MathML script sizes are tracked when scriptlevel
changes and they have relative fonts in between.

With this patch, any explicitly specified font-size is treated the same (being a
scriptlevel boundary), regardless of whether it's either an absolute size, a
relative size, or a wide keyword.

Relative lengths always resolve relative to the constrained size, which allows
us to avoid the double font-size computation, and not give up on sanity with
keyword font-sizes.

I think given no other browser supports scriptlevel it seems like the right
trade-off.

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

components/style/properties/cascade.rs

@@ -384,8 +384,6 @@ struct Cascade<'a, 'b: 'a> {
     cascade_mode: CascadeMode<'a>,
     seen: LonghandIdSet,
     reverted: PerOrigin<LonghandIdSet>,
-    saved_font_size: Option<PropertyDeclaration>,
-    saved_font_family: Option<PropertyDeclaration>,
 }
 
 impl<'a, 'b: 'a> Cascade<'a, 'b> {
@@ -395,8 +393,6 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
             cascade_mode,
             seen: LonghandIdSet::default(),
             reverted: Default::default(),
-            saved_font_size: None,
-            saved_font_family: None,
         }
     }
 
@@ -424,33 +420,8 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
         ))
     }
 
-    fn apply_declaration<Phase: CascadePhase>(
-        &mut self,
-        longhand_id: LonghandId,
-        declaration: &PropertyDeclaration,
-    ) {
-        // FIXME(emilio): Find a cleaner abstraction for this.
-        //
-        // font-size and font-family are special because in Gecko they're
-        // they're dependent on other early props, like lang and
-        // -moz-min-font-size-ratio. This sucks a bit, we should ideally
-        // move the font-size computation code somewhere else...
-        if Phase::is_early() {
-            if longhand_id == LonghandId::FontSize {
-                self.saved_font_size = Some(declaration.clone());
-                return;
-            }
-            if longhand_id == LonghandId::FontFamily {
-                self.saved_font_family = Some(declaration.clone());
-                return;
-            }
-        }
-
-        self.apply_declaration_ignoring_phase(longhand_id, declaration);
-    }
-
     #[inline(always)]
-    fn apply_declaration_ignoring_phase(
+    fn apply_declaration<Phase: CascadePhase>(
         &mut self,
         longhand_id: LonghandId,
         declaration: &PropertyDeclaration,
@@ -577,7 +548,7 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
         }
 
         if Phase::is_early() {
-            self.fixup_font_and_apply_saved_font_properties();
+            self.fixup_font_stuff();
             self.compute_writing_mode();
         } else {
             self.finished_applying_properties();
@@ -702,30 +673,129 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
         true
     }
 
-    // FIXME(emilio): It'd be really nice to simplify all this, somehow. This is
+    // The default font type (which is stored in FontFamilyList's
-    // very annoying code in lots of ways, and there are various bits about it
+    // `mDefaultFontType`) depends on the current lang group, so
-    // which I think are broken or could be improved, see the various FIXMEs
+    // we may need to recompute it if it changed.
-    // below.
+    #[inline]
-    fn fixup_font_and_apply_saved_font_properties(&mut self) {
-        let font_family = self.saved_font_family.take();
-        let font_size = self.saved_font_size.take();
-        let mut _skip_font_family = false;
-
     #[cfg(feature = "gecko")]
-        {
+    fn recompute_default_font_family_type_if_needed(&mut self) {
-            // <svg:text> is not affected by text zoom, and it uses a preshint
+        use crate::gecko_bindings::{bindings, structs};
-            // to disable it. We fix up the struct when this happens by
+
-            // unzooming its contained font values, which will have been zoomed
+        if !self.seen.contains(LonghandId::XLang) &&
-            // in the parent.
+           !self.seen.contains(LonghandId::FontFamily) {
-            //
+            return;
-            // FIXME(emilio): Could be cleaner if we just removed this property
+        }
-            // and made a style adjustment o something like that.
+
-            if self.seen.contains(LonghandId::XTextZoom) {
+        let builder = &mut self.context.builder;
+        let default_font_type = {
+            let font = builder.get_font().gecko();
+            let default_font_type = if font.mFont.systemFont {
+                structs::FontFamilyType::eFamily_none
+            } else {
+                unsafe {
+                    bindings::Gecko_nsStyleFont_ComputeDefaultFontType(
+                        builder.device.document(),
+                        font.mGenericID,
+                        font.mLanguage.mRawPtr,
+                    )
+                }
+            };
+
+            if font.mFont.fontlist.mDefaultFontType == default_font_type {
+                return;
+            }
+
+            default_font_type
+        };
+
+        builder.mutate_font().gecko_mut().mFont.fontlist.mDefaultFontType =
+            default_font_type;
+    }
+
+    /// Some keyword sizes depend on the font family and language.
+    #[cfg(feature = "gecko")]
+    fn recompute_keyword_font_size_if_needed(&mut self) {
+        use crate::values::computed::ToComputedValue;
+        use crate::values::specified;
+
+        if !self.seen.contains(LonghandId::XLang) &&
+           !self.seen.contains(LonghandId::FontFamily) {
+            return;
+        }
+
+        let new_size = {
+            let font = self.context.builder.get_font();
+            let new_size = match font.clone_font_size().keyword_info {
+                Some(info) => {
+                    self.context.for_non_inherited_property = None;
+                    specified::FontSize::Keyword(info).to_computed_value(self.context)
+                }
+                None => return,
+            };
+
+            if font.gecko().mScriptUnconstrainedSize == new_size.size().0 {
+                return;
+            }
+
+            new_size
+        };
+
+        self.context.builder.mutate_font().set_font_size(new_size);
+    }
+
+    /// Some properties, plus setting font-size itself, may make us go out of
+    /// our minimum font-size range.
+    #[cfg(feature = "gecko")]
+    fn constrain_font_size_if_needed(&mut self) {
+        use crate::gecko_bindings::bindings;
+
+        if !self.seen.contains(LonghandId::XLang) &&
+           !self.seen.contains(LonghandId::FontFamily) &&
+           !self.seen.contains(LonghandId::MozMinFontSizeRatio) &&
+           !self.seen.contains(LonghandId::FontSize) {
+            return;
+        }
+
+        let builder = &mut self.context.builder;
+        let min_font_size = {
+            let font = builder.get_font().gecko();
+            let min_font_size = unsafe {
+                bindings::Gecko_nsStyleFont_ComputeMinSize(
+                    font,
+                    builder.device.document(),
+                )
+            };
+
+            if font.mFont.size >= min_font_size {
+                return;
+            }
+
+            min_font_size
+        };
+
+        builder.mutate_font().gecko_mut().mFont.size = min_font_size;
+    }
+
+    /// <svg:text> is not affected by text zoom, and it uses a preshint
+    /// to disable it. We fix up the struct when this happens by
+    /// unzooming its contained font values, which will have been zoomed
+    /// in the parent.
+    ///
+    /// FIXME(emilio): Also, why doing this _before_ handling font-size? That
+    /// sounds wrong.
+    #[cfg(feature = "gecko")]
+    fn unzoom_fonts_if_needed(&mut self) {
+        if !self.seen.contains(LonghandId::XTextZoom) {
+            return;
+        }
+
         let builder = &mut self.context.builder;
 
         let parent_zoom = builder.get_parent_font().gecko().mAllowZoom;
         let zoom = builder.get_font().gecko().mAllowZoom;
-                if zoom != parent_zoom {
+        if zoom == parent_zoom {
+            return;
+        }
         debug_assert!(
             !zoom,
             "We only ever disable text zoom (in svg:text), never enable it"
@@ -733,111 +803,92 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
         let device = builder.device;
         builder.mutate_font().unzoom_fonts(device);
     }
+
+    /// MathML script* attributes do some very weird shit with font-size.
+    ///
+    /// Handle them specially here, separate from other font-size stuff.
+    ///
+    /// How this should interact with lang="" and font-family-dependent sizes is
+    /// not clear to me. For now just pretend those don't exist here.
+    #[cfg(feature = "gecko")]
+    fn handle_mathml_scriptlevel_if_needed(&mut self) {
+        use app_units::Au;
+        use std::cmp;
+
+        if !self.seen.contains(LonghandId::MozScriptLevel) &&
+           !self.seen.contains(LonghandId::MozScriptMinSize) &&
+           !self.seen.contains(LonghandId::MozScriptSizeMultiplier) {
+            return;
         }
 
-            // Whenever a single generic value is specified, Gecko used to do a
+        // If the user specifies a font-size, just let it be.
-            // bunch of recalculation walking up the rule tree, including
+        if self.seen.contains(LonghandId::FontSize) {
-            // handling the font-size stuff.
+            return;
-            //
-            // It basically repopulated the font struct with the default font
-            // for a given generic and language. We handle the font-size stuff
-            // separately, so this boils down to just copying over the
-            // font-family lists (no other aspect of the default font can be
-            // configured).
-            if self.seen.contains(LonghandId::XLang) || font_family.is_some() {
-                // If just the language changed, the inherited generic is all we
-                // need.
-                let mut generic = self.context.builder.get_parent_font().gecko().mGenericID;
-
-                // FIXME(emilio): Isn't this bogus for CSS wide keywords like
-                // reset or such?
-                if let Some(ref declaration) = font_family {
-                    if let PropertyDeclaration::FontFamily(ref fam) = *declaration {
-                        if let Some(id) = fam.single_generic() {
-                            generic = id;
-                            // In case of a specified font family with a single
-                            // generic, we will end up setting font family
-                            // below, but its value would get overwritten later
-                            // in the pipeline when cascading.
-                            //
-                            // We instead skip cascading font-family in that
-                            // case.
-                            //
-                            // In case of the language changing, we wish for a
-                            // specified font-family to override this, so we do
-                            // not skip cascading then.
-                            _skip_font_family = true;
-                        }
-                    }
         }
 
-                // FIXME(emilio): Why both setting the generic and passing it
+        let builder = &mut self.context.builder;
-                // down?
+        let (new_size, new_unconstrained_size) = {
-                let doc = self.context.builder.device.document();
+            let font = builder.get_font().gecko();
-                let gecko_font = self.context.builder.mutate_font().gecko_mut();
+            let parent_font = builder.get_parent_font().gecko();
-                gecko_font.mGenericID = generic;
+
-                unsafe {
+            let delta =
-                    crate::gecko_bindings::bindings::Gecko_nsStyleFont_PrefillDefaultForGeneric(
+                font.mScriptLevel.saturating_sub(parent_font.mScriptLevel);
-                        gecko_font,
+
-                        doc,
+            if delta == 0 {
-                        generic,
+                return;
-                    );
-                }
-            }
             }
 
-        // It is important that font-size is computed before the late
+            let mut min = Au(parent_font.mScriptMinSize);
-        // properties (for em units), but after font-family (for the
+            if font.mAllowZoom {
-        // base-font-size dependence for default and keyword font-sizes).
+                min = builder.device.zoom_text(min);
-        //
+            }
-        // It's important that font-family comes after the other font properties
+
-        // to support system fonts.
+            let scale = (parent_font.mScriptSizeMultiplier as f32).powi(delta as i32);
-        //
+            let parent_size = Au(parent_font.mSize);
-        // NOTE(emilio): I haven't verified that comment, but it was there.
+            let parent_unconstrained_size = Au(parent_font.mScriptUnconstrainedSize);
-        // Verify, and if it's false make font-size the only weird property?
+            let new_size = parent_size.scale_by(scale);
-        if !_skip_font_family {
+            let new_unconstrained_size = parent_unconstrained_size.scale_by(scale);
-            if let Some(ref declaration) = font_family {
+
-                self.apply_declaration_ignoring_phase(LonghandId::FontFamily, declaration);
+            if scale <= 1. {
+                // The parent size can be smaller than scriptminsize, e.g. if it
+                // was specified explicitly. Don't scale in this case, but we
+                // don't want to set it to scriptminsize either since that will
+                // make it larger.
+                if parent_size <= min {
+                    (parent_size, new_unconstrained_size)
+                } else {
+                    (cmp::max(min, new_size), new_unconstrained_size)
+                }
+            } else {
+                // If the new unconstrained size is larger than the min size,
+                // this means we have escaped the grasp of scriptminsize and can
+                // revert to using the unconstrained size.
+                // However, if the new size is even larger (perhaps due to usage
+                // of em units), use that instead.
+                (
+                    cmp::min(new_size, cmp::max(new_unconstrained_size, min)),
+                    new_unconstrained_size
+                )
+            }
+        };
+        let font = builder.mutate_font().gecko_mut();
+        font.mFont.size = new_size.0;
+        font.mSize = new_size.0;
+        font.mScriptUnconstrainedSize = new_unconstrained_size.0;
+    }
+
+    /// Various properties affect how font-size and font-family are computed.
+    ///
+    /// These need to be handled here, since relative lengths and ex / ch units
+    /// for late properties depend on these.
+    fn fixup_font_stuff(&mut self) {
         #[cfg(feature = "gecko")]
         {
-                    let context = &mut self.context;
+            self.unzoom_fonts_if_needed();
-                    let device = context.builder.device;
+            self.recompute_default_font_family_type_if_needed();
-                    if let PropertyDeclaration::FontFamily(ref val) = *declaration {
+            self.recompute_keyword_font_size_if_needed();
-                        if val.get_system().is_some() {
+            self.handle_mathml_scriptlevel_if_needed();
-                            let default = context
+            self.constrain_font_size_if_needed()
-                                .cached_system_font
-                                .as_ref()
-                                .unwrap()
-                                .default_font_type;
-                            context.builder.mutate_font().fixup_system(default);
-                        } else {
-                            context.builder.mutate_font().fixup_none_generic(device);
-                        }
-                    }
-                }
-            }
-        }
-
-        if let Some(declaration) = font_size {
-            self.apply_declaration_ignoring_phase(LonghandId::FontSize, &declaration);
-        } else {
-            #[cfg(feature = "gecko")]
-            {
-                if self.seen.contains(LonghandId::XLang) ||
-                    self.seen.contains(LonghandId::MozScriptLevel) ||
-                    self.seen.contains(LonghandId::MozMinFontSizeRatio) ||
-                    self.seen.contains(LonghandId::FontFamily)
-                {
-                    use crate::values::computed::FontSize;
-
-                    // font-size must be explicitly inherited to handle lang
-                    // changes and scriptlevel changes.
-                    //
-                    // FIXME(emilio): That looks a bit bogus...
-                    self.context.for_non_inherited_property = None;
-                    FontSize::cascade_inherit_font_size(&mut self.context);
-                }
-            }
         }
     }
 }

components/style/properties/gecko.mako.rs

@@ -1971,25 +1971,19 @@ fn static_assert() {
     <% impl_font_settings("font_feature_settings", "gfxFontFeature", "FeatureTagValue", "i32", "u32") %>
     <% impl_font_settings("font_variation_settings", "gfxFontVariation", "VariationValue", "f32", "f32") %>
 
-    pub fn fixup_none_generic(&mut self, device: &Device) {
-        self.gecko.mFont.systemFont = false;
-        unsafe {
-            bindings::Gecko_nsStyleFont_FixupNoneGeneric(&mut self.gecko, device.document())
-        }
-    }
-
-    pub fn fixup_system(&mut self, default_font_type: structs::FontFamilyType) {
-        self.gecko.mFont.systemFont = true;
-        self.gecko.mGenericID = structs::kGenericFont_NONE;
-        self.gecko.mFont.fontlist.mDefaultFontType = default_font_type;
-    }
-
     pub fn set_font_family(&mut self, v: longhands::font_family::computed_value::T) {
-        self.gecko.mGenericID = structs::kGenericFont_NONE;
+        use crate::gecko_bindings::structs::FontFamilyType;
-        if let Some(generic) = v.0.single_generic() {
+
-            self.gecko.mGenericID = generic;
+        let is_system_font = v.is_system_font;
-        }
+        self.gecko.mFont.systemFont = is_system_font;
-        self.gecko.mFont.fontlist.mFontlist.mBasePtr.set_move((v.0).0.clone());
+        self.gecko.mGenericID = if is_system_font {
+            structs::kGenericFont_NONE
+        } else {
+            v.families.single_generic().unwrap_or(structs::kGenericFont_NONE)
+        };
+        self.gecko.mFont.fontlist.mFontlist.mBasePtr.set_move(v.families.0.clone());
+        // Fixed-up if needed in Cascade::fixup_font_stuff.
+        self.gecko.mFont.fontlist.mDefaultFontType = FontFamilyType::eFamily_none;
     }
 
     pub fn copy_font_family_from(&mut self, other: &Self) {
@@ -2009,7 +2003,7 @@ fn static_assert() {
         let fontlist = &self.gecko.mFont.fontlist;
         let shared_fontlist = unsafe { fontlist.mFontlist.mBasePtr.to_safe() };
 
-        if shared_fontlist.mNames.is_empty() {
+        let families = if shared_fontlist.mNames.is_empty() {
             let default = fontlist.mDefaultFontType;
             let default = match default {
                 FontFamilyType::eFamily_serif => {
@@ -2030,9 +2024,14 @@ fn static_assert() {
                     SingleFontFamily::Generic(atom!("sans-serif"))
                 }
             };
-            FontFamily(FontFamilyList::new(Box::new([default])))
+            FontFamilyList::new(Box::new([default]))
         } else {
-            FontFamily(FontFamilyList(shared_fontlist))
+            FontFamilyList(shared_fontlist)
+        };
+
+        FontFamily {
+            families,
+            is_system_font: self.gecko.mFont.systemFont,
         }
     }
 
@@ -2042,10 +2041,32 @@ fn static_assert() {
         self.gecko.mFont.size = device.unzoom_text(Au(self.gecko.mFont.size)).0;
     }
 
+    pub fn copy_font_size_from(&mut self, other: &Self) {
+        self.gecko.mScriptUnconstrainedSize = other.gecko.mScriptUnconstrainedSize;
+
+        self.gecko.mSize = other.gecko.mScriptUnconstrainedSize;
+        self.gecko.mFont.size = other.gecko.mSize;
+        self.gecko.mFontSizeKeyword = other.gecko.mFontSizeKeyword;
+
+        // TODO(emilio): Should we really copy over these two?
+        self.gecko.mFontSizeFactor = other.gecko.mFontSizeFactor;
+        self.gecko.mFontSizeOffset = other.gecko.mFontSizeOffset;
+    }
+
+    pub fn reset_font_size(&mut self, other: &Self) {
+        self.copy_font_size_from(other)
+    }
+
     pub fn set_font_size(&mut self, v: FontSize) {
         use crate::values::generics::font::KeywordSize;
-        self.gecko.mSize = v.size().0;
+
-        self.gecko.mScriptUnconstrainedSize = v.size().0;
+        let size = v.size();
+        self.gecko.mScriptUnconstrainedSize = size.0;
+
+        // These two may be changed from Cascade::fixup_font_stuff.
+        self.gecko.mSize = size.0;
+        self.gecko.mFont.size = size.0;
+
         if let Some(info) = v.keyword_info {
             self.gecko.mFontSizeKeyword = match info.kw {
                 KeywordSize::XXSmall => structs::NS_STYLE_FONT_SIZE_XXSMALL,
@@ -2066,196 +2087,6 @@ fn static_assert() {
         }
     }
 
-    /// 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> {
-        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
-        } else {
-            self.gecko.mSize = v.size().0;
-            self.fixup_font_min_size(device);
-            Some(Au(parent.gecko.mScriptUnconstrainedSize).into())
-        }
-    }
-
-    pub fn fixup_font_min_size(&mut self, device: &Device) {
-        unsafe { bindings::Gecko_nsStyleFont_FixupMinFontSize(&mut self.gecko, device.document()) }
-    }
-
-    pub fn apply_unconstrained_font_size(&mut self, v: NonNegativeLength) {
-        self.gecko.mScriptUnconstrainedSize = v.0.to_i32_au();
-    }
-
-    /// Calculates the constrained and unconstrained font sizes to be inherited
-    /// from the parent.
-    ///
-    /// This is a port of Gecko's old ComputeScriptLevelSize function:
-    /// https://searchfox.org/mozilla-central/rev/c05d9d61188d32b8/layout/style/nsRuleNode.cpp#3103
-    ///
-    /// scriptlevel is a property that affects how font-size is inherited. If scriptlevel is
-    /// +1, for example, it will inherit as the script size multiplier times
-    /// the parent font. This does not affect cases where the font-size is
-    /// explicitly set.
-    ///
-    /// However, this transformation is not allowed to reduce the size below
-    /// scriptminsize. If this inheritance will reduce it to below
-    /// scriptminsize, it will be set to scriptminsize or the parent size,
-    /// whichever is smaller (the parent size could be smaller than the min size
-    /// because it was explicitly specified).
-    ///
-    /// Now, within a node that has inherited a font-size which was
-    /// crossing scriptminsize once the scriptlevel was applied, a negative
-    /// scriptlevel may be used to increase the size again.
-    ///
-    /// This should work, however if we have already been capped by the
-    /// scriptminsize multiple times, this can lead to a jump in the size.
-    ///
-    /// For example, if we have text of the form:
-    ///
-    /// huge large medium small tiny reallytiny tiny small medium huge
-    ///
-    /// which is represented by progressive nesting and scriptlevel values of
-    /// +1 till the center after which the scriptlevel is -1, the "tiny"s should
-    /// be the same size, as should be the "small"s and "medium"s, etc.
-    ///
-    /// However, if scriptminsize kicked it at around "medium", then
-    /// medium/tiny/reallytiny will all be the same size (the min size).
-    /// A -1 scriptlevel change after this will increase the min size by the
-    /// multiplier, making the second tiny larger than medium.
-    ///
-    /// Instead, we wish for the second "tiny" to still be capped by the script
-    /// level, and when we reach the second "large", it should be the same size
-    /// as the original one.
-    ///
-    /// We do this by cascading two separate font sizes. The font size (mSize)
-    /// is the actual displayed font size. The unconstrained font size
-    /// (mScriptUnconstrainedSize) is the font size in the situation where
-    /// scriptminsize never applied.
-    ///
-    /// We calculate the proposed inherited font size based on scriptlevel and
-    /// the parent unconstrained size, instead of using the parent font size.
-    /// This is stored in the node's unconstrained size and will also be stored
-    /// in the font size provided that it is above the min size.
-    ///
-    /// All of this only applies when inheriting. When the font size is
-    /// manually set, scriptminsize does not apply, and both the real and
-    /// unconstrained size are set to the explicit value. However, if the font
-    /// size is manually set to an em or percent unit, the unconstrained size
-    /// will be set to the value of that unit computed against the parent
-    /// unconstrained size, whereas the font size will be set computing against
-    /// the parent font size.
-    pub fn calculate_script_level_size(&self, parent: &Self, device: &Device) -> (Au, Au) {
-        use std::cmp;
-
-        let delta = self.gecko.mScriptLevel.saturating_sub(parent.gecko.mScriptLevel);
-
-        let parent_size = Au(parent.gecko.mSize);
-        let parent_unconstrained_size = Au(parent.gecko.mScriptUnconstrainedSize);
-
-        if delta == 0 {
-            return (parent_size, parent_unconstrained_size)
-        }
-
-
-        let mut min = Au(parent.gecko.mScriptMinSize);
-        if self.gecko.mAllowZoom {
-            min = device.zoom_text(min);
-        }
-
-        let scale = (parent.gecko.mScriptSizeMultiplier as f32).powi(delta as i32);
-
-        let new_size = parent_size.scale_by(scale);
-        let new_unconstrained_size = parent_unconstrained_size.scale_by(scale);
-
-        if scale < 1. {
-            // The parent size can be smaller than scriptminsize,
-            // e.g. if it was specified explicitly. Don't scale
-            // in this case, but we don't want to set it to scriptminsize
-            // either since that will make it larger.
-            if parent_size < min {
-                (parent_size, new_unconstrained_size)
-            } else {
-                (cmp::max(min, new_size), new_unconstrained_size)
-            }
-        } else {
-            // If the new unconstrained size is larger than the min size,
-            // this means we have escaped the grasp of scriptminsize
-            // and can revert to using the unconstrained size.
-            // However, if the new size is even larger (perhaps due to usage
-            // of em units), use that instead.
-            (cmp::min(new_size, cmp::max(new_unconstrained_size, min)),
-             new_unconstrained_size)
-        }
-    }
-
-    /// This function will also handle scriptminsize and scriptlevel
-    /// so should not be called when you just want the font sizes to be copied.
-    /// Hence the different name.
-    pub fn inherit_font_size_from(&mut self, parent: &Self,
-                                  kw_inherited_size: Option<NonNegativeLength>,
-                                  device: &Device) {
-        let (adjusted_size, adjusted_unconstrained_size)
-            = self.calculate_script_level_size(parent, device);
-        if adjusted_size.0 != parent.gecko.mSize ||
-           adjusted_unconstrained_size.0 != parent.gecko.mScriptUnconstrainedSize {
-            // FIXME(Manishearth): This is incorrect. When there is both a
-            // keyword size being inherited and a scriptlevel change, we must
-            // handle the keyword size the same way we handle em units. This
-            // complicates things because we now have to keep track of the
-            // adjusted and unadjusted ratios in the kw font size. This only
-            // affects the use case of a generic font being used in MathML.
-            //
-            // If we were to fix this I would prefer doing it not doing
-            // something like the ruletree walk that Gecko used to do in
-            // nsRuleNode::SetGenericFont and instead using extra bookkeeping in
-            // the mSize and mScriptUnconstrainedSize values, and reusing those
-            // instead of font_size_keyword.
-
-            // In the case that MathML has given us an adjusted size, apply it.
-            // Keep track of the unconstrained adjusted size.
-            self.gecko.mSize = adjusted_size.0;
-
-            // Technically the MathML constrained size may also be keyword-derived
-            // but we ignore this since it would be too complicated
-            // to correctly track and it's mostly unnecessary.
-            self.gecko.mFontSizeKeyword = structs::NS_STYLE_FONT_SIZE_NO_KEYWORD as u8;
-            self.gecko.mFontSizeFactor = 1.;
-            self.gecko.mFontSizeOffset = 0;
-
-            self.gecko.mScriptUnconstrainedSize = adjusted_unconstrained_size.0;
-        } else if let Some(size) = kw_inherited_size {
-            // Parent element was a keyword-derived size.
-            self.gecko.mSize = size.0.to_i32_au();
-            // Copy keyword info over.
-            self.gecko.mFontSizeFactor = parent.gecko.mFontSizeFactor;
-            self.gecko.mFontSizeOffset = parent.gecko.mFontSizeOffset;
-            self.gecko.mFontSizeKeyword = parent.gecko.mFontSizeKeyword;
-            // MathML constraints didn't apply here, so we can ignore this.
-            self.gecko.mScriptUnconstrainedSize = size.0.to_i32_au();
-        } else {
-            // MathML isn't affecting us, and our parent element does not
-            // have a keyword-derived size. Set things normally.
-            self.gecko.mSize = parent.gecko.mSize;
-            // copy keyword info over
-            self.gecko.mFontSizeKeyword = structs::NS_STYLE_FONT_SIZE_NO_KEYWORD as u8;
-            self.gecko.mFontSizeFactor = 1.;
-            self.gecko.mFontSizeOffset = 0;
-            self.gecko.mScriptUnconstrainedSize = parent.gecko.mScriptUnconstrainedSize;
-        }
-        self.fixup_font_min_size(device);
-    }
-
     pub fn clone_font_size(&self) -> FontSize {
         use crate::values::generics::font::{KeywordInfo, KeywordSize};
         let size = Au(self.gecko.mSize).into();
@@ -2270,16 +2101,16 @@ fn static_assert() {
             structs::NS_STYLE_FONT_SIZE_XXXLARGE => KeywordSize::XXXLarge,
             structs::NS_STYLE_FONT_SIZE_NO_KEYWORD => {
                 return FontSize {
-                    size: size,
+                    size,
                     keyword_info: None,
                 }
             }
             _ => unreachable!("mFontSizeKeyword should be an absolute keyword or NO_KEYWORD")
         };
         FontSize {
-            size: size,
+            size,
             keyword_info: Some(KeywordInfo {
-                kw: kw,
+                kw,
                 factor: self.gecko.mFontSizeFactor,
                 offset: Au(self.gecko.mFontSizeOffset).into()
             })

components/style/properties/helpers.mako.rs

@@ -332,13 +332,9 @@
                         CSSWideKeyword::Initial => {
                             % if not property.style_struct.inherited:
                                 debug_assert!(false, "Should be handled in apply_properties");
-                            % else:
-                                % if property.name == "font-size":
-                                computed::FontSize::cascade_initial_font_size(context);
                             % else:
                                 context.builder.reset_${property.ident}();
                             % endif
-                            % endif
                         },
                         % if property.style_struct.inherited:
                         CSSWideKeyword::Unset |
@@ -394,16 +390,8 @@
                 % else:
                 let computed = specified_value.to_computed_value(context);
                 % endif
-                % if property.ident == "font_size":
-                    specified::FontSize::cascade_specified_font_size(
-                        context,
-                        &specified_value,
-                        computed,
-                    );
-                % else:
                 context.builder.set_${property.ident}(computed)
             % endif
-            % endif
         }
 
         pub fn parse_declared<'i, 't>(

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

@@ -379,7 +379,7 @@ ${helpers.predefined_type(
                 use crate::gecko_bindings::structs::{LookAndFeel_FontID, nsFont};
                 use std::mem;
                 use crate::values::computed::Percentage;
-                use crate::values::computed::font::{FontSize, FontStretch, FontStyle, FontFamilyList};
+                use crate::values::computed::font::{FontFamily, FontSize, FontStretch, FontStyle, FontFamilyList};
                 use crate::values::generics::NonNegative;
 
                 let id = match *self {
@@ -405,11 +405,12 @@ ${helpers.predefined_type(
                 })));
                 let font_style = FontStyle::from_gecko(system.style);
                 let ret = ComputedSystemFont {
-                    font_family: longhands::font_family::computed_value::T(
+                    font_family: FontFamily {
-                        FontFamilyList(
+                        families: FontFamilyList(unsafe {
-                            unsafe { system.fontlist.mFontlist.mBasePtr.to_safe() }
+                            system.fontlist.mFontlist.mBasePtr.to_safe()
-                        )
+                        }),
-                    ),
+                        is_system_font: true,
+                    },
                     font_size: FontSize {
                         size: Au(system.size).into(),
                         keyword_info: None

components/style/properties/properties.mako.rs

@@ -3470,7 +3470,7 @@ impl<'a> StyleBuilder<'a> {
                 % endif
             );
     }
-    % elif property.name != "font-size":
+    % else:
     /// Reset `${property.ident}` to the initial value.
     #[allow(non_snake_case)]
     pub fn reset_${property.ident}(&mut self) {

components/style/values/computed/font.rs

@@ -152,49 +152,6 @@ impl FontSize {
             keyword_info: Some(KeywordInfo::medium()),
         }
     }
-
-    /// FIXME(emilio): This is very complex. Also, it should move to
-    /// StyleBuilder.
-    pub fn cascade_inherit_font_size(context: &mut Context) {
-        // If inheriting, we must recompute font-size in case of language
-        // changes using the font_size_keyword. We also need to do this to
-        // handle mathml scriptlevel changes
-        let kw_inherited_size = context
-            .builder
-            .get_parent_font()
-            .clone_font_size()
-            .keyword_info
-            .map(|info| {
-                specified::FontSize::Keyword(info)
-                    .to_computed_value(context)
-                    .size
-            });
-        let mut font = context.builder.take_font();
-        font.inherit_font_size_from(
-            context.builder.get_parent_font(),
-            kw_inherited_size,
-            context.builder.device,
-        );
-        context.builder.put_font(font);
-    }
-
-    /// Cascade the initial value for the `font-size` property.
-    ///
-    /// FIXME(emilio): This is the only function that is outside of the
-    /// `StyleBuilder`, and should really move inside!
-    ///
-    /// Can we move the font stuff there?
-    pub fn cascade_initial_font_size(context: &mut Context) {
-        // font-size's default ("medium") does not always
-        // compute to the same value and depends on the font
-        let computed = specified::FontSize::medium().to_computed_value(context);
-        context.builder.mutate_font().set_font_size(computed);
-        #[cfg(feature = "gecko")]
-        {
-            let device = context.builder.device;
-            context.builder.mutate_font().fixup_font_min_size(device);
-        }
-    }
 }
 
 /// XXXManishearth it might be better to
@@ -221,15 +178,23 @@ impl ToAnimatedValue for FontSize {
 #[derive(Clone, Debug, Eq, Hash, PartialEq)]
 #[cfg_attr(feature = "servo", derive(MallocSizeOf))]
 /// Specifies a prioritized list of font family names or generic family names.
-pub struct FontFamily(pub FontFamilyList);
+pub struct FontFamily {
+    /// The actual list of family names.
+    pub families: FontFamilyList,
+    /// Whether this font-family came from a specified system-font.
+    pub is_system_font: bool,
+}
 
 impl FontFamily {
     #[inline]
     /// Get default font family as `serif` which is a generic font-family
     pub fn serif() -> Self {
-        FontFamily(FontFamilyList::new(Box::new([SingleFontFamily::Generic(
+        FontFamily {
+            families: FontFamilyList::new(Box::new([SingleFontFamily::Generic(
                 atom!("serif"),
-        )])))
+            )])),
+            is_system_font: false,
+        }
     }
 }
 
@@ -239,7 +204,7 @@ impl MallocSizeOf for FontFamily {
         // SharedFontList objects are generally shared from the pointer
         // stored in the specified value. So only count this if the
         // SharedFontList is unshared.
-        unsafe { bindings::Gecko_SharedFontList_SizeOfIncludingThisIfUnshared((self.0).0.get()) }
+        unsafe { bindings::Gecko_SharedFontList_SizeOfIncludingThisIfUnshared(self.families.0.get()) }
     }
 }
 
@@ -248,7 +213,7 @@ impl ToCss for FontFamily {
     where
         W: fmt::Write,
     {
-        let mut iter = self.0.iter();
+        let mut iter = self.families.iter();
         iter.next().unwrap().to_css(dest)?;
         for family in iter {
             dest.write_str(", ")?;

components/style/values/specified/font.rs

@@ -548,13 +548,16 @@ impl ToComputedValue for FontFamily {
 
     fn to_computed_value(&self, context: &Context) -> Self::ComputedValue {
         match *self {
-            FontFamily::Values(ref v) => computed::FontFamily(v.clone()),
+            FontFamily::Values(ref v) => computed::FontFamily {
+                families: v.clone(),
+                is_system_font: false,
+            },
             FontFamily::System(_) => self.compute_system(context),
         }
     }
 
     fn from_computed_value(other: &computed::FontFamily) -> Self {
-        FontFamily::Values(other.0.clone())
+        FontFamily::Values(other.families.clone())
     }
 }
 
@@ -958,48 +961,6 @@ impl FontSize {
             "larger" => Ok(FontSize::Larger),
         }
     }
-
-    #[allow(unused_mut)]
-    /// Cascade `font-size` with specified value
-    pub fn cascade_specified_font_size(
-        context: &mut Context,
-        specified_value: &FontSize,
-        mut computed: computed::FontSize,
-    ) {
-        // we could use clone_language and clone_font_family() here but that's
-        // expensive. Do it only in gecko mode for now.
-        #[cfg(feature = "gecko")]
-        {
-            // if the language or generic changed, we need to recalculate
-            // the font size from the stored font-size origin information.
-            if context.builder.get_font().gecko().mLanguage.mRawPtr !=
-                context.builder.get_parent_font().gecko().mLanguage.mRawPtr ||
-                context.builder.get_font().gecko().mGenericID !=
-                    context.builder.get_parent_font().gecko().mGenericID
-            {
-                if let Some(info) = computed.keyword_info {
-                    computed.size = info.to_computed_value(context);
-                }
-            }
-        }
-
-        let device = context.builder.device;
-        let mut font = context.builder.take_font();
-        let parent_unconstrained = {
-            let parent_font = context.builder.get_parent_font();
-            font.apply_font_size(computed, parent_font, device)
-        };
-        context.builder.put_font(font);
-
-        if let Some(parent) = parent_unconstrained {
-            let new_unconstrained = specified_value
-                .to_computed_value_against(context, FontBaseSize::Custom(Au::from(parent)));
-            context
-                .builder
-                .mutate_font()
-                .apply_unconstrained_font_size(new_unconstrained.size);
-        }
-    }
 }
 
 impl Parse for FontSize {

components/style/values/specified/length.rs

@@ -82,17 +82,12 @@ pub enum FontBaseSize {
     ///
     /// 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()