Bug 1268195 - When restoring a scroll position outside of incremental load, don't keep trying in a loop - just do it once and stop. r=tnikkel
authorKartikaya Gupta <kgupta@mozilla.com>
Fri, 29 Apr 2016 23:06:18 -0400
changeset 295603 a12cdfe43e929dfa151e3ea1c35982cef674ff66
parent 295602 774c838c19e217e177ba22876d57a344b54c33e4
child 295604 0939fd079113f9fe252f8b242baf974c694c1e10
push id19015
push usercbook@mozilla.com
push dateMon, 02 May 2016 09:39:23 +0000
treeherderfx-team@2080375bc69d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1268195
milestone49.0a1
Bug 1268195 - When restoring a scroll position outside of incremental load, don't keep trying in a loop - just do it once and stop. r=tnikkel It may be that when the frame is reconstructed after load, the frame gets shorter, and the old scroll position cannot be restored, because it is out of bounds. In such a case, we don't want to keep mRestorePos tracking the old scroll position, because it can get incorrectly applied on a future frame reconstruction. Instead, for scroll position restorations during frame reconstructions, we just try the restore once and then clear mRestorePos. MozReview-Commit-ID: BHoJHz0mGmf
layout/base/nsPresShell.cpp
layout/base/nsPresState.h
layout/base/tests/mochitest.ini
layout/base/tests/test_frame_reconstruction_scroll_restore.html
layout/forms/nsTextControlFrame.cpp
layout/generic/nsGfxScrollFrame.cpp
layout/generic/nsGfxScrollFrame.h
layout/generic/nsIScrollableFrame.h
--- a/layout/base/nsPresShell.cpp
+++ b/layout/base/nsPresShell.cpp
@@ -2406,16 +2406,17 @@ PresShell::EndUpdate(nsIDocument *aDocum
   mFrameConstructor->EndUpdate();
 }
 
 void
 PresShell::RestoreRootScrollPosition()
 {
   nsIScrollableFrame* scrollableFrame = GetRootScrollFrameAsScrollable();
   if (scrollableFrame) {
+    scrollableFrame->SetRestoringHistoryScrollPosition(true);
     scrollableFrame->ScrollToRestoredPosition();
   }
 }
 
 void
 PresShell::MaybeReleaseCapturingContent()
 {
   RefPtr<nsFrameSelection> frameSelection = FrameSelection();
@@ -2463,16 +2464,21 @@ PresShell::EndLoad(nsIDocument *aDocumen
   RestoreRootScrollPosition();
 
   mDocumentLoading = false;
 }
 
 void
 PresShell::LoadComplete()
 {
+  nsIScrollableFrame* scrollableFrame = GetRootScrollFrameAsScrollable();
+  if (scrollableFrame) {
+    scrollableFrame->SetRestoringHistoryScrollPosition(false);
+  }
+
   gfxTextPerfMetrics *tp = nullptr;
   if (mPresContext) {
     tp = mPresContext->GetTextPerfMetrics();
   }
 
   // log load
   bool shouldLog = MOZ_LOG_TEST(gLog, LogLevel::Debug);
   if (shouldLog || tp) {
--- a/layout/base/nsPresState.h
+++ b/layout/base/nsPresState.h
@@ -16,33 +16,40 @@
 #include "nsAutoPtr.h"
 
 class nsPresState
 {
 public:
   nsPresState()
     : mContentData(nullptr)
     , mScrollState(0, 0)
+    , mRestoringHistoryScrollPosition(true)
     , mResolution(1.0)
     , mScaleToResolution(false)
     , mDisabledSet(false)
     , mDisabled(false)
     , mDroppedDown(false)
   {}
 
-  void SetScrollState(const nsPoint& aState)
+  void SetScrollState(const nsPoint& aState, bool aRestoringHistoryScrollPosition = true)
   {
     mScrollState = aState;
+    mRestoringHistoryScrollPosition = aRestoringHistoryScrollPosition;
   }
 
-  nsPoint GetScrollState() const
+  nsPoint GetScrollPosition() const
   {
     return mScrollState;
   }
 
+  bool GetRestoringHistoryScrollPosition() const
+  {
+    return mRestoringHistoryScrollPosition;
+  }
+
   void SetResolution(float aSize)
   {
     mResolution = aSize;
   }
 
   float GetResolution() const
   {
     return mResolution;
@@ -99,16 +106,17 @@ public:
   {
     return mDroppedDown;
   }
 
 // MEMBER VARIABLES
 protected:
   nsCOMPtr<nsISupports> mContentData;
   nsPoint mScrollState;
+  bool mRestoringHistoryScrollPosition;
   float mResolution;
   bool mScaleToResolution;
   bool mDisabledSet;
   bool mDisabled;
   bool mDroppedDown;
 };
 
 #endif /* nsPresState_h_ */
--- a/layout/base/tests/mochitest.ini
+++ b/layout/base/tests/mochitest.ini
@@ -283,8 +283,9 @@ support-files =
 support-files = bug1226904.html
 [test_bug1246622.html]
 [test_transformed_scrolling_repaints.html]
 skip-if = buildapp == 'b2g'
 [test_transformed_scrolling_repaints_2.html]
 skip-if = buildapp == 'b2g'
 [test_transformed_scrolling_repaints_3.html]
 skip-if = buildapp == 'b2g'
+[test_frame_reconstruction_scroll_restore.html]
new file mode 100644
--- /dev/null
+++ b/layout/base/tests/test_frame_reconstruction_scroll_restore.html
@@ -0,0 +1,68 @@
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1268195
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 1268195</title>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+  <style>
+    html, body {
+        margin: 0;
+        padding: 0;
+    }
+
+    .noscroll {
+        overflow: hidden;
+        height: 100%;
+    }
+
+    /* Toggling this on and off triggers a frame reconstruction on the <body> */
+    html.reconstruct-body::before {
+        top: 0;
+        content: '';
+        display: block;
+        height: 2px;
+        position: absolute;
+        width: 100%;
+        z-index: 99;
+    }
+  </style>
+  <script type="application/javascript">
+    SimpleTest.waitForExplicitFinish();
+
+    function run() {
+        // Make sure we have the right scroll element
+        SimpleTest.is(document.body.scrollTopMax > 0, true, "Body is the scrolling element");
+
+        // Scroll to the bottom
+        document.body.scrollTop = document.body.scrollTopMax;
+        SimpleTest.is(document.body.scrollTop > 0, true, "Scrolled body");
+
+        // Do a frame reconstruction on the body while also shortening the
+        // height.
+        document.body.classList.toggle('noscroll');
+        document.documentElement.classList.toggle('reconstruct-body');
+        document.getElementById('spacer').style.height = '1px';
+        SimpleTest.is(document.body.scrollTop, 0, "Scroll forced to top");
+
+        // Do another frame reconstruction while lengthening the height again.
+        document.body.classList.toggle('noscroll');
+        document.documentElement.classList.toggle('reconstruct-body');
+        document.getElementById('spacer').style.height = '5000px';
+        SimpleTest.is(document.body.scrollTop, 0, "Scroll remained at top");
+
+        SimpleTest.finish();
+    }
+  </script>
+</head>
+<body onload="setTimeout(run, 0)">
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1268195">Mozilla Bug 1268195</a><br/>
+The scroll position should end the top of the page. This is the top, yay!
+<div id="spacer" style="height: 5000px"></div>
+The scroll position should end the top of the page. This is the bottom!
+<pre id="test">
+</pre>
+</body>
+</html>
--- a/layout/forms/nsTextControlFrame.cpp
+++ b/layout/forms/nsTextControlFrame.cpp
@@ -1389,17 +1389,17 @@ nsTextControlFrame::RestoreState(nsPresS
     if (scrollStateFrame) {
       return scrollStateFrame->RestoreState(aState);
     }
   }
 
   // Most likely, we don't have our anonymous content constructed yet, which
   // would cause us to end up here.  In this case, we'll just store the scroll
   // pos ourselves, and forward it to the scroll frame later when it's created.
-  Properties().Set(ContentScrollPos(), new nsPoint(aState->GetScrollState()));
+  Properties().Set(ContentScrollPos(), new nsPoint(aState->GetScrollPosition()));
   return NS_OK;
 }
 
 nsresult
 nsTextControlFrame::PeekOffset(nsPeekOffsetStruct *aPos)
 {
   return NS_ERROR_FAILURE;
 }
--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@@ -1886,16 +1886,17 @@ ScrollFrameHelper::ScrollFrameHelper(nsC
   , mWillBuildScrollableLayer(false)
   , mIsScrollParent(false)
   , mIsScrollableLayerInRootContainer(false)
   , mHasBeenScrolled(false)
   , mIgnoreMomentumScroll(false)
   , mTransformingByAPZ(false)
   , mScrollableByAPZ(false)
   , mZoomableByAPZ(false)
+  , mRestoringHistoryScrollPosition(true)
   , mVelocityQueue(aOuter->PresContext())
   , mAsyncScrollEvent(END_DOM)
 {
   if (LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars) != 0) {
     mScrollbarActivity = new ScrollbarActivity(do_QueryFrame(aOuter));
   }
 
   EnsureFrameVisPrefsCached();
@@ -4054,30 +4055,45 @@ ScrollFrameHelper::ScrollToRestoredPosit
         scrollToPos.x = mScrollPort.x -
           (mScrollPort.XMost() - scrollToPos.x - mScrolledFrame->GetRect().width);
       nsWeakFrame weakFrame(mOuter);
       ScrollToWithOrigin(scrollToPos, nsIScrollableFrame::INSTANT,
                          nsGkAtoms::restore, nullptr);
       if (!weakFrame.IsAlive()) {
         return;
       }
-      // Re-get the scroll position, it might not be exactly equal to
-      // mRestorePos due to rounding and clamping.
-      mLastPos = GetLogicalScrollPosition();
-    } else {
-      // if we reached the position then stop
-      mRestorePos.y = -1;
-      mLastPos.x = -1;
-      mLastPos.y = -1;
+      if (mRestoringHistoryScrollPosition) {
+        // If we're trying to do a history scroll restore, then we want to
+        // keep trying this until we succeed, because the page can be loading
+        // incrementally. So re-get the scroll position for the next iteration,
+        // it might not be exactly equal to mRestorePos due to rounding and
+        // clamping.
+        mLastPos = GetLogicalScrollPosition();
+        return;
+      }
     }
+    // If we get here, either we reached the desired position (mLastPos ==
+    // mRestorePos) or we're not trying to do a history scroll restore, so
+    // we can stop after the scroll attempt above.
+    mRestorePos.y = -1;
+    mLastPos.x = -1;
+    mLastPos.y = -1;
+    mRestoringHistoryScrollPosition = false;
   } else {
     // user moved the position, so we won't need to restore
     mLastPos.x = -1;
     mLastPos.y = -1;
-  }
+    mRestoringHistoryScrollPosition = false;
+  }
+}
+
+void
+ScrollFrameHelper::SetRestoringHistoryScrollPosition(bool aValue)
+{
+  mRestoringHistoryScrollPosition = aValue;
 }
 
 nsresult
 ScrollFrameHelper::FireScrollPortEvent()
 {
   mAsyncScrollPortEvent.Forget();
 
   // Keep this in sync with PostOverflowEvent().
@@ -5651,30 +5667,31 @@ ScrollFrameHelper::SaveState() const
   // valid and we haven't moved since the last update of mLastPos (same check
   // that ScrollToRestoredPosition uses). This ensures if a reframe occurs
   // while we're in the process of loading content to scroll to a restored
   // position, we'll keep trying after the reframe.
   nsPoint pt = GetLogicalScrollPosition();
   if (mRestorePos.y != -1 && pt == mLastPos) {
     pt = mRestorePos;
   }
-  state->SetScrollState(pt);
+  state->SetScrollState(pt, mRestoringHistoryScrollPosition);
   if (mIsRoot) {
     // Only save resolution properties for root scroll frames
     nsIPresShell* shell = mOuter->PresContext()->PresShell();
     state->SetResolution(shell->GetResolution());
     state->SetScaleToResolution(shell->ScaleToResolution());
   }
   return state;
 }
 
 void
 ScrollFrameHelper::RestoreState(nsPresState* aState)
 {
-  mRestorePos = aState->GetScrollState();
+  mRestorePos = aState->GetScrollPosition();
+  mRestoringHistoryScrollPosition = aState->GetRestoringHistoryScrollPosition();
   mDidHistoryRestore = true;
   mLastPos = mScrolledFrame ? GetLogicalScrollPosition() : nsPoint(0,0);
 
   // Resolution properties should only exist on root scroll frames.
   MOZ_ASSERT(mIsRoot || (!aState->GetScaleToResolution() &&
                          aState->GetResolution() == 1.0));
 
   if (mIsRoot) {
--- a/layout/generic/nsGfxScrollFrame.h
+++ b/layout/generic/nsGfxScrollFrame.h
@@ -251,16 +251,19 @@ public:
                 nsIAtom* aOrigin = nullptr,
                 nsIScrollableFrame::ScrollMomentum aMomentum = nsIScrollableFrame::NOT_MOMENTUM,
                 nsIScrollbarMediator::ScrollSnapMode aSnap
                   = nsIScrollbarMediator::DISABLE_SNAP);
   /**
    * @note This method might destroy the frame, pres shell and other objects.
    */
   void ScrollToRestoredPosition();
+
+  void SetRestoringHistoryScrollPosition(bool aValue);
+
   /**
    * GetSnapPointForDestination determines which point to snap to after
    * scrolling. aStartPos gives the position before scrolling and aDestination
    * gives the position after scrolling, with no snapping. Behaviour is
    * dependent on the value of aUnit.
    * Returns true if a suitable snap point could be found and aDestination has
    * been updated to a valid snapping position.
    */
@@ -573,16 +576,21 @@ public:
   bool mScrollableByAPZ:1;
 
   // True if the APZ is allowed to zoom this scrollframe.
   bool mZoomableByAPZ:1;
 
   // True if we don't want the scrollbar to repaint itself right now.
   bool mSuppressScrollbarRepaints:1;
 
+  // True if the calls to ScrollToRestoredPosition() are trying to restore the
+  // scroll position from history, and need to account for incremental page
+  // load.
+  bool mRestoringHistoryScrollPosition:1;
+
   mozilla::layout::ScrollVelocityQueue mVelocityQueue;
 
 protected:
   class AutoScrollbarRepaintSuppression;
   friend class AutoScrollbarRepaintSuppression;
   class AutoScrollbarRepaintSuppression {
   public:
     AutoScrollbarRepaintSuppression(ScrollFrameHelper* aHelper, bool aSuppress)
@@ -837,16 +845,19 @@ public:
     mHelper.ScrollSnap();
   }
   /**
    * @note This method might destroy the frame, pres shell and other objects.
    */
   virtual void ScrollToRestoredPosition() override {
     mHelper.ScrollToRestoredPosition();
   }
+  virtual void SetRestoringHistoryScrollPosition(bool aValue) override {
+    mHelper.SetRestoringHistoryScrollPosition(aValue);
+  }
   virtual void AddScrollPositionListener(nsIScrollPositionListener* aListener) override {
     mHelper.AddScrollPositionListener(aListener);
   }
   virtual void RemoveScrollPositionListener(nsIScrollPositionListener* aListener) override {
     mHelper.RemoveScrollPositionListener(aListener);
   }
   /**
    * @note This method might destroy the frame, pres shell and other objects.
@@ -1246,16 +1257,19 @@ public:
     mHelper.ScrollSnap();
   }
   /**
    * @note This method might destroy the frame, pres shell and other objects.
    */
   virtual void ScrollToRestoredPosition() override {
     mHelper.ScrollToRestoredPosition();
   }
+  virtual void SetRestoringHistoryScrollPosition(bool aValue) override {
+    mHelper.SetRestoringHistoryScrollPosition(aValue);
+  }
   virtual void AddScrollPositionListener(nsIScrollPositionListener* aListener) override {
     mHelper.AddScrollPositionListener(aListener);
   }
   virtual void RemoveScrollPositionListener(nsIScrollPositionListener* aListener) override {
     mHelper.RemoveScrollPositionListener(aListener);
   }
   /**
    * @note This method might destroy the frame, pres shell and other objects.
--- a/layout/generic/nsIScrollableFrame.h
+++ b/layout/generic/nsIScrollableFrame.h
@@ -283,16 +283,25 @@ public:
    * at least once after state has been restored. It is called by the
    * scrolled frame itself during reflow, but sometimes state can be
    * restored after reflows are done...
    * XXX should we take an aMode parameter here? Currently it's instant.
    */
   virtual void ScrollToRestoredPosition() = 0;
 
   /**
+   * Set whether or not the scrollframe should treat scroll position restoration
+   * as coming from history or not. This changes behaviour slightly, as history-
+   * basied scroll position restorations need to deal with incremental page
+   * loading, where the restore attempt might not work until more of the page
+   * is loaded.
+   */
+  virtual void SetRestoringHistoryScrollPosition(bool aValue) = 0;
+
+  /**
    * Add a scroll position listener. This listener must be removed
    * before it is destroyed.
    */
   virtual void AddScrollPositionListener(nsIScrollPositionListener* aListener) = 0;
   /**
    * Remove a scroll position listener.
    */
   virtual void RemoveScrollPositionListener(nsIScrollPositionListener* aListener) = 0;