Bug 1611462 - Optimize position changes better. r=dholbert

When our position changes from / to absolute / fixed, then we need to reframe
(most likely, at least). But in some easier cases we can just live with a reflow
plus (optionally) a containing-block update or a repaint, depending on the case.

We need to delete the normal position property when this happens and we switch
from static to relative/sticky, and also need to update the handling of
UpdateContainingBlock to avoid making decisions based on position not changing.

This will actually not cause more reframes, as one would expect, because that
optimization became somewhat obsolete by bug 1519371 (which made the
optimization exact).

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

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

layout/base/RestyleManager.cpp

@@ -904,22 +904,23 @@ static bool HasBoxAncestor(nsIFrame* aFrame) {
 
 /**
  * Return true if aFrame's subtree has placeholders for out-of-flow content
- * 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).
+ * 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.
+ * The `aIs{Abs,Fixed}PosContainingBlock` params represent whether
+ * `aPossiblyChangingContainingBlock` is a containing block for abs pos / fixed
+ * pos stuff, respectively, for the _new_ style that the frame already has, not
+ * the old one.
  */
 static bool ContainingBlockChangeAffectsDescendants(
     nsIFrame* aPossiblyChangingContainingBlock, nsIFrame* aFrame,
-    uint32_t aPositionMask, bool aIsContainingBlock) {
-  MOZ_ASSERT(aPositionMask & (1 << NS_STYLE_POSITION_FIXED));
+    bool aIsAbsPosContainingBlock, bool aIsFixedPosContainingBlock) {
+  // All fixed-pos containing blocks should also be abs-pos containing blocks.
+  MOZ_ASSERT_IF(aIsFixedPosContainingBlock, aIsAbsPosContainingBlock);
 
   for (nsIFrame::ChildListIterator lists(aFrame); !lists.IsDone();
        lists.Next()) {
@@ -930,11 +931,16 @@ static bool ContainingBlockChangeAffectsDescendants(
         // 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)) {
+        auto* display = outOfFlow->StyleDisplay();
+        if (display->IsAbsolutelyPositionedStyle()) {
+          const bool isContainingBlock =
+              aIsFixedPosContainingBlock ||
+              (aIsAbsPosContainingBlock &&
+               display->mPosition == NS_STYLE_POSITION_ABSOLUTE);
           // NOTE(emilio): aPossiblyChangingContainingBlock is guaranteed to be
           // a first continuation, see the assertion in the caller.
           nsIFrame* parent = outOfFlow->GetParent()->FirstContinuation();
-          if (aIsContainingBlock) {
+          if (isContainingBlock) {
             // 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.
@@ -961,8 +967,8 @@ static bool ContainingBlockChangeAffectsDescendants(
       // cont->MarkAsNotAbsoluteContainingBlock() before we've reframed
       // the descendant and taken it off the absolute list.
       if (ContainingBlockChangeAffectsDescendants(
-              aPossiblyChangingContainingBlock, f, aPositionMask,
-              aIsContainingBlock)) {
+              aPossiblyChangingContainingBlock, f, aIsAbsPosContainingBlock,
+              aIsFixedPosContainingBlock)) {
         return true;
       }
     }
@@ -971,40 +977,17 @@ static bool ContainingBlockChangeAffectsDescendants(
 }
 
 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");
+  // NOTE: This looks at the new style.
+  const bool isFixedContainingBlock = aFrame->IsFixedPosContainingBlock();
+  MOZ_ASSERT_IF(isFixedContainingBlock, aFrame->IsAbsPosContainingBlock());
 
-  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 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");
+  const bool isAbsPosContainingBlock =
+      isFixedContainingBlock || aFrame->IsAbsPosContainingBlock();
 
   for (nsIFrame* f = aFrame; f;
        f = nsLayoutUtils::GetNextContinuationOrIBSplitSibling(f)) {
-    if (ContainingBlockChangeAffectsDescendants(aFrame, f, positionMask,
-                                                isContainingBlock)) {
+    if (ContainingBlockChangeAffectsDescendants(
+            aFrame, f, isAbsPosContainingBlock, isFixedContainingBlock)) {
       return true;
     }
   }

layout/generic/nsFrame.cpp

@@ -710,23 +710,10 @@ void nsFrame::Init(nsIContent* aContent, nsContainerFrame* aParent,
 
   const nsStyleDisplay* disp = StyleDisplay();
   if (disp->HasTransform(this)) {
-    // The frame gets reconstructed if we toggle the transform
-    // property, so we can set this bit here and then ignore it.
+    // If 'transform' dynamically changes, RestyleManager takes care of
+    // updating this bit.
     AddStateBits(NS_FRAME_MAY_BE_TRANSFORMED);
   }
-  if (disp->mPosition == NS_STYLE_POSITION_STICKY && !aPrevInFlow &&
-      !(mState & NS_FRAME_IS_NONDISPLAY)) {
-    // Note that we only add first continuations, but we really only
-    // want to add first continuation-or-ib-split-siblings.  But since we
-    // don't yet know if we're a later part of a block-in-inline split,
-    // we'll just add later members of a block-in-inline split here, and
-    // then StickyScrollContainer will remove them later.
-    StickyScrollContainer* ssc =
-        StickyScrollContainer::GetStickyScrollContainerForFrame(this);
-    if (ssc) {
-      ssc->AddFrame(this);
-    }
-  }
 
   if (disp->IsContainLayout() && disp->IsContainSize() &&
       // All frames that support contain:layout also support contain:size.
@@ -1202,6 +1189,8 @@ void nsFrame::DidSetComputedStyle(ComputedStyle* aOldComputedStyle) {
   newLayers = &StyleSVGReset()->mMask;
   AddAndRemoveImageAssociations(*imageLoader, this, oldLayers, newLayers);
 
+  const nsStyleDisplay* disp = StyleDisplay();
+  bool handleStickyChange = false;
   if (aOldComputedStyle) {
     // Detect style changes that should trigger a scroll anchor adjustment
     // suppression.
@@ -1243,7 +1232,7 @@ void nsFrame::DidSetComputedStyle(ComputedStyle* aOldComputedStyle) {
     }
 
     const nsStyleDisplay* oldDisp = aOldComputedStyle->StyleDisplay();
-    if (oldDisp->mOverflowAnchor != StyleDisplay()->mOverflowAnchor) {
+    if (oldDisp->mOverflowAnchor != disp->mOverflowAnchor) {
       if (auto* container = ScrollAnchorContainer::FindFor(this)) {
         container->InvalidateAnchor();
       }
@@ -1253,17 +1242,17 @@ void nsFrame::DidSetComputedStyle(ComputedStyle* aOldComputedStyle) {
     }
 
     if (mInScrollAnchorChain) {
-      const nsStylePosition* oldPosition = aOldComputedStyle->StylePosition();
+      const nsStylePosition* pos = StylePosition();
+      const nsStylePosition* oldPos = aOldComputedStyle->StylePosition();
       if (!needAnchorSuppression &&
-          (oldPosition->mOffset != StylePosition()->mOffset ||
-           oldPosition->mWidth != StylePosition()->mWidth ||
-           oldPosition->mMinWidth != StylePosition()->mMinWidth ||
-           oldPosition->mMaxWidth != StylePosition()->mMaxWidth ||
-           oldPosition->mHeight != StylePosition()->mHeight ||
-           oldPosition->mMinHeight != StylePosition()->mMinHeight ||
-           oldPosition->mMaxHeight != StylePosition()->mMaxHeight ||
-           oldDisp->mPosition != StyleDisplay()->mPosition ||
-           oldDisp->mTransform != StyleDisplay()->mTransform)) {
+          (oldPos->mOffset != pos->mOffset || oldPos->mWidth != pos->mWidth ||
+           oldPos->mMinWidth != pos->mMinWidth ||
+           oldPos->mMaxWidth != pos->mMaxWidth ||
+           oldPos->mHeight != pos->mHeight ||
+           oldPos->mMinHeight != pos->mMinHeight ||
+           oldPos->mMaxHeight != pos->mMaxHeight ||
+           oldDisp->mPosition != disp->mPosition ||
+           oldDisp->mTransform != disp->mTransform)) {
         needAnchorSuppression = true;
       }
 
@@ -1272,6 +1261,35 @@ void nsFrame::DidSetComputedStyle(ComputedStyle* aOldComputedStyle) {
         ScrollAnchorContainer::FindFor(this)->SuppressAdjustments();
       }
     }
+
+    if (disp->mPosition != oldDisp->mPosition) {
+      if (!disp->IsRelativelyPositionedStyle() &&
+          oldDisp->IsRelativelyPositionedStyle()) {
+        RemoveProperty(NormalPositionProperty());
+      }
+
+      handleStickyChange = disp->mPosition == NS_STYLE_POSITION_STICKY ||
+                           oldDisp->mPosition == NS_STYLE_POSITION_STICKY;
+    }
+  } else {  // !aOldComputedStyle
+    handleStickyChange = disp->mPosition == NS_STYLE_POSITION_STICKY;
+  }
+
+  if (handleStickyChange && !HasAnyStateBits(NS_FRAME_IS_NONDISPLAY) &&
+      !GetPrevInFlow()) {
+    // Note that we only add first continuations, but we really only
+    // want to add first continuation-or-ib-split-siblings. But since we don't
+    // yet know if we're a later part of a block-in-inline split, we'll just
+    // add later members of a block-in-inline split here, and then
+    // StickyScrollContainer will remove them later.
+    if (auto* ssc =
+            StickyScrollContainer::GetStickyScrollContainerForFrame(this)) {
+      if (disp->mPosition == NS_STYLE_POSITION_STICKY) {
+        ssc->AddFrame(this);
+      } else {
+        ssc->RemoveFrame(this);
+      }
+    }
   }
 
   imgIRequest* oldBorderImage =

layout/style/nsStyleStruct.cpp

@@ -2852,10 +2852,7 @@ static bool ScrollbarGenerationChanged(const nsStyleDisplay& aOld,
 
 nsChangeHint nsStyleDisplay::CalcDifference(
     const nsStyleDisplay& aNewData, const nsStylePosition& aOldPosition) const {
-  nsChangeHint hint = nsChangeHint(0);
-
-  if (mPosition != aNewData.mPosition || mDisplay != aNewData.mDisplay ||
-      mContain != aNewData.mContain ||
+  if (mDisplay != aNewData.mDisplay || mContain != aNewData.mContain ||
       (mFloat == StyleFloat::None) != (aNewData.mFloat == StyleFloat::None) ||
       mScrollBehavior != aNewData.mScrollBehavior ||
       mScrollSnapType != aNewData.mScrollSnapType ||
@@ -2876,6 +2873,35 @@ nsChangeHint nsStyleDisplay::CalcDifference(
     return nsChangeHint_ReconstructFrame;
   }
 
+  auto hint = nsChangeHint(0);
+  if (mPosition != aNewData.mPosition) {
+    if (IsAbsolutelyPositionedStyle() ||
+        aNewData.IsAbsolutelyPositionedStyle()) {
+      // This changes our parent relationship on the frame tree and / or needs
+      // to create a placeholder, so gotta reframe. There are some cases (when
+      // switching from fixed to absolute or viceversa, if our containing block
+      // happens to remain the same, i.e., if it has a transform or such) where
+      // this wouldn't really be needed (though we'd still need to move the
+      // frame from one child list to another). In any case we don't have a hand
+      // to that information from here, and it doesn't seem like a case worth
+      // optimizing for.
+      return nsChangeHint_ReconstructFrame;
+    }
+    // We start or stop being a containing block for abspos descendants. This
+    // also causes painting to change, as we'd become a pseudo-stacking context.
+    if (IsRelativelyPositionedStyle() !=
+        aNewData.IsRelativelyPositionedStyle()) {
+      hint |= nsChangeHint_UpdateContainingBlock | nsChangeHint_RepaintFrame;
+    }
+    if (IsPositionForcingStackingContext() !=
+        aNewData.IsPositionForcingStackingContext()) {
+      hint |= nsChangeHint_RepaintFrame;
+    }
+    // On top of that: if the above ends up not reframing, we need a reflow to
+    // compute our relative, static or sticky position.
+    hint |= nsChangeHint_NeedReflow | nsChangeHint_ReflowChangesSizeOrPosition;
+  }
+
   if (mScrollSnapAlign != aNewData.mScrollSnapAlign) {
     // FIXME: Bug 1530253 Support re-snapping when scroll-snap-align changes.
     hint |= nsChangeHint_NeutralChange;

layout/style/test/test_dynamic_change_causing_reflow.html

@@ -14,6 +14,8 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1131371
 <div id="display">
   <div id="content">
   </div>
+  <div id="elemWithAbsPosChild"><div style="position:absolute"></div></div>
+  <div id="elemWithFixedPosChild"><div style="position:fixed"></div></div>
 </div>
 <pre id="test">
 <script type="application/javascript">
@@ -21,6 +23,9 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1131371
 
 /** Test for Bug 1131371 **/
 
+const elemWithAbsPosChild = document.getElementById("elemWithAbsPosChild");
+const elemWithFixedPosChild = document.getElementById("elemWithFixedPosChild");
+
 /**
  * This test verifies that certain style changes do or don't cause reflow
  * and/or frame construction. We do this by checking the framesReflowed &
@@ -247,6 +252,85 @@ const gTestcases = [
     expectReflow: true,
   },
 
+
+  // * Position changes trigger a reframe, unless whether we're a containing
+  // block doesn't change, in which case we just need to reflow.
+  {
+    beforeStyle: "position: static",
+    afterStyle:  "position: absolute",
+    expectConstruction: true,
+    expectReflow: true,
+  },
+  {
+    beforeStyle:  "position: absolute",
+    afterStyle: "position: fixed",
+    expectConstruction: true,
+    expectReflow: true,
+  },
+  {
+    beforeStyle:  "position: relative",
+    afterStyle: "position: fixed",
+    expectConstruction: true,
+    expectReflow: true,
+  },
+
+  // This doesn't change whether we're a containing block because there are no
+  // abspos descendants.
+  {
+    afterStyle: "position: static",
+    beforeStyle:  "position: relative",
+    expectReflow: true,
+  },
+
+  // This doesn't change whether we're a containing block, shouldn't reframe.
+  {
+    afterStyle: "position: sticky",
+    beforeStyle:  "position: relative",
+    expectReflow: true,
+  },
+
+  // These don't change whether we're a containing block for our
+  // absolutely-positioned child, so shouldn't reframe.
+  {
+    elem: elemWithAbsPosChild,
+    afterStyle: "position: sticky",
+    beforeStyle:  "position: relative",
+    expectReflow: true,
+  },
+  {
+    elem: elemWithFixedPosChild,
+    afterStyle: "position: sticky",
+    beforeStyle:  "position: relative",
+    expectReflow: true,
+  },
+  {
+    elem: elemWithFixedPosChild,
+    afterStyle: "position: static",
+    beforeStyle:  "position: relative",
+    expectReflow: true,
+  },
+  {
+    elem: elemWithFixedPosChild,
+    afterStyle: "position: static",
+    beforeStyle:  "position: sticky",
+    expectReflow: true,
+  },
+
+  // These ones do though.
+  {
+    elem: elemWithAbsPosChild,
+    afterStyle: "position: static",
+    beforeStyle:  "position: relative",
+    expectConstruction: true,
+    expectReflow: true,
+  },
+  {
+    elem: elemWithAbsPosChild,
+    afterStyle: "position: static",
+    beforeStyle:  "position: sticky",
+    expectConstruction: true,
+    expectReflow: true,
+  },
 ];
 
 // Helper function to let us call either "is" or "isnot" & assemble