From 79fe5e1e560708dc174e93af15d07cfde7cfff25 Mon Sep 17 00:00:00 2001 From: Jan Henning Date: Fri, 11 Jan 2019 19:50:24 +0000 Subject: [PATCH] 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 | 8 ++++-- layout/base/PresState.ipdlh | 2 ++ layout/generic/nsGfxScrollFrame.cpp | 28 +++++++++++-------- layout/generic/nsGfxScrollFrame.h | 20 +++++++++++++ layout/generic/nsIScrollableFrame.h | 5 ++++ .../test_session_scroll_visual_viewport.html | 16 +++++++++-- 6 files changed, 62 insertions(+), 17 deletions(-) diff --git a/gfx/layers/apz/util/APZCCallbackHelper.cpp b/gfx/layers/apz/util/APZCCallbackHelper.cpp index 162abc1f6588..3cd86ab34770 100644 --- a/gfx/layers/apz/util/APZCCallbackHelper.cpp +++ b/gfx/layers/apz/util/APZCCallbackHelper.cpp @@ -174,9 +174,11 @@ static ScreenMargin ScrollFrame(nsIContent* aContent, sf->SetScrollableByAPZ(!aRequest.IsScrollInfoLayer()); if (sf->IsRootScrollFrameOfDocument()) { if (nsCOMPtr shell = GetPresShell(aContent)) { - shell->SetVisualViewportOffset( - CSSPoint::ToAppUnits(aRequest.GetScrollOffset()), - shell->GetLayoutViewportOffset()); + if (shell->SetVisualViewportOffset( + CSSPoint::ToAppUnits(aRequest.GetScrollOffset()), + shell->GetLayoutViewportOffset())) { + sf->MarkEverScrolled(); + } } } } diff --git a/layout/base/PresState.ipdlh b/layout/base/PresState.ipdlh index 65f6ffd3b2fe..43bc5369cccf 100644 --- a/layout/base/PresState.ipdlh +++ b/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; diff --git a/layout/generic/nsGfxScrollFrame.cpp b/layout/generic/nsGfxScrollFrame.cpp index a5042c087aa5..59e477a776d0 100644 --- a/layout/generic/nsGfxScrollFrame.cpp +++ b/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 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); diff --git a/layout/generic/nsGfxScrollFrame.h b/layout/generic/nsGfxScrollFrame.h index 1a4bf36d3938..683f8d1cb06a 100644 --- a/layout/generic/nsGfxScrollFrame.h +++ b/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); } diff --git a/layout/generic/nsIScrollableFrame.h b/layout/generic/nsIScrollableFrame.h index 1008c3f5a65a..e753e698c94b 100644 --- a/layout/generic/nsIScrollableFrame.h +++ b/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. diff --git a/mobile/android/tests/browser/chrome/test_session_scroll_visual_viewport.html b/mobile/android/tests/browser/chrome/test_session_scroll_visual_viewport.html index 5116c2492007..9358a11e830a 100644 --- a/mobile/android/tests/browser/chrome/test_session_scroll_visual_viewport.html +++ b/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(); });