Bug 1536718 - Honor browser.display.use_document_fonts again. r=manishearth,jwatt
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 26 Mar 2019 18:42:27 +0000
changeset 466185 d25cc097d600ac2423c9b3f13aba671f201376d1
parent 466184 8691a4583cae7e73381a844c3d616354671184a9
child 466186 b88010af82cf67f99fb6c64bb9bb3d1494f8c6d8
push id112571
push usercsabou@mozilla.com
push dateWed, 27 Mar 2019 05:03:16 +0000
treeherdermozilla-inbound@528eacf3d3f6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmanishearth, jwatt
bugs1536718
milestone68.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
Bug 1536718 - Honor browser.display.use_document_fonts again. r=manishearth,jwatt Differential Revision: https://phabricator.services.mozilla.com/D24218
layout/base/nsLayoutUtils.cpp
layout/base/nsLayoutUtils.h
layout/style/GeckoBindings.cpp
layout/style/GeckoBindings.h
layout/style/test/mochitest.ini
layout/style/test/test_dont_use_document_fonts.html
servo/components/style/properties/cascade.rs
--- a/layout/base/nsLayoutUtils.cpp
+++ b/layout/base/nsLayoutUtils.cpp
@@ -9736,44 +9736,16 @@ already_AddRefed<nsFontMetrics> nsLayout
   params.userFontSet =
       aUseUserFontSet ? aPresContext->GetUserFontSet() : nullptr;
   params.textPerf = aPresContext->GetTextPerfMetrics();
   params.featureValueLookup = aPresContext->GetFontFeatureValuesLookup();
   return aPresContext->DeviceContext()->GetMetricsFor(font, params);
 }
 
 /* static */
-void nsLayoutUtils::FixupNoneGeneric(nsFont* aFont, uint8_t aGenericFontID,
-                                     const nsFont* aDefaultVariableFont) {
-  bool useDocumentFonts = StaticPrefs::browser_display_use_document_fonts();
-  if (aGenericFontID == kGenericFont_NONE ||
-      (!useDocumentFonts && (aGenericFontID == kGenericFont_cursive ||
-                             aGenericFontID == kGenericFont_fantasy))) {
-    FontFamilyType defaultGeneric =
-        aDefaultVariableFont->fontlist.GetDefaultFontType();
-    MOZ_ASSERT(aDefaultVariableFont->fontlist.IsEmpty() &&
-               (defaultGeneric == eFamily_serif ||
-                defaultGeneric == eFamily_sans_serif));
-    if (defaultGeneric != eFamily_none) {
-      if (useDocumentFonts) {
-        aFont->fontlist.SetDefaultFontType(defaultGeneric);
-      } else {
-        // Either prioritize the first generic in the list,
-        // or (if there isn't one) prepend the default variable font.
-        if (!aFont->fontlist.PrioritizeFirstGeneric()) {
-          aFont->fontlist.PrependGeneric(defaultGeneric);
-        }
-      }
-    }
-  } else {
-    aFont->fontlist.SetDefaultFontType(eFamily_none);
-  }
-}
-
-/* static */
 void nsLayoutUtils::ComputeSystemFont(nsFont* aSystemFont,
                                       LookAndFeel::FontID aFontID,
                                       const nsFont* aDefaultVariableFont) {
   gfxFontStyle fontStyle;
   nsAutoString systemFontName;
   if (LookAndFeel::GetFont(aFontID, systemFontName, fontStyle)) {
     systemFontName.Trim("\"'");
     aSystemFont->fontlist =
--- a/layout/base/nsLayoutUtils.h
+++ b/layout/base/nsLayoutUtils.h
@@ -2983,23 +2983,16 @@ class nsLayoutUtils {
   static uint8_t ControlCharVisibilityDefault();
 
   // Callers are responsible to ensure the user-font-set is up-to-date if
   // aUseUserFontSet is true.
   static already_AddRefed<nsFontMetrics> GetMetricsFor(
       nsPresContext* aPresContext, bool aIsVertical,
       const nsStyleFont* aStyleFont, nscoord aFontSize, bool aUseUserFontSet);
 
-  /**
-   * Appropriately add the correct font if we are using DocumentFonts or
-   * overriding for XUL
-   */
-  static void FixupNoneGeneric(nsFont* aFont, uint8_t aGenericFontID,
-                               const nsFont* aDefaultVariableFont);
-
   static void ComputeSystemFont(nsFont* aSystemFont,
                                 mozilla::LookAndFeel::FontID aFontID,
                                 const nsFont* aDefaultVariableFont);
 
   static void ComputeFontFeatures(const nsCSSValuePairList* aFeaturesList,
                                   nsTArray<gfxFontFeature>& aFeatureSettings);
 
   static void ComputeFontVariations(
--- a/layout/style/GeckoBindings.cpp
+++ b/layout/style/GeckoBindings.cpp
@@ -1882,22 +1882,23 @@ void Gecko_nsStyleFont_SetLang(nsStyleFo
   aFont->mExplicitLanguage = true;
 }
 
 void Gecko_nsStyleFont_CopyLangFrom(nsStyleFont* aFont,
                                     const nsStyleFont* aSource) {
   aFont->mLanguage = aSource->mLanguage;
 }
 
-void Gecko_nsStyleFont_FixupNoneGeneric(nsStyleFont* aFont,
-                                        const Document* aDocument) {
-  const nsFont* defaultVariableFont = ThreadSafeGetDefaultFontHelper(
-      *aDocument, aFont->mLanguage, kPresContext_DefaultVariableFont_ID);
-  nsLayoutUtils::FixupNoneGeneric(&aFont->mFont, aFont->mGenericID,
-                                  defaultVariableFont);
+void Gecko_nsStyleFont_PrioritizeUserFonts(nsStyleFont* aFont,
+                                           FontFamilyType aDefaultGeneric) {
+  MOZ_ASSERT(!StaticPrefs::browser_display_use_document_fonts());
+  MOZ_ASSERT(aDefaultGeneric != eFamily_none);
+  if (!aFont->mFont.fontlist.PrioritizeFirstGeneric()) {
+    aFont->mFont.fontlist.PrependGeneric(aDefaultGeneric);
+  }
 }
 
 void Gecko_nsStyleFont_PrefillDefaultForGeneric(nsStyleFont* aFont,
                                                 const Document* aDocument,
                                                 uint8_t aGenericId) {
   const nsFont* defaultFont =
       ThreadSafeGetDefaultFontHelper(*aDocument, aFont->mLanguage, aGenericId);
   // In case of just the language changing, the parent could have had no
--- a/layout/style/GeckoBindings.h
+++ b/layout/style/GeckoBindings.h
@@ -667,18 +667,23 @@ float Gecko_FontWeight_ToFloat(mozilla::
 
 void Gecko_FontWeight_SetFloat(mozilla::FontWeight* aWeight, float aFloatValue);
 
 void Gecko_nsStyleFont_SetLang(nsStyleFont* font, nsAtom* atom);
 
 void Gecko_nsStyleFont_CopyLangFrom(nsStyleFont* aFont,
                                     const nsStyleFont* aSource);
 
-void Gecko_nsStyleFont_FixupNoneGeneric(nsStyleFont* font,
-                                        const mozilla::dom::Document*);
+// Moves the generic family in the font-family to the front, or prepends
+// aDefaultGeneric, so that user-configured fonts take precedent over document
+// fonts.
+//
+// Document fonts may still be used as fallback for unsupported glyphs though.
+void Gecko_nsStyleFont_PrioritizeUserFonts(
+    nsStyleFont* font, mozilla::FontFamilyType aDefaultGeneric);
 
 void Gecko_nsStyleFont_PrefillDefaultForGeneric(nsStyleFont* font,
                                                 const mozilla::dom::Document*,
                                                 uint8_t generic_id);
 
 nscoord Gecko_nsStyleFont_ComputeMinSize(const nsStyleFont*,
                                          const mozilla::dom::Document*);
 
--- a/layout/style/test/mochitest.ini
+++ b/layout/style/test/mochitest.ini
@@ -205,16 +205,17 @@ skip-if = toolkit == 'android' #bug 5366
 [test_css_supports.html]
 [test_css_supports_variables.html]
 [test_custom_content_inheritance.html]
 [test_default_bidi_css.html]
 [test_default_computed_style.html]
 [test_descriptor_storage.html]
 [test_descriptor_syntax_errors.html]
 [test_dont_use_document_colors.html]
+[test_dont_use_document_fonts.html]
 [test_dynamic_change_causing_reflow.html]
 [test_exposed_prop_accessors.html]
 [test_extra_inherit_initial.html]
 [test_flexbox_child_display_values.xhtml]
 [test_flexbox_flex_grow_and_shrink.html]
 [test_flexbox_flex_shorthand.html]
 [test_flexbox_focus_order.html]
 [test_flexbox_layout.html]
new file mode 100644
--- /dev/null
+++ b/layout/style/test/test_dont_use_document_fonts.html
@@ -0,0 +1,64 @@
+<!doctype html>
+<title>Test for preference to not use document fonts</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<link rel='stylesheet' href='/resources/testharness.css'>
+<div id="content"></div>
+<script>
+const content = document.getElementById("content");
+
+setup({explicit_done: true })
+
+content.style.fontFamily = "initial";
+const kInitialFamily = getComputedStyle(content).fontFamily;
+content.style.fontFamily = "";
+
+const kTests = [
+  {
+    specified: "monospace",
+    computed: "monospace",
+    description: "Single generic family should not be changed",
+  },
+  {
+    specified: "monospace, sans-serif",
+    computed: "monospace, sans-serif",
+    description: "Generic families should not be changed",
+  },
+  {
+    specified: "Courier, monospace",
+    computed: "monospace, Courier",
+    description: "Generics are preferred, but may still fall back to document fonts",
+  },
+  {
+    specified: "Courier, something-else",
+    computed: `${kInitialFamily}, Courier, something-else`,
+    description: "Generic is prepended to the font-family if none is found",
+  },
+];
+
+// compute expectations while the pref is not active yet.
+test(function() {
+  for (const test of kTests) {
+    content.style.fontFamily = "";
+    content.style.fontFamily = test.computed;
+    assert_not_equals(content.style.fontFamily, "", `computed font ${test.computed} was invalid`);
+    test.expected = getComputedStyle(content).fontFamily;
+  }
+  content.style.font = "";
+}, "Sanity");
+
+function runTest({ specified, computed, description, expected }) {
+  test(function() {
+    content.style.fontFamily = "";
+    content.style.fontFamily = specified;
+    assert_equals(getComputedStyle(content).fontFamily, expected);
+  }, description);
+}
+
+(async function() {
+  await SpecialPowers.pushPrefEnv({'set': [['browser.display.use_document_fonts', 0]]});
+  for (const test of kTests)
+    runTest(test);
+  done();
+})();
+</script>
--- a/servo/components/style/properties/cascade.rs
+++ b/servo/components/style/properties/cascade.rs
@@ -668,53 +668,80 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
             Some(style) => style,
             None => return false,
         };
 
         self.context.builder.copy_reset_from(cached_style);
         true
     }
 
-    // The default font type (which is stored in FontFamilyList's
-    // `mDefaultFontType`) depends on the current lang group, so
-    // we may need to recompute it if it changed.
+    /// The default font type (which is stored in FontFamilyList's
+    /// `mDefaultFontType`) depends on the current lang group and generic font
+    /// family, so we may need to recompute it if or the family changed.
+    ///
+    /// Also, we prioritize non-document fonts here if we need to (see the pref
+    /// `browser.display.use_document_fonts`).
     #[inline]
     #[cfg(feature = "gecko")]
     fn recompute_default_font_family_type_if_needed(&mut self) {
         use crate::gecko_bindings::{bindings, structs};
 
         if !self.seen.contains(LonghandId::XLang) &&
            !self.seen.contains(LonghandId::FontFamily) {
             return;
         }
 
+        let use_document_fonts = unsafe { structs::StaticPrefs_sVarCache_browser_display_use_document_fonts != 0 };
         let builder = &mut self.context.builder;
-        let default_font_type = {
+        let (default_font_type, prioritize_user_fonts) = {
             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 {
+            // System fonts are all right, and should have the default font type
+            // set to none already, so bail out early.
+            if font.mFont.systemFont {
+                debug_assert_eq!(font.mFont.fontlist.mDefaultFontType, structs::FontFamilyType::eFamily_none);
                 return;
             }
 
-            default_font_type
+            let default_font_type = unsafe {
+                bindings::Gecko_nsStyleFont_ComputeDefaultFontType(
+                    builder.device.document(),
+                    font.mGenericID,
+                    font.mLanguage.mRawPtr,
+                )
+            };
+
+            // We prioritize user fonts over document fonts if the pref is set,
+            // and we don't have a generic family already (or we're using
+            // cursive or fantasy, since they're ignored, see bug 789788), and
+            // we have a generic family to actually replace it with.
+            let prioritize_user_fonts =
+                !use_document_fonts &&
+                matches!(
+                    font.mGenericID,
+                    structs::kGenericFont_NONE |
+                    structs::kGenericFont_fantasy |
+                    structs::kGenericFont_cursive
+                ) &&
+                default_font_type != structs::FontFamilyType::eFamily_none;
+
+            if !prioritize_user_fonts && default_font_type == font.mFont.fontlist.mDefaultFontType {
+                // Nothing to do.
+                return;
+            }
+            (default_font_type, prioritize_user_fonts)
         };
 
-        builder.mutate_font().gecko_mut().mFont.fontlist.mDefaultFontType =
-            default_font_type;
+        let font = builder.mutate_font().gecko_mut();
+        font.mFont.fontlist.mDefaultFontType = default_font_type;
+        if prioritize_user_fonts {
+            unsafe {
+                bindings::Gecko_nsStyleFont_PrioritizeUserFonts(font, 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;