Bug 1531057 - In ScrollToRestoredPosition(), clamp the layout scroll offset being restored to the layout scroll range. r=kats
authorBotond Ballo <botond@mozilla.com>
Sat, 16 Mar 2019 00:00:36 +0000
changeset 464897 e0bc9e7cdfe11ddf31b5e84dc152d8b95975f7c3
parent 464896 8d1828122cf28320b41d00baea5d3b8992c2f643
child 464898 9aebabe324dab2e881782658bb75e0c5257eb5cf
push id35727
push userdvarga@mozilla.com
push dateTue, 19 Mar 2019 09:48:59 +0000
treeherdermozilla-central@70baa37ae1eb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskats
bugs1531057
milestone68.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 1531057 - In ScrollToRestoredPosition(), clamp the layout scroll offset being restored to the layout scroll range. r=kats Differential Revision: https://phabricator.services.mozilla.com/D23725
layout/generic/nsGfxScrollFrame.cpp
--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@@ -4450,53 +4450,64 @@ void ScrollFrameHelper::ScrollToRestored
   // make sure our scroll position did not change for where we last put
   // it. if it does then the user must have moved it, and we no longer
   // need to restore.
   //
   // In the RTL case, we check whether the scroll position changed using the
   // logical scroll position, but we scroll to the physical scroll position in
   // all cases
 
+  // The layout offset we want to restore is the same as the visual offset
+  // (for now, may change in bug 1499210), but clamped to the layout scroll
+  // range (which can be a subset of the visual scroll range).
+  // Note that we can't do the clamping when initializing mRestorePos in
+  // RestoreState(), since the scrollable rect (which the clamping depends
+  // on) can change over the course of the restoration process.
+  nsPoint layoutRestorePos = GetScrollRange().ClampPoint(mRestorePos);
+
   // Continue restoring until both the layout and visual scroll positions
   // reach the destination. (Note that the two can only be different for
   // the root content document's root scroll frame, and when zoomed in).
   // This is necessary to avoid situations where the two offsets get stuck
   // at different values and nothing reconciles them (see bug 1519621 comment
   // 8).
   nsPoint logicalLayoutScrollPos = GetLogicalScrollPosition();
 
   // if we didn't move, we still need to restore
   if (GetLogicalVisualViewportOffset() == mLastPos ||
       logicalLayoutScrollPos == mLastPos) {
     // if our desired position is different to the scroll position, scroll.
     // remember that we could be incrementally loading so we may enter
     // and scroll many times.
     if (mRestorePos != mLastPos /* GetLogicalVisualViewportOffset() */ ||
-        mRestorePos != logicalLayoutScrollPos) {
+        layoutRestorePos != logicalLayoutScrollPos) {
       LoadingState state = GetPageLoadingState();
       if (state == LoadingState::Stopped && !NS_SUBTREE_DIRTY(mOuter)) {
         return;
       }
-      nsPoint scrollToPos = mRestorePos;
+      nsPoint visualScrollToPos = mRestorePos;
+      nsPoint layoutScrollToPos = layoutRestorePos;
       if (!IsPhysicalLTR()) {
         // convert from logical to physical scroll position
-        scrollToPos.x -=
+        visualScrollToPos.x -=
+            (GetVisualViewportSize().width - mScrolledFrame->GetRect().width);
+        layoutScrollToPos.x -=
             (GetVisualViewportSize().width - mScrolledFrame->GetRect().width);
       }
       AutoWeakFrame weakFrame(mOuter);
       // It's very important to pass nsGkAtoms::restore here, so
       // ScrollToWithOrigin won't clear out mRestorePos.
-      ScrollToWithOrigin(scrollToPos, nsIScrollableFrame::INSTANT,
+      ScrollToWithOrigin(layoutScrollToPos, nsIScrollableFrame::INSTANT,
                          nsGkAtoms::restore, nullptr);
       if (!weakFrame.IsAlive()) {
         return;
       }
       if (mIsRoot && mOuter->PresContext()->IsRootContentDocument()) {
         mOuter->PresShell()->SetPendingVisualScrollUpdate(
-            scrollToPos, FrameMetrics::eRestore);
+            visualScrollToPos, FrameMetrics::eRestore);
       }
       if (state == LoadingState::Loading || NS_SUBTREE_DIRTY(mOuter)) {
         // 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 = GetLogicalVisualViewportOffset();