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
2016-04-29 23:06:18 -04:00 · 2016-04-29 23:06:18 -04:00 · 0858e31ab6
--- a/layout/base/nsPresShell.cpp
+++ b/layout/base/nsPresShell.cpp
@ -2411,6 +2411,7 @@ PresShell::RestoreRootScrollPosition()
 {
  nsIScrollableFrame* scrollableFrame = GetRootScrollFrameAsScrollable();
  if (scrollableFrame) {
+    scrollableFrame->SetRestoringHistoryScrollPosition(true);
    scrollableFrame->ScrollToRestoredPosition();
  }
 }
@ -2468,6 +2469,11 @@ PresShell::EndLoad(nsIDocument *aDocument)
 void
 PresShell::LoadComplete()
 {
+  nsIScrollableFrame* scrollableFrame = GetRootScrollFrameAsScrollable();
+  if (scrollableFrame) {
+    scrollableFrame->SetRestoringHistoryScrollPosition(false);
+  }
+
  gfxTextPerfMetrics *tp = nullptr;
  if (mPresContext) {
    tp = mPresContext->GetTextPerfMetrics();
--- a/layout/base/nsPresState.h
+++ b/layout/base/nsPresState.h
@ -21,6 +21,7 @@ public:
  nsPresState()
    : mContentData(nullptr)
    , mScrollState(0, 0)
+    , mRestoringHistoryScrollPosition(true)
    , mResolution(1.0)
    , mScaleToResolution(false)
    , mDisabledSet(false)
@ -28,16 +29,22 @@ public:
    , 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;
@ -104,6 +111,7 @@ public:
 protected:
  nsCOMPtr<nsISupports> mContentData;
  nsPoint mScrollState;
+  bool mRestoringHistoryScrollPosition;
  float mResolution;
  bool mScaleToResolution;
  bool mDisabledSet;
--- a/layout/base/tests/mochitest.ini
+++ b/layout/base/tests/mochitest.ini
@ -288,3 +288,4 @@ skip-if = buildapp == 'b2g'
 skip-if = buildapp == 'b2g'
 [test_transformed_scrolling_repaints_3.html]
 skip-if = buildapp == 'b2g'
+[test_frame_reconstruction_scroll_restore.html]
--- a/layout/base/tests/test_frame_reconstruction_scroll_restore.html
+++ 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
@ -1394,7 +1394,7 @@ nsTextControlFrame::RestoreState(nsPresState* 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;
 }

--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@ -1891,6 +1891,7 @@ ScrollFrameHelper::ScrollFrameHelper(nsContainerFrame* aOuter,
  , mTransformingByAPZ(false)
  , mScrollableByAPZ(false)
  , mZoomableByAPZ(false)
+  , mRestoringHistoryScrollPosition(true)
  , mVelocityQueue(aOuter->PresContext())
  , mAsyncScrollEvent(END_DOM)
 {
@ -4059,22 +4060,37 @@ ScrollFrameHelper::ScrollToRestoredPosition()
      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()
 {
@ -5656,7 +5672,7 @@ ScrollFrameHelper::SaveState() const
  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();
@ -5669,7 +5685,8 @@ ScrollFrameHelper::SaveState() const
 void
 ScrollFrameHelper::RestoreState(nsPresState* aState)
 {
-  mRestorePos = aState->GetScrollState();
+  mRestorePos = aState->GetScrollPosition();
+  mRestoringHistoryScrollPosition = aState->GetRestoringHistoryScrollPosition();
  mDidHistoryRestore = true;
  mLastPos = mScrolledFrame ? GetLogicalScrollPosition() : nsPoint(0,0);

--- a/layout/generic/nsGfxScrollFrame.h
+++ b/layout/generic/nsGfxScrollFrame.h
@ -256,6 +256,9 @@ public:
   * @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
@ -578,6 +581,11 @@ public:
  // 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:
@ -842,6 +850,9 @@ public:
  virtual void ScrollToRestoredPosition() override {
    mHelper.ScrollToRestoredPosition();
  }
+  virtual void SetRestoringHistoryScrollPosition(bool aValue) override {
+    mHelper.SetRestoringHistoryScrollPosition(aValue);
+  }
  virtual void AddScrollPositionListener(nsIScrollPositionListener* aListener) override {
    mHelper.AddScrollPositionListener(aListener);
  }
@ -1251,6 +1262,9 @@ public:
  virtual void ScrollToRestoredPosition() override {
    mHelper.ScrollToRestoredPosition();
  }
+  virtual void SetRestoringHistoryScrollPosition(bool aValue) override {
+    mHelper.SetRestoringHistoryScrollPosition(aValue);
+  }
  virtual void AddScrollPositionListener(nsIScrollPositionListener* aListener) override {
    mHelper.AddScrollPositionListener(aListener);
  }
--- a/layout/generic/nsIScrollableFrame.h
+++ b/layout/generic/nsIScrollableFrame.h
@ -287,6 +287,15 @@ public:
   */
  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.