Bug 1531057 - In ScrollToRestoredPosition(), clamp the layout scroll offset being restored to the layout scroll range. r=kats a=pascalc
authorBotond Ballo <botond@mozilla.com>
Sat, 16 Mar 2019 00:00:36 +0000
changeset 525747 d5a9d2da215fe31f734b047cdfdb12fbc5afd732
parent 525746 56567a8607b5cf0e804c737a9fcd03a422e3fdf4
child 525748 d86ab22b61a1f8bf3cc1484deb69a24f0bef6578
push id2032
push userffxbld-merge
push dateMon, 13 May 2019 09:36:57 +0000
treeherdermozilla-release@455c1065dcbe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskats, pascalc
bugs1531057
milestone67.0
Bug 1531057 - In ScrollToRestoredPosition(), clamp the layout scroll offset being restored to the layout scroll range. r=kats a=pascalc 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();