Bug 1620359 - Don't clear the "uses viewport units" bit when a font that doesn't cause a style change loads. r=jfkthame

This is probably an old-ish bug made more frequent by the font loading
optimizations.

PostRebuildAllStyleData is a bit of a misnomer, but was always calling
ClearCachedData() on the style set, even if we weren't guaranteed to restyle
every element.

This means both wasted work and correctness issues (as the "uses <rare-feature>"
bits are cleared during this call, on the assumption that we'll then visit all
elements and that'd recompute it properly).

For now, unify a bit the different code paths and only clear these bits if we're
guaranteed to restyle all elements.

I should rename this to something better in a follow-up, and ideally also
decouple the ClearCachedData() calls a bit...

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

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

dom/base/Document.cpp

@@ -6033,7 +6033,7 @@ void Document::SetDocumentCharacterSet(NotNull<const Encoding*> aEncoding) {
     RecomputeLanguageFromCharset();
 
     if (nsPresContext* context = GetPresContext()) {
-      context->DispatchCharSetChange(aEncoding);
+      context->DocumentCharSetChanged(aEncoding);
     }
   }
 }

layout/base/RestyleManager.cpp

@@ -2267,18 +2267,21 @@ void RestyleManager::PostRestyleEventForAnimations(Element* aElement,
 
 void RestyleManager::RebuildAllStyleData(nsChangeHint aExtraHint,
                                          RestyleHint aRestyleHint) {
-  // NOTE(emilio): GeckoRestlyeManager does a sync style flush, which seems not
-  // to be needed in my testing.
-  PostRebuildAllStyleDataEvent(aExtraHint, aRestyleHint);
-}
-
-void RestyleManager::PostRebuildAllStyleDataEvent(nsChangeHint aExtraHint,
-                                                  RestyleHint aRestyleHint) {
   // NOTE(emilio): The semantics of these methods are quite funny, in the sense
   // that we're not supposed to need to rebuild the actual stylist data.
   //
   // That's handled as part of the MediumFeaturesChanged stuff, if needed.
-  StyleSet()->ClearCachedStyleData();
+  //
+  // Clear the cached style data only if we are guaranteed to process the whole
+  // DOM tree again.
+  //
+  // FIXME(emilio): Decouple this, probably. This probably just wants to reset
+  // the "uses viewport units / uses rem" bits, and _maybe_ clear cached anon
+  // box styles and such... But it doesn't really always need to clear the
+  // initial style of the document and similar...
+  if (aRestyleHint.DefinitelyRecascadesAllSubtree()) {
+    StyleSet()->ClearCachedStyleData();
+  }
 
   DocumentStyleRootIterator iter(mPresContext->Document());
   while (Element* root = iter.GetNextStyleRoot()) {

layout/base/RestyleManager.h

@@ -358,7 +358,6 @@ class RestyleManager {
   void NextRestyleIsForCSSRuleChanges() { mRestyleForCSSRuleChanges = true; }
 
   void RebuildAllStyleData(nsChangeHint aExtraHint, RestyleHint);
-  void PostRebuildAllStyleDataEvent(nsChangeHint aExtraHint, RestyleHint);
 
   void ProcessPendingRestyles();
   void ProcessAllPendingAttributeAndStateInvalidations();

layout/base/nsPresContext.cpp

@@ -207,7 +207,6 @@ nsPresContext::nsPresContext(dom::Document* aDocument, nsPresContextType aType)
       mPrefChangePendingNeedsReflow(false),
       mPostedPrefChangedRunnable(false),
       mIsGlyph(false),
-      mUsesRootEMUnits(false),
       mUsesExChUnits(false),
       mCounterStylesDirty(true),
       mFontFeatureValuesDirty(true),
@@ -766,7 +765,7 @@ void nsPresContext::DetachPresShell() {
   }
 }
 
-void nsPresContext::DoChangeCharSet(NotNull<const Encoding*> aCharSet) {
+void nsPresContext::DocumentCharSetChanged(NotNull<const Encoding*> aCharSet) {
   UpdateCharSet(aCharSet);
   mDeviceContext->FlushFontCache();
 
@@ -794,11 +793,6 @@ void nsPresContext::UpdateCharSet(NotNull<const Encoding*> aCharSet) {
   }
 }
 
-void nsPresContext::DispatchCharSetChange(NotNull<const Encoding*> aEncoding) {
-  // In Servo RebuildAllStyleData is async, so no need to do the runnable dance.
-  DoChangeCharSet(aEncoding);
-}
-
 nsPresContext* nsPresContext::GetParentPresContext() {
   mozilla::PresShell* presShell = GetPresShell();
   if (presShell) {
@@ -1485,11 +1479,6 @@ void nsPresContext::RebuildAllStyleData(nsChangeHint aExtraHint,
     return;
   }
 
-  // FIXME(emilio): Why is it safe to reset mUsesRootEMUnits / mUsesEXChUnits
-  // here if there's no restyle hint? That looks pretty bogus.
-  mUsesRootEMUnits = false;
-  mUsesExChUnits = false;
-
   // TODO(emilio): It's unclear to me why would these three calls below be
   // needed. In particular, RebuildAllStyleData doesn't rebuild rules or
   // specified style information and such (note the comment in
@@ -1499,8 +1488,7 @@ void nsPresContext::RebuildAllStyleData(nsChangeHint aExtraHint,
   mDocument->MarkUserFontSetDirty();
   MarkCounterStylesDirty();
   MarkFontFeatureValuesDirty();
-
-  RestyleManager()->RebuildAllStyleData(aExtraHint, aRestyleHint);
+  PostRebuildAllStyleDataEvent(aExtraHint, aRestyleHint);
 }
 
 void nsPresContext::PostRebuildAllStyleDataEvent(
@@ -1509,7 +1497,10 @@ void nsPresContext::PostRebuildAllStyleDataEvent(
     // We must have been torn down. Nothing to do here.
     return;
   }
-  RestyleManager()->PostRebuildAllStyleDataEvent(aExtraHint, aRestyleHint);
+  if (aRestyleHint.DefinitelyRecascadesAllSubtree()) {
+    mUsesExChUnits = false;
+  }
+  RestyleManager()->RebuildAllStyleData(aExtraHint, aRestyleHint);
 }
 
 static CallState MediaFeatureValuesChangedAllDocumentsCallback(

layout/base/nsPresContext.h

@@ -175,7 +175,7 @@ class nsPresContext : public nsISupports,
 
   mozilla::PresShell* GetPresShell() const { return mPresShell; }
 
-  void DispatchCharSetChange(NotNull<const Encoding*> aCharSet);
+  void DocumentCharSetChanged(NotNull<const Encoding*> aCharSet);
 
   /**
    * Returns the parent prescontext for this one. Returns null if this is a
@@ -266,11 +266,16 @@ class nsPresContext : public nsISupports,
    *
    * For the restyle hint argument, see RestyleManager::RebuildAllStyleData.
    * Also rebuild the user font set and counter style manager.
+   *
+   * FIXME(emilio): The name of this is an utter lie. We should probably call
+   * this PostGlobalStyleChange or something, as it doesn't really rebuild
+   * anything unless you tell it to via the change hint / restyle hint
+   * machinery.
    */
   void RebuildAllStyleData(nsChangeHint, const mozilla::RestyleHint&);
   /**
    * Just like RebuildAllStyleData, except (1) asynchronous and (2) it
-   * doesn't rebuild the user font set.
+   * doesn't rebuild the user font set / counter-style manager / etc.
    */
   void PostRebuildAllStyleDataEvent(nsChangeHint, const mozilla::RestyleHint&);
 
@@ -1015,10 +1020,6 @@ class nsPresContext : public nsISupports,
   void NotifyContentfulPaint();
   void NotifyDOMContentFlushed();
 
-  bool UsesRootEMUnits() const { return mUsesRootEMUnits; }
-
-  void SetUsesRootEMUnits(bool aValue) { mUsesRootEMUnits = aValue; }
-
   bool UsesExChUnits() const { return mUsesExChUnits; }
 
   void SetUsesExChUnits(bool aValue) { mUsesExChUnits = aValue; }
@@ -1099,8 +1100,6 @@ class nsPresContext : public nsISupports,
   // has been updated, potentially affecting font selection and layout.
   void ForceReflowForFontInfoUpdate();
 
-  void DoChangeCharSet(NotNull<const Encoding*> aCharSet);
-
   /**
    * Checks for MozAfterPaint listeners on the document
    */
@@ -1288,9 +1287,10 @@ class nsPresContext : public nsISupports,
   // Are we currently drawing an SVG glyph?
   unsigned mIsGlyph : 1;
 
-  // Does the associated document use root-em (rem) units?
-  unsigned mUsesRootEMUnits : 1;
   // Does the associated document use ex or ch units?
+  //
+  // TODO(emilio): It's a bit weird that this lives here but all the other
+  // relevant bits live in Device on the rust side.
   unsigned mUsesExChUnits : 1;
 
   // Is the current mCounterStyleManager valid?

layout/style/ServoStyleConstsInlines.h

@@ -907,16 +907,22 @@ inline nsRect StyleGenericClipRect<LengthOrAuto>::ToLayoutRect(
 using RestyleHint = StyleRestyleHint;
 
 inline RestyleHint RestyleHint::RestyleSubtree() {
-  return RestyleHint::RESTYLE_SELF | RestyleHint::RESTYLE_DESCENDANTS;
+  return RESTYLE_SELF | RESTYLE_DESCENDANTS;
 }
 
 inline RestyleHint RestyleHint::RecascadeSubtree() {
-  return RestyleHint::RECASCADE_SELF | RestyleHint::RECASCADE_DESCENDANTS;
+  return RECASCADE_SELF | RECASCADE_DESCENDANTS;
 }
 
 inline RestyleHint RestyleHint::ForAnimations() {
-  return RestyleHint::RESTYLE_CSS_TRANSITIONS |
-         RestyleHint::RESTYLE_CSS_ANIMATIONS | RestyleHint::RESTYLE_SMIL;
+  return RESTYLE_CSS_TRANSITIONS | RESTYLE_CSS_ANIMATIONS | RESTYLE_SMIL;
+}
+
+inline bool RestyleHint::DefinitelyRecascadesAllSubtree() const {
+  if (!(*this & (RECASCADE_DESCENDANTS | RESTYLE_DESCENDANTS))) {
+    return false;
+  }
+  return bool(*this & (RESTYLE_SELF | RECASCADE_SELF));
 }
 
 template <>

servo/ports/geckolib/cbindgen.toml

@@ -431,6 +431,9 @@ renaming_overrides_prefixing = true
   static inline StyleRestyleHint RestyleSubtree();
   static inline StyleRestyleHint RecascadeSubtree();
   static inline StyleRestyleHint ForAnimations();
+  // Returns true if this change hint is guaranteed to at least recascade all
+  // elements in the subtree of the element it is applied to.
+  inline bool DefinitelyRecascadesAllSubtree() const;
 """
 
 "TextTransform" = """

testing/web-platform/tests/css/css-values/viewport-units-after-font-load.html

@@ -0,0 +1,43 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>CSS Values: Viewport units are computed correctly after font load.</title>
+<link rel="author"  title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="author"  title="Mozilla" href="https://mozilla.org">
+<link rel="help" href="https://drafts.csswg.org/css-values-4/#viewport-relative-lengths">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1620359">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<iframe width=300 height=300 scrolling=no srcdoc=""></iframe>
+<script>
+let t = async_test("Viewport units are correctly updated after resize even if a font load has happened before");
+let iframe = document.querySelector("iframe");
+onload = t.step_func(function() {
+  let doc = iframe.contentDocument;
+  let win = iframe.contentWindow;
+  doc.body.innerHTML = `
+    <div style="width: 100vw; height: 100vh; background: green"></div>
+  `;
+  let div = doc.querySelector("div");
+  let oldWidth = win.getComputedStyle(div).width;
+  let oldHeight = win.getComputedStyle(div).height;
+  assert_equals(oldWidth, win.innerWidth + "px", "Should fill the viewport");
+  assert_equals(oldHeight, win.innerHeight + "px", "Should fill the viewport");
+  let link = doc.createElement("link");
+  link.rel = "stylesheet";
+  link.href = "/fonts/ahem.css";
+  link.onload = t.step_func(function() {
+    iframe.width = 400;
+    win.requestAnimationFrame(t.step_func(function() {
+      win.requestAnimationFrame(t.step_func_done(function() {
+        let newWidth = win.getComputedStyle(div).width;
+        let newHeight = win.getComputedStyle(div).height;
+        assert_equals(newWidth, win.innerWidth + "px", "Should fill the viewport");
+        assert_equals(newHeight, win.innerHeight + "px", "Should fill the viewport");
+        assert_equals(newHeight, oldHeight, "Height shouldn't have changed");
+        assert_not_equals(newWidth, oldWidth, "Width should have changed");
+      }));
+    }));
+  });
+  doc.body.appendChild(link);
+});
+</script>