Bug 1519371 - Reframe less as a result of UpdateContainingBlock. r=dholbert
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 17 Jan 2019 21:00:34 +0000
changeset 454391 9aa3456410843b6ae80f75c6d668c96dfd31b5fd
parent 454390 f365b9fa9697d6700adf1ea5db66f541001a78f8
child 454392 1d2ae392021c5fabbd8a19290e965cafc7c09dec
push id76340
push userealvarez@mozilla.com
push dateFri, 18 Jan 2019 00:56:20 +0000
treeherderautoland@9aa345641084 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs1519371
milestone66.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 1519371 - Reframe less as a result of UpdateContainingBlock. r=dholbert This patch should make the detection of whether we should reframe in UpdateContainingBlock exact. It should have no behavior change, but sometimes reframing can confuse event handling code or what not. We don't have a reduced test-case for the event handling regression this fixes, but I added a test to ensure we don't uselessly reframe in this case that fails without this patch and passes with. Differential Revision: https://phabricator.services.mozilla.com/D16333
layout/base/RestyleManager.cpp
layout/style/test/mochitest.ini
layout/style/test/test_reframe_cb.html
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@@ -902,77 +902,117 @@ static bool HasBoxAncestor(nsIFrame* aFr
       return true;
     }
   }
   return false;
 }
 
 /**
  * Return true if aFrame's subtree has placeholders for out-of-flow content
- * whose 'position' style's bit in aPositionMask is set.
+ * whose 'position' style's bit in aPositionMask is set that would be affected
+ * due to the change to `aPossiblyChangingContainingBlock` (and thus would need
+ * to get reframed).
+ *
+ * In particular, this function returns true if there are placeholders whose OOF
+ * frames may need to be reparented (via reframing) as a result of whatever
+ * change actually happened.
+ *
+ * `aIsContainingBlock` represents whether `aPossiblyChangingContainingBlock` is
+ * a containing block with the _new_ style it just got, for any of the sorts of
+ * positioned descendants in aPositionMask.
  */
-static bool FrameHasPositionedPlaceholderDescendants(nsIFrame* aFrame,
-                                                     uint32_t aPositionMask) {
+static bool ContainingBlockChangeAffectsDescendants(
+    nsIFrame* aPossiblyChangingContainingBlock, nsIFrame* aFrame,
+    uint32_t aPositionMask, bool aIsContainingBlock) {
   MOZ_ASSERT(aPositionMask & (1 << NS_STYLE_POSITION_FIXED));
 
   for (nsIFrame::ChildListIterator lists(aFrame); !lists.IsDone();
        lists.Next()) {
     for (nsIFrame* f : lists.CurrentList()) {
       if (f->IsPlaceholderFrame()) {
         nsIFrame* outOfFlow = nsPlaceholderFrame::GetRealFrameForPlaceholder(f);
         // If SVG text frames could appear here, they could confuse us since
         // they ignore their position style ... but they can't.
         NS_ASSERTION(!nsSVGUtils::IsInSVGTextSubtree(outOfFlow),
                      "SVG text frames can't be out of flow");
         if (aPositionMask & (1 << outOfFlow->StyleDisplay()->mPosition)) {
-          return true;
+          // NOTE(emilio): aPossiblyChangingContainingBlock is guaranteed to be
+          // a first continuation, see the assertion in the caller.
+          nsIFrame* parent = outOfFlow->GetParent()->FirstContinuation();
+          if (aIsContainingBlock) {
+            // If we are becoming a containing block, we only need to reframe if
+            // this oof's current containing block is an ancestor of the new
+            // frame.
+            if (parent != aPossiblyChangingContainingBlock &&
+                nsLayoutUtils::IsProperAncestorFrame(
+                    parent, aPossiblyChangingContainingBlock)) {
+              return true;
+            }
+          } else {
+            // If we are not a containing block anymore, we only need to reframe
+            // if we are the current containing block of the oof frame.
+            if (parent == aPossiblyChangingContainingBlock) {
+              return true;
+            }
+          }
         }
       }
-      uint32_t positionMask = aPositionMask;
       // NOTE:  It's tempting to check f->IsAbsPosContainingBlock() or
       // f->IsFixedPosContainingBlock() here.  However, that would only
       // be testing the *new* style of the frame, which might exclude
       // descendants that currently have this frame as an abs-pos
       // containing block.  Taking the codepath where we don't reframe
       // could lead to an unsafe call to
       // cont->MarkAsNotAbsoluteContainingBlock() before we've reframed
       // the descendant and taken it off the absolute list.
-      if (FrameHasPositionedPlaceholderDescendants(f, positionMask)) {
+      if (ContainingBlockChangeAffectsDescendants(
+              aPossiblyChangingContainingBlock, f, aPositionMask,
+              aIsContainingBlock)) {
         return true;
       }
     }
   }
   return false;
 }
 
-static bool NeedToReframeForAddingOrRemovingTransform(nsIFrame* aFrame) {
+static bool NeedToReframeToUpdateContainingBlock(nsIFrame* aFrame) {
   static_assert(
       0 <= NS_STYLE_POSITION_ABSOLUTE && NS_STYLE_POSITION_ABSOLUTE < 32,
       "Style constant out of range");
   static_assert(0 <= NS_STYLE_POSITION_FIXED && NS_STYLE_POSITION_FIXED < 32,
                 "Style constant out of range");
 
   uint32_t positionMask;
+  bool isContainingBlock;
   // Don't call aFrame->IsPositioned here, since that returns true if
   // the frame already has a transform, and we want to ignore that here
   if (aFrame->IsAbsolutelyPositioned() || aFrame->IsRelativelyPositioned()) {
-    // This frame is a container for abs-pos descendants whether or not it
-    // has a transform.
+    // This frame is a container for abs-pos descendants whether or not its
+    // containing-block-ness could change for some other reasons (since we
+    // reframe for position changes).
     // So abs-pos descendants are no problem; we only need to reframe if
     // we have fixed-pos descendants.
     positionMask = 1 << NS_STYLE_POSITION_FIXED;
+    isContainingBlock = aFrame->IsFixedPosContainingBlock();
   } else {
     // This frame may not be a container for abs-pos descendants already.
     // So reframe if we have abs-pos or fixed-pos descendants.
     positionMask =
         (1 << NS_STYLE_POSITION_FIXED) | (1 << NS_STYLE_POSITION_ABSOLUTE);
+    isContainingBlock = aFrame->IsAbsPosContainingBlock() ||
+                        aFrame->IsFixedPosContainingBlock();
   }
+
+  MOZ_ASSERT(!aFrame->GetPrevContinuation(),
+             "We only process change hints on first continuations");
+
   for (nsIFrame* f = aFrame; f;
        f = nsLayoutUtils::GetNextContinuationOrIBSplitSibling(f)) {
-    if (FrameHasPositionedPlaceholderDescendants(f, positionMask)) {
+    if (ContainingBlockChangeAffectsDescendants(aFrame, f, positionMask,
+                                                isContainingBlock)) {
       return true;
     }
   }
   return false;
 }
 
 static void DoApplyRenderingChangeToTree(nsIFrame* aFrame,
                                          nsChangeHint aChange) {
@@ -1440,17 +1480,17 @@ void RestyleManager::ProcessRestyledFram
       frame = nullptr;
       if (!(hint & nsChangeHint_ReconstructFrame)) {
         continue;
       }
     }
 
     if ((hint & nsChangeHint_UpdateContainingBlock) && frame &&
         !(hint & nsChangeHint_ReconstructFrame)) {
-      if (NeedToReframeForAddingOrRemovingTransform(frame) ||
+      if (NeedToReframeToUpdateContainingBlock(frame) ||
           frame->IsFieldSetFrame() ||
           frame->GetContentInsertionFrame() != frame) {
         // The frame has positioned children that need to be reparented, or
         // it can't easily be converted to/from being an abs-pos container
         // correctly.
         hint |= nsChangeHint_ReconstructFrame;
       } else {
         for (nsIFrame* cont = frame; cont;
--- a/layout/style/test/mochitest.ini
+++ b/layout/style/test/mochitest.ini
@@ -288,16 +288,17 @@ run-if = (os == 'mac' || toolkit == 'and
 [test_property_database.html]
 [test_property_syntax_errors.html]
 [test_pseudo_display_fixup.html]
 [test_pseudoelement_state.html]
 skip-if = (verify && debug && (os == 'linux'))
 [test_pseudoelement_parsing.html]
 [test_redundant_font_download.html]
 support-files = redundant_font_download.sjs
+[test_reframe_cb.html]
 [test_reframe_pseudo_element.html]
 [test_rem_unit.html]
 [test_restyle_table_wrapper.html]
 [test_restyles_in_smil_animation.html]
 skip-if = toolkit == 'android' # bug 1328522
 [test_root_node_display.html]
 [test_rule_insertion.html]
 [test_rule_serialization.html]
new file mode 100644
--- /dev/null
+++ b/layout/style/test/test_reframe_cb.html
@@ -0,0 +1,56 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>
+  Test for bug 1519371: We don't reframe for changes that affect our containing
+  block status unless whether we're a containing block really changed.
+</title>
+<link rel="author" href="mailto:emilio@crisal.io" title="Emilio Cobos Álvarez">
+<script src="/tests/SimpleTest/SimpleTest.js"></script>
+<div id="content"></div>
+<script>
+SimpleTest.waitForExplicitFinish();
+const utils = SpecialPowers.getDOMWindowUtils(window);
+
+function expectReframe(shouldHaveReframed, callback) {
+  document.documentElement.offsetTop;
+  const previousConstructCount = utils.framesConstructed;
+  const previousRestyleGeneration = utils.restyleGeneration;
+
+  callback();
+
+  document.documentElement.offsetTop;
+  isnot(previousRestyleGeneration, utils.restyleGeneration,
+        "We should have restyled");
+  (shouldHaveReframed ? isnot : is)(previousConstructCount,
+                                    utils.framesConstructed,
+    `We should ${shouldHaveReframed ? "" : "not"} have reframed`);
+}
+
+const content = document.getElementById("content");
+
+// Without fixed-pos descendants, we should not reframe.
+expectReframe(false, () => {
+  content.style.transform = "scale(1)";
+});
+
+content.style.transform = "";
+content.innerHTML = `<div style="position: fixed"></div>`;
+
+// With fixed-pos descendants, got to reframe.
+expectReframe(true, () => {
+  content.style.transform = "scale(1)";
+});
+
+// If our containing-block-ness didn't change, we should not need to reframe.
+expectReframe(false, () => {
+  content.style.willChange = "transform";
+});
+
+// If it does change, we need to reframe.
+expectReframe(true, () => {
+  content.style.willChange = "";
+  content.style.transform = "";
+});
+
+SimpleTest.finish();
+</script>