Bug 1498812 - Part 11: Use Visual Viewport for storing scroll position in the PresState. r=botond,tnikkel

Differential Revision: https://phabricator.services.mozilla.com/D15691

--HG--
extra : moz-landing-system : lando

gfx/layers/apz/util/APZCCallbackHelper.cpp

@@ -174,9 +174,11 @@ static ScreenMargin ScrollFrame(nsIContent* aContent,
     sf->SetScrollableByAPZ(!aRequest.IsScrollInfoLayer());
     if (sf->IsRootScrollFrameOfDocument()) {
       if (nsCOMPtr<nsIPresShell> shell = GetPresShell(aContent)) {
-        shell->SetVisualViewportOffset(
-            CSSPoint::ToAppUnits(aRequest.GetScrollOffset()),
-            shell->GetLayoutViewportOffset());
+        if (shell->SetVisualViewportOffset(
+                CSSPoint::ToAppUnits(aRequest.GetScrollOffset()),
+                shell->GetLayoutViewportOffset())) {
+          sf->MarkEverScrolled();
+        }
       }
     }
   }

layout/base/PresState.ipdlh

@@ -36,6 +36,8 @@ union PresContentData {
 
 struct PresState {
   PresContentData contentData;
+  // For frames where layout and visual viewport aren't one and the same thing,
+  // scrollState will store the position of the *visual* viewport.
   nsPoint scrollState;
   bool allowScrollOriginDowngrade;
   float resolution;

layout/generic/nsGfxScrollFrame.cpp

@@ -2436,6 +2436,14 @@ static void RemoveDisplayPortCallback(nsITimer* aTimer, void* aClosure) {
   helper->mScrollableByAPZ = false;
 }
 
+void ScrollFrameHelper::MarkEverScrolled() {
+  // Mark this frame as having been scrolled. If this is the root
+  // scroll frame of a content document, then IsAlwaysActive()
+  // will return true from now on and MarkNotRecentlyScrolled() won't
+  // have any effect.
+  mHasBeenScrolled = true;
+}
+
 void ScrollFrameHelper::MarkNotRecentlyScrolled() {
   if (!mHasBeenScrolledRecently) return;
 
@@ -2500,11 +2508,7 @@ void ScrollFrameHelper::TriggerDisplayPortExpiration() {
 }
 
 void ScrollFrameHelper::ScrollVisual() {
-  // Mark this frame as having been scrolled. If this is the root
-  // scroll frame of a content document, then IsAlwaysActive()
-  // will return true from now on and MarkNotRecentlyScrolled() won't
-  // have any effect.
-  mHasBeenScrolled = true;
+  MarkEverScrolled();
 
   AdjustViews(mScrolledFrame);
   // We need to call this after fixing up the view positions
@@ -4319,11 +4323,11 @@ void ScrollFrameHelper::ScrollToRestoredPosition() {
   // all cases
 
   // if we didn't move, we still need to restore
-  if (GetLogicalScrollPosition() == mLastPos) {
+  if (GetLogicalVisualViewportOffset() == 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 /* GetLogicalScrollPosition() */) {
+    if (mRestorePos != mLastPos /* GetLogicalVisualViewportOffset() */) {
       LoadingState state = GetPageLoadingState();
       if (state == LoadingState::Stopped && !NS_SUBTREE_DIRTY(mOuter)) {
         return;
@@ -4331,8 +4335,8 @@ void ScrollFrameHelper::ScrollToRestoredPosition() {
       nsPoint scrollToPos = mRestorePos;
       if (!IsPhysicalLTR()) {
         // convert from logical to physical scroll position
-        scrollToPos.x = mScrollPort.x - (mScrollPort.XMost() - scrollToPos.x -
-                                         mScrolledFrame->GetRect().width);
+        scrollToPos.x -=
+            (GetVisualViewportSize().width - mScrolledFrame->GetRect().width);
       }
       AutoWeakFrame weakFrame(mOuter);
       // It's very important to pass nsGkAtoms::restore here, so
@@ -4348,7 +4352,7 @@ void ScrollFrameHelper::ScrollToRestoredPosition() {
         // 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();
+        mLastPos = GetLogicalVisualViewportOffset();
         return;
       }
     }
@@ -6140,7 +6144,7 @@ UniquePtr<PresState> ScrollFrameHelper::SaveState() const {
   // we'll jump straight to the end of the scroll animation, rather than
   // effectively dropping it. Note that the mRestorePos will override the
   // smooth scroll destination if both are present.
-  nsPoint pt = GetLogicalScrollPosition();
+  nsPoint pt = GetLogicalVisualViewportOffset();
   if (isInSmoothScroll) {
     pt = mDestination;
     allowScrollOriginDowngrade = false;
@@ -6163,7 +6167,7 @@ void ScrollFrameHelper::RestoreState(PresState* aState) {
   MOZ_ASSERT(mLastScrollOrigin == nsGkAtoms::other);
   mAllowScrollOriginDowngrade = aState->allowScrollOriginDowngrade();
   mDidHistoryRestore = true;
-  mLastPos = mScrolledFrame ? GetLogicalScrollPosition() : nsPoint(0, 0);
+  mLastPos = mScrolledFrame ? GetLogicalVisualViewportOffset() : nsPoint(0, 0);
 
   // Resolution properties should only exist on root scroll frames.
   MOZ_ASSERT(mIsRoot || aState->resolution() == 1.0);

layout/generic/nsGfxScrollFrame.h

@@ -220,6 +220,18 @@ class ScrollFrameHelper : public nsIReflowCallback {
   nsRect GetScrollRange(nscoord aWidth, nscoord aHeight) const;
   nsSize GetVisualViewportSize() const;
   nsPoint GetVisualViewportOffset() const;
+  /**
+   * For LTR frames, this is the same as GetVisualViewportOffset().
+   * For RTL frames, we take the offset from the top right corner of the frame
+   * to the top right corner of the visual viewport.
+   */
+  nsPoint GetLogicalVisualViewportOffset() const {
+    nsPoint pt = GetVisualViewportOffset();
+    if (!IsPhysicalLTR()) {
+      pt.x += GetVisualViewportSize().width - mScrolledFrame->GetRect().width;
+    }
+    return pt;
+  }
   void ScrollSnap(
       nsIScrollableFrame::ScrollMode aMode = nsIScrollableFrame::SMOOTH_MSD);
   void ScrollSnap(
@@ -410,6 +422,7 @@ class ScrollFrameHelper : public nsIReflowCallback {
   bool ShouldClampScrollPosition() const;
 
   bool IsAlwaysActive() const;
+  void MarkEverScrolled();
   void MarkRecentlyScrolled();
   void MarkNotRecentlyScrolled();
   nsExpirationState* GetExpirationState() { return &mActivityExpirationState; }
@@ -541,10 +554,15 @@ class ScrollFrameHelper : public nsIReflowCallback {
   // matches the current logical scroll position, we try to scroll to
   // mRestorePos after every reflow --- because after each time content is
   // loaded/added to the scrollable element, there will be a reflow.
+  // Note that for frames where layout and visual viewport aren't one and the
+  // same thing, this scroll position will be the logical scroll position of
+  // the *visual* viewport, as its position will be more relevant to the user.
   nsPoint mRestorePos;
   // The last logical position we scrolled to while trying to restore
   // mRestorePos, or 0,0 when this is a new frame. Set to -1,-1 once we've
   // scrolled for any reason other than trying to restore mRestorePos.
+  // Just as with mRestorePos, this position will be the logical position of
+  // the *visual* viewport where available.
   nsPoint mLastPos;
 
   // The latest scroll position we've sent or received from APZ. This
@@ -966,6 +984,7 @@ class nsHTMLScrollFrame : public nsContainerFrame,
   virtual void ClearDidHistoryRestore() override {
     mHelper.mDidHistoryRestore = false;
   }
+  virtual void MarkEverScrolled() override { mHelper.MarkEverScrolled(); }
   virtual bool IsRectNearlyVisible(const nsRect& aRect) override {
     return mHelper.IsRectNearlyVisible(aRect);
   }
@@ -1433,6 +1452,7 @@ class nsXULScrollFrame final : public nsBoxFrame,
   virtual void ClearDidHistoryRestore() override {
     mHelper.mDidHistoryRestore = false;
   }
+  virtual void MarkEverScrolled() override { mHelper.MarkEverScrolled(); }
   virtual bool IsRectNearlyVisible(const nsRect& aRect) override {
     return mHelper.IsRectNearlyVisible(aRect);
   }

layout/generic/nsIScrollableFrame.h

@@ -375,6 +375,11 @@ class nsIScrollableFrame : public nsIScrollbarMediator {
    * @see nsIStatefulFrame::RestoreState
    */
   virtual void ClearDidHistoryRestore() = 0;
+  /**
+   * Mark the frame as having been scrolled at least once, so that it remains
+   * active and we can also start storing its scroll position when saving state.
+   */
+  virtual void MarkEverScrolled() = 0;
   /**
    * Determine if the passed in rect is nearly visible according to the frame
    * visibility heuristics for how close it is to the visible scrollport.

mobile/android/tests/browser/chrome/test_session_scroll_visual_viewport.html

@@ -138,13 +138,13 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1498812
     index -= 1; // session history uses 1-based index
     let {entries} = tabData;
     let prevPage = entries[index - 1];
-    todo(prevPage.presState, "presState exists");
+    ok(prevPage.presState, "presState exists");
     if (prevPage.presState) {
       let presState = prevPage.presState[0];
       // The presState operates in app units, while all other scroll positions
       // in JS-land use CSS pixels.
       presState = presStateToCSSPx(presState);
-      todo_is(presState.scroll, getScrollString(scrollPos1), "stored scroll position for previous page is correct");
+      is(presState.scroll, getScrollString(scrollPos1), "stored scroll position for previous page is correct");
       ok(fuzzyEquals(presState.res, scrollPos1.zoom), "stored zoom level for previous page is correct");
     }
 
@@ -160,6 +160,18 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1498812
     // Check the scroll position and zoom level.
     checkScroll(browser, scrollPos2);
 
+    // Now go back in history and check that the scroll position
+    // is restored there as well.
+    is(browser.canGoBack, true, "can go back");
+    pageshow = promiseBrowserEvent(browser, "pageshow");
+    scroll = promiseBrowserEvent(browser, "mozvisualscroll",
+                                 { mozSystemGroup: true });
+    browser.goBack();
+    await pageshow;
+    await scroll;
+
+    checkScroll(browser, scrollPos1);
+
     // Remove the tab.
     cleanupTabs();
   });