Bug 1519462 - Coalesce all scroll anchor adjustments to be performed after layout when flushing notifcations. r=dholbert
authorRyan Hunt <rhunt@eqrion.net>
Sun, 13 Jan 2019 00:54:05 -0600
changeset 511331 8e7a368e513a746650e08297b50c4d8ec971386c
parent 511330 ee96fa31af70531541004213fe82acbb2fa1943e
child 511332 bd1415025d3e6b5fdf51803201ce06081beffd95
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs1519462
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 1519462 - Coalesce all scroll anchor adjustments to be performed after layout when flushing notifcations. r=dholbert We currently perform anchor adjustment in three spots: 1. If the target of RestyleManager::RecomputePosition is in a scroll anchor chain 2. If the reflow root is in a scroll anchor chain 3. In nsHTMLScrollFrame::DidReflow, for itself It looks like it's possible for a scroll anchor container to be adjusted by (1) and (2 or 3) in the same PresShell flush. This should be okay, except that we consume mSuppressAnchorAdjustment when performing an adjustment, and this can lead us to miss the second time that we perform adjustments in a PresShell flush. This commit reworks how we run anchor adjustments so that we collect all scroll anchor containers that should be adjusted, and only perform the adjustments once. Differential Revision: https://phabricator.services.mozilla.com/D16407
layout/base/PresShell.cpp
layout/base/PresShell.h
layout/base/RestyleManager.cpp
layout/base/nsIPresShell.h
layout/generic/ScrollAnchorContainer.cpp
layout/generic/nsGfxScrollFrame.cpp
testing/web-platform/meta/css/css-scroll-anchoring/anchoring-with-bounds-clamping-div.html.ini
testing/web-platform/tests/css/css-scroll-anchoring/heuristic-with-offset-update.html
--- a/layout/base/PresShell.cpp
+++ b/layout/base/PresShell.cpp
@@ -1306,17 +1306,18 @@ void PresShell::Destroy() {
   mCurrentEventFrame = nullptr;
 
   int32_t i, count = mCurrentEventFrameStack.Length();
   for (i = 0; i < count; i++) {
     mCurrentEventFrameStack[i] = nullptr;
   }
 
   mFramesToDirty.Clear();
-  mDirtyScrollAnchorContainers.Clear();
+  mPendingScrollAnchorSelection.Clear();
+  mPendingScrollAnchorAdjustment.Clear();
 
   if (mViewManager) {
     // Clear the view manager's weak pointer back to |this| in case it
     // was leaked.
     mViewManager->SetPresShell(nullptr);
     mViewManager = nullptr;
   }
 
@@ -2146,17 +2147,18 @@ void PresShell::NotifyDestroyingFrame(ns
         mCurrentEventFrameStack[i] = nullptr;
       }
     }
 
     mFramesToDirty.RemoveEntry(aFrame);
 
     nsIScrollableFrame* scrollableFrame = do_QueryFrame(aFrame);
     if (scrollableFrame) {
-      mDirtyScrollAnchorContainers.RemoveEntry(scrollableFrame);
+      mPendingScrollAnchorSelection.RemoveEntry(scrollableFrame);
+      mPendingScrollAnchorAdjustment.RemoveEntry(scrollableFrame);
     }
   }
 }
 
 already_AddRefed<nsCaret> PresShell::GetCaret() const {
   RefPtr<nsCaret> caret = mCaret;
   return caret.forget();
 }
@@ -2569,27 +2571,42 @@ void PresShell::VerifyHasDirtyRootAncest
   }
 
   MOZ_ASSERT_UNREACHABLE(
       "Frame has dirty bits set but isn't scheduled to be "
       "reflowed?");
 }
 #endif
 
-void PresShell::PostDirtyScrollAnchorContainer(nsIScrollableFrame* aFrame) {
-  mDirtyScrollAnchorContainers.PutEntry(aFrame);
-}
-
-void PresShell::FlushDirtyScrollAnchorContainers() {
-  for (auto iter = mDirtyScrollAnchorContainers.Iter(); !iter.Done();
+void PresShell::PostPendingScrollAnchorSelection(
+    mozilla::layout::ScrollAnchorContainer* aContainer) {
+  mPendingScrollAnchorSelection.PutEntry(aContainer->ScrollableFrame());
+}
+
+void PresShell::FlushPendingScrollAnchorSelections() {
+  for (auto iter = mPendingScrollAnchorSelection.Iter(); !iter.Done();
        iter.Next()) {
     nsIScrollableFrame* scroll = iter.Get()->GetKey();
     scroll->GetAnchor()->SelectAnchor();
   }
-  mDirtyScrollAnchorContainers.Clear();
+  mPendingScrollAnchorSelection.Clear();
+}
+
+void PresShell::PostPendingScrollAnchorAdjustment(
+    ScrollAnchorContainer* aContainer) {
+  mPendingScrollAnchorAdjustment.PutEntry(aContainer->ScrollableFrame());
+}
+
+void PresShell::FlushPendingScrollAnchorAdjustments() {
+  for (auto iter = mPendingScrollAnchorAdjustment.Iter(); !iter.Done();
+       iter.Next()) {
+    nsIScrollableFrame* scroll = iter.Get()->GetKey();
+    scroll->GetAnchor()->ApplyAdjustments();
+  }
+  mPendingScrollAnchorAdjustment.Clear();
 }
 
 void PresShell::FrameNeedsReflow(nsIFrame* aFrame,
                                  IntrinsicDirty aIntrinsicDirty,
                                  nsFrameState aBitToAdd,
                                  ReflowRootHandling aRootHandling) {
   MOZ_ASSERT(aBitToAdd == NS_FRAME_IS_DIRTY ||
                  aBitToAdd == NS_FRAME_HAS_DIRTY_CHILDREN || !aBitToAdd,
@@ -4169,23 +4186,27 @@ void PresShell::DoFlushPendingNotificati
 
     if (flushType >= (SuppressInterruptibleReflows()
                           ? FlushType::Layout
                           : FlushType::InterruptibleLayout) &&
         !mIsDestroying) {
       didLayoutFlush = true;
       mFrameConstructor->RecalcQuotesAndCounters();
       viewManager->FlushDelayedResize(true);
-      if (ProcessReflowCommands(flushType < FlushType::Layout) &&
-          mContentToScrollTo) {
-        // We didn't get interrupted.  Go ahead and scroll to our content
-        DoScrollContentIntoView();
+      if (ProcessReflowCommands(flushType < FlushType::Layout)) {
+        // We didn't get interrupted. Go ahead and perform scroll anchor
+        // adjustments and scroll content into view
+        FlushPendingScrollAnchorAdjustments();
+
         if (mContentToScrollTo) {
-          mContentToScrollTo->DeleteProperty(nsGkAtoms::scrolling);
-          mContentToScrollTo = nullptr;
+          DoScrollContentIntoView();
+          if (mContentToScrollTo) {
+            mContentToScrollTo->DeleteProperty(nsGkAtoms::scrolling);
+            mContentToScrollTo = nullptr;
+          }
         }
       }
     }
 
     if (flushType >= FlushType::Layout) {
       if (!mIsDestroying) {
         viewManager->UpdateWidgetGeometry();
       }
@@ -8493,17 +8514,17 @@ bool PresShell::DoReflow(nsIFrame* targe
 #ifdef MOZ_GECKO_PROFILER
   DECLARE_DOCSHELL_AND_HISTORY_ID(docShell);
   AutoProfilerTracing tracingLayoutFlush("Paint", "Reflow",
                                          std::move(mReflowCause), docShellId,
                                          docShellHistoryId);
   mReflowCause = nullptr;
 #endif
 
-  FlushDirtyScrollAnchorContainers();
+  FlushPendingScrollAnchorSelections();
 
   if (mReflowContinueTimer) {
     mReflowContinueTimer->Cancel();
     mReflowContinueTimer = nullptr;
   }
 
   const bool isRoot = target == mFrameConstructor->GetRootFrame();
 
@@ -8616,17 +8637,17 @@ bool PresShell::DoReflow(nsIFrame* targe
       mPresContext, target, target->GetView(), boundsRelativeToTarget);
   nsContainerFrame::SyncWindowProperties(mPresContext, target,
                                          target->GetView(), rcx,
                                          nsContainerFrame::SET_ASYNC);
 
   target->DidReflow(mPresContext, nullptr);
   if (target->IsInScrollAnchorChain()) {
     ScrollAnchorContainer* container = ScrollAnchorContainer::FindFor(target);
-    container->ApplyAdjustments();
+    PostPendingScrollAnchorAdjustment(container);
   }
   if (isRoot && size.BSize(wm) == NS_UNCONSTRAINEDSIZE) {
     mPresContext->SetVisibleArea(boundsRelativeToTarget);
   }
 
 #ifdef DEBUG
   mCurrentReflowRoot = nullptr;
 #endif
@@ -10029,17 +10050,18 @@ void PresShell::AddSizeOfIncludingThis(n
   mFrameArena.AddSizeOfExcludingThis(aSizes);
   aSizes.mLayoutPresShellSize += mallocSizeOf(this);
   if (mCaret) {
     aSizes.mLayoutPresShellSize += mCaret->SizeOfIncludingThis(mallocSizeOf);
   }
   aSizes.mLayoutPresShellSize +=
       mApproximatelyVisibleFrames.ShallowSizeOfExcludingThis(mallocSizeOf) +
       mFramesToDirty.ShallowSizeOfExcludingThis(mallocSizeOf) +
-      mDirtyScrollAnchorContainers.ShallowSizeOfExcludingThis(mallocSizeOf);
+      mPendingScrollAnchorSelection.ShallowSizeOfExcludingThis(mallocSizeOf) +
+      mPendingScrollAnchorAdjustment.ShallowSizeOfExcludingThis(mallocSizeOf);
 
   StyleSet()->AddSizeOfIncludingThis(aSizes);
 
   aSizes.mLayoutTextRunsSize += SizeOfTextRuns(mallocSizeOf);
 
   aSizes.mLayoutPresContextSize +=
       mPresContext->SizeOfIncludingThis(mallocSizeOf);
 
--- a/layout/base/PresShell.h
+++ b/layout/base/PresShell.h
@@ -108,18 +108,22 @@ class PresShell final : public nsIPresSh
       nscoord aOldHeight = 0,
       ResizeReflowOptions aOptions = ResizeReflowOptions::eBSizeExact) override;
   nsresult ResizeReflowIgnoreOverride(
       nscoord aWidth, nscoord aHeight, nscoord aOldWidth, nscoord aOldHeight,
       ResizeReflowOptions aOptions = ResizeReflowOptions::eBSizeExact) override;
   nsIPageSequenceFrame* GetPageSequenceFrame() const override;
   nsCanvasFrame* GetCanvasFrame() const override;
 
-  void PostDirtyScrollAnchorContainer(nsIScrollableFrame* aFrame) override;
-  void FlushDirtyScrollAnchorContainers() override;
+  void PostPendingScrollAnchorSelection(
+      mozilla::layout::ScrollAnchorContainer* aContainer) override;
+  void FlushPendingScrollAnchorSelections() override;
+  void PostPendingScrollAnchorAdjustment(
+      mozilla::layout::ScrollAnchorContainer* aContainer) override;
+  void FlushPendingScrollAnchorAdjustments();
 
   void FrameNeedsReflow(
       nsIFrame* aFrame, IntrinsicDirty aIntrinsicDirty, nsFrameState aBitToAdd,
       ReflowRootHandling aRootHandling = eInferFromBitToAdd) override;
   void FrameNeedsToContinueReflow(nsIFrame* aFrame) override;
   void CancelAllPendingReflows() override;
   void DoFlushPendingNotifications(FlushType aType) override;
   void DoFlushPendingNotifications(ChangesToFlush aType) override;
@@ -742,17 +746,18 @@ class PresShell final : public nsIPresSh
   layers::ScrollableLayerGuid mMouseEventTargetGuid;
 
   // mStyleSet owns it but we maintain a ref, may be null
   RefPtr<StyleSheet> mPrefStyleSheet;
 
   // Set of frames that we should mark with NS_FRAME_HAS_DIRTY_CHILDREN after
   // we finish reflowing mCurrentReflowRoot.
   nsTHashtable<nsPtrHashKey<nsIFrame>> mFramesToDirty;
-  nsTHashtable<nsPtrHashKey<nsIScrollableFrame>> mDirtyScrollAnchorContainers;
+  nsTHashtable<nsPtrHashKey<nsIScrollableFrame>> mPendingScrollAnchorSelection;
+  nsTHashtable<nsPtrHashKey<nsIScrollableFrame>> mPendingScrollAnchorAdjustment;
 
   nsTArray<UniquePtr<DelayedEvent>> mDelayedEvents;
 
  private:
   nsRevocableEventPtr<nsSynthMouseMoveEvent> mSynthMouseMoveEvent;
   nsCOMPtr<nsIContent> mLastAnchorScrolledTo;
   RefPtr<nsCaret> mCaret;
   RefPtr<nsCaret> mOriginalCaret;
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@@ -771,17 +771,17 @@ static bool RecomputePosition(nsIFrame* 
         }
         cont->SetPosition(normalPosition +
                           nsPoint(newOffsets.left, newOffsets.top));
       }
     }
 
     if (aFrame->IsInScrollAnchorChain()) {
       ScrollAnchorContainer* container = ScrollAnchorContainer::FindFor(aFrame);
-      container->ApplyAdjustments();
+      aFrame->PresShell()->PostPendingScrollAnchorAdjustment(container);
     }
     return true;
   }
 
   // For the absolute positioning case, set up a fake HTML reflow state for
   // the frame, and then get the offsets and size from it. If the frame's size
   // doesn't need to change, we can simply update the frame position. Otherwise
   // we fall back to a reflow.
@@ -880,17 +880,17 @@ static bool RecomputePosition(nsIFrame* 
     nsPoint pos(parentBorder.left + reflowInput.ComputedPhysicalOffsets().left +
                     reflowInput.ComputedPhysicalMargin().left,
                 parentBorder.top + reflowInput.ComputedPhysicalOffsets().top +
                     reflowInput.ComputedPhysicalMargin().top);
     aFrame->SetPosition(pos);
 
     if (aFrame->IsInScrollAnchorChain()) {
       ScrollAnchorContainer* container = ScrollAnchorContainer::FindFor(aFrame);
-      container->ApplyAdjustments();
+      aFrame->PresShell()->PostPendingScrollAnchorAdjustment(container);
     }
     return true;
   }
 
   // Fall back to a reflow
   StyleChangeReflow(aFrame, nsChangeHint_NeedReflow |
                                 nsChangeHint_ReflowChangesSizeOrPosition);
   return false;
@@ -2967,17 +2967,17 @@ void RestyleManager::DoProcessPendingRes
     // fix up the callers and assert against this case, but we just detect and
     // handle it for now.
     return;
   }
 
   // Select scroll anchors for frames that have been scrolled. Do this
   // before restyling so that anchor nodes are correctly marked for
   // scroll anchor update suppressions.
-  presContext->PresShell()->FlushDirtyScrollAnchorContainers();
+  presContext->PresShell()->FlushPendingScrollAnchorSelections();
 
   // Create a AnimationsWithDestroyedFrame during restyling process to
   // stop animations and transitions on elements that have no frame at the end
   // of the restyling process.
   AnimationsWithDestroyedFrame animationsWithDestroyedFrame(this);
 
   ServoStyleSet* styleSet = StyleSet();
   Document* doc = presContext->Document();
--- a/layout/base/nsIPresShell.h
+++ b/layout/base/nsIPresShell.h
@@ -98,16 +98,20 @@ class Element;
 class Event;
 class Document;
 class HTMLSlotElement;
 class Touch;
 class Selection;
 class ShadowRoot;
 }  // namespace dom
 
+namespace layout {
+class ScrollAnchorContainer;
+}  // namespace layout
+
 namespace layers {
 class LayerManager;
 }  // namespace layers
 
 namespace gfx {
 class SourceSurface;
 }  // namespace gfx
 }  // namespace mozilla
@@ -451,18 +455,21 @@ class nsIPresShell : public nsStubDocume
   virtual nsIPageSequenceFrame* GetPageSequenceFrame() const = 0;
 
   /**
    * Returns the canvas frame associated with the frame hierarchy.
    * Returns nullptr if is XUL document.
    */
   virtual nsCanvasFrame* GetCanvasFrame() const = 0;
 
-  virtual void PostDirtyScrollAnchorContainer(nsIScrollableFrame* aFrame) = 0;
-  virtual void FlushDirtyScrollAnchorContainers() = 0;
+  virtual void PostPendingScrollAnchorSelection(
+      mozilla::layout::ScrollAnchorContainer* aContainer) = 0;
+  virtual void FlushPendingScrollAnchorSelections() = 0;
+  virtual void PostPendingScrollAnchorAdjustment(
+      mozilla::layout::ScrollAnchorContainer* aContainer) = 0;
 
   /**
    * Tell the pres shell that a frame needs to be marked dirty and needs
    * Reflow.  It's OK if this is an ancestor of the frame needing reflow as
    * long as the ancestor chain between them doesn't cross a reflow root.
    *
    * The bit to add should be NS_FRAME_IS_DIRTY, NS_FRAME_HAS_DIRTY_CHILDREN
    * or nsFrameState(0); passing 0 means that dirty bits won't be set on the
--- a/layout/generic/ScrollAnchorContainer.cpp
+++ b/layout/generic/ScrollAnchorContainer.cpp
@@ -222,17 +222,17 @@ void ScrollAnchorContainer::InvalidateAn
   ANCHOR_LOG("Invalidating scroll anchor %p for %p.\n", mAnchorNode, this);
 
   if (mAnchorNode) {
     SetAnchorFlags(mScrollFrame->mScrolledFrame, mAnchorNode, false);
   }
   mAnchorNode = nullptr;
   mAnchorNodeIsDirty = true;
   mLastAnchorPos = nsPoint();
-  Frame()->PresShell()->PostDirtyScrollAnchorContainer(ScrollableFrame());
+  Frame()->PresShell()->PostPendingScrollAnchorSelection(this);
 }
 
 void ScrollAnchorContainer::Destroy() {
   if (mAnchorNode) {
     SetAnchorFlags(mScrollFrame->mScrolledFrame, mAnchorNode, false);
   }
   mAnchorNode = nullptr;
   mAnchorNodeIsDirty = false;
--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@@ -1131,17 +1131,17 @@ void nsHTMLScrollFrame::Reflow(nsPresCon
   aStatus.Reset();  // This type of frame can't be split.
   NS_FRAME_SET_TRUNCATION(aStatus, aReflowInput, aDesiredSize);
   mHelper.PostOverflowEvent();
 }
 
 void nsHTMLScrollFrame::DidReflow(nsPresContext* aPresContext,
                                   const ReflowInput* aReflowInput) {
   nsContainerFrame::DidReflow(aPresContext, aReflowInput);
-  mHelper.mAnchor.ApplyAdjustments();
+  PresShell()->PostPendingScrollAnchorAdjustment(GetAnchor());
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 
 #ifdef DEBUG_FRAME_DUMP
 nsresult nsHTMLScrollFrame::GetFrameName(nsAString& aResult) const {
   return MakeFrameName(NS_LITERAL_STRING("HTMLScroll"), aResult);
 }
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/meta/css/css-scroll-anchoring/anchoring-with-bounds-clamping-div.html.ini
@@ -0,0 +1,3 @@
+[anchoring-with-bounds-clamping-div.html]
+    [Anchoring combined with scroll bounds clamping in a <div>.]
+        expected: FAIL
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-scroll-anchoring/heuristic-with-offset-update.html
@@ -0,0 +1,58 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<link rel="help" href="https://drafts.csswg.org/css-scroll-anchoring-1/">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<html>
+<head>
+    <style type="text/css">
+        #scroller {
+            overflow: scroll;
+            height: 500px;
+            height: 500px;
+        }
+        #before {
+            height: 200px;
+        }
+        #anchor {
+            position: relative;
+            width: 200px;
+            height: 200px;
+            margin-bottom: 500px;
+            background-color: blue;
+            /*
+             * To trigger the Gecko bug that's being regression-tested here, we
+             * need 'top' to start out at a non-'auto' value, so that the
+             * dynamic change can trigger Gecko's "RecomputePosition" fast path
+             */
+            top: 0px;
+        }
+    </style>
+</head>
+<body>
+    <div id="scroller">
+        <div id="before">
+        </div>
+        <div id="anchor">
+        </div>
+    </div>
+
+    <script type="text/javascript">
+        test(() => {
+            let scroller = document.querySelector('#scroller');
+            let before = document.querySelector('#before');
+            let anchor = document.querySelector('#anchor');
+
+            // Scroll down to select #anchor as a scroll anchor
+            scroller.scrollTop = 200;
+
+            // Adjust the 'top' of #anchor, which should trigger a suppression
+            anchor.style.top = '10px';
+
+            // Expand #before and make sure we don't apply an adjustment
+            before.style.height = '300px';
+            assert_equals(scroller.scrollTop, 200);
+        }, 'Positioned ancestors with dynamic changes to offsets trigger scroll suppressions.');
+    </script>
+</body>
+</html>