Bug 1498812 - Part 9: Switch session store/session history to use visual viewport for scroll position tracking. r=mikedeboer,snorp
authorJan Henning <jh+bugzilla@buttercookie.de>
Fri, 11 Jan 2019 19:50:09 +0000
changeset 453553 39207d39e5c2d3a8980bc910820c8b37f812b77e
parent 453552 2d9a52630c0414f20acc825f7e8dbc931965e908
child 453554 f0f5124781cc6bc5d517b2d2bb9a499b15e06b07
push id35360
push usernbeleuzu@mozilla.com
push dateSat, 12 Jan 2019 09:39:47 +0000
treeherdermozilla-central@cb35977ae7a4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmikedeboer, snorp
bugs1498812, 1499210
milestone66.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 1498812 - Part 9: Switch session store/session history to use visual viewport for scroll position tracking. r=mikedeboer,snorp For simplicity's sake, for now we keep storing only one scroll position per history entry (bug 1499210), so if we have to choose between the layout and the visual viewport, the latter is a vastly better choice, as it more accurately represents the scroll position as perceived by the user, especially when the page has been pinch-zoomed. This also means that instead of the normal scroll events, the session store now has to listen for the corresponding events specific to the visual viewport. We also extend the scroll position test to check that the scroll position isn't just properly saved, but also actually properly restored in practice as well. We only add this test now instead of already adding it beforehand like we did with the rest of the test - to avoid having to temporarily extend the checkScroll() helper function to deal with todo()/todo_is etc. - because getting that part of the test to complete without timing out (which would be one of its natural failure modes, because the expected events would be missing) would require faking even more scroll events - because we already have the todo() tests that are telling us the we didn't *store* any scroll position in the first place, so there's no point in trying to actually restore anything For the GeckoView saveAndRestoreState test, we now spin the event loop once before setting the scroll position in order to give APZ opportunity to settle down after the initial page load. Differential Revision: https://phabricator.services.mozilla.com/D15690
browser/components/sessionstore/ContentSessionStore.jsm
browser/components/sessionstore/test/content.js
docshell/base/nsDocShell.cpp
docshell/shistory/nsISHEntry.idl
mobile/android/components/SessionStore.js
mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ContentDelegateTest.kt
mobile/android/tests/browser/chrome/head.js
mobile/android/tests/browser/chrome/head_scroll.js
mobile/android/tests/browser/chrome/test_session_scroll_position.html
mobile/android/tests/browser/chrome/test_session_scroll_visual_viewport.html
toolkit/components/sessionstore/nsSessionStoreUtils.cpp
--- a/browser/components/sessionstore/ContentSessionStore.jsm
+++ b/browser/components/sessionstore/ContentSessionStore.jsm
@@ -324,17 +324,18 @@ SessionHistoryListener.prototype.QueryIn
  *
  * Example:
  *   {scroll: "100,100", children: [null, null, {scroll: "200,200"}]}
  */
 class ScrollPositionListener extends Handler {
   constructor(store) {
     super(store);
 
-    ssu.addDynamicFrameFilteredListener(this.mm, "scroll", this, false);
+    ssu.addDynamicFrameFilteredListener(this.mm, "mozvisualscroll", this,
+                                        /* capture */ false, /* system group */ true);
     this.stateChangeNotifier.addObserver(this);
   }
 
   handleEvent() {
     this.messageQueue.push("scroll", () => this.collect());
   }
 
   onPageLoadCompleted() {
--- a/browser/components/sessionstore/test/content.js
+++ b/browser/components/sessionstore/test/content.js
@@ -119,34 +119,36 @@ addMessageListener("ss-test:setUsePrivat
   sendAsyncMessage("ss-test:setUsePrivateBrowsing");
 });
 
 addMessageListener("ss-test:getScrollPosition", function(msg) {
   let frame = content;
   if (msg.data.hasOwnProperty("frame")) {
     frame = content.frames[msg.data.frame];
   }
-  let {scrollX: x, scrollY: y} = frame;
-  sendAsyncMessage("ss-test:getScrollPosition", {x, y});
+  let x = {}, y = {};
+  frame.windowUtils.getVisualViewportOffset(x, y);
+  sendAsyncMessage("ss-test:getScrollPosition", {x: x.value, y: y.value});
 });
 
 addMessageListener("ss-test:setScrollPosition", function(msg) {
   let frame = content;
   let {x, y} = msg.data;
   if (msg.data.hasOwnProperty("frame")) {
     frame = content.frames[msg.data.frame];
   }
   frame.scrollTo(x, y);
 
-  frame.addEventListener("scroll", function onScroll(event) {
-    if (frame.document == event.target) {
-      frame.removeEventListener("scroll", onScroll);
+  frame.addEventListener("mozvisualscroll", function onScroll(event) {
+    if (frame.document.ownerGlobal.visualViewport == event.target) {
+      frame.removeEventListener("mozvisualscroll", onScroll,
+                                { mozSystemGroup: true });
       sendAsyncMessage("ss-test:setScrollPosition");
     }
-  });
+  }, { mozSystemGroup: true });
 });
 
 addMessageListener("ss-test:click", function({data}) {
   content.document.getElementById(data.id).click();
   sendAsyncMessage("ss-test:click");
 });
 
 addEventListener("load", function(event) {
--- a/docshell/base/nsDocShell.cpp
+++ b/docshell/base/nsDocShell.cpp
@@ -5621,17 +5621,17 @@ nsDocShell::SetTitle(const nsAString& aT
   }
 
   return NS_OK;
 }
 
 nsPoint nsDocShell::GetCurScrollPos() {
   nsPoint scrollPos;
   if (nsIScrollableFrame* sf = GetRootScrollFrame()) {
-    scrollPos = sf->GetScrollPosition();
+    scrollPos = sf->GetVisualViewportOffset();
   }
   return scrollPos;
 }
 
 nsresult nsDocShell::SetCurScrollPosEx(int32_t aCurHorizontalPos,
                                        int32_t aCurVerticalPos) {
   nsIScrollableFrame* sf = GetRootScrollFrame();
   NS_ENSURE_TRUE(sf, NS_ERROR_FAILURE);
--- a/docshell/shistory/nsISHEntry.idl
+++ b/docshell/shistory/nsISHEntry.idl
@@ -221,17 +221,20 @@ interface nsISHEntry : nsISupports
 
     /**
      * When an entry is serving is within nsISHistory's array of entries, this
      * property specifies if it should persist. If not it will be replaced by
      * new additions to the list.
      */
     [infallible] attribute boolean persist;
 
-    /** Set/Get scrollers' position in anchored pages */
+    /**
+     * Set/Get the visual viewport scroll position if session history is
+     * changed through anchor navigation or pushState.
+     */
     void setScrollPosition(in long x, in long y);
     void getScrollPosition(out long x, out long y);
 
     /**
      * Saved position and dimensions of the content viewer; we must adjust the
      * root view's widget accordingly if this has changed when the presentation
      * is restored.
      */
--- a/mobile/android/components/SessionStore.js
+++ b/mobile/android/components/SessionStore.js
@@ -541,18 +541,18 @@ SessionStore.prototype = {
           this._formdataSavePending =
             window.setTimeout(() => {
               this._formdataSavePending = null;
               this.onTabInput(window, browser);
             }, 2000);
         }
         break;
       }
-      case "resize":
-      case "scroll": {
+      case "mozvisualresize":
+      case "mozvisualscroll": {
         let browser = aEvent.currentTarget;
         // Duplicated logging check to avoid calling getTabForBrowser on each scroll event.
         if (loggingEnabled) {
           log(aEvent.type + " for tab " + window.BrowserApp.getTabForBrowser(browser).id);
         }
         if (!this._scrollSavePending) {
           this._scrollSavePending =
             window.setTimeout(() => {
@@ -645,18 +645,20 @@ SessionStore.prototype = {
     aBrowser.addEventListener("pageshow", this, true);
     aBrowser.addEventListener("AboutReaderContentReady", this, true);
 
     // Use a combination of events to watch for text data changes
     aBrowser.addEventListener("input", this, true);
     aBrowser.addEventListener("DOMAutoComplete", this, true);
 
     // Record the current scroll position and zoom level.
-    aBrowser.addEventListener("scroll", this, true);
-    aBrowser.addEventListener("resize", this, true);
+    aBrowser.addEventListener("mozvisualscroll", this,
+                              { capture: true, mozSystemGroup: true });
+    aBrowser.addEventListener("mozvisualresize", this,
+                              { capture: true, mozSystemGroup: true });
 
     log("onTabAdd() ran for tab " + aWindow.BrowserApp.getTabForBrowser(aBrowser).id +
         ", aNoNotification = " + aNoNotification);
     if (!aNoNotification) {
       if (this._loadState == STATE_QUITTING) {
         // A tab arrived just as were starting to shut down. Since we haven't yet received
         // application-quit, we refresh the window data one more time before the window closes.
         this._forEachBrowserWindow((aWindow) => {
@@ -671,18 +673,20 @@ SessionStore.prototype = {
   onTabRemove(aWindow, aBrowser, aNoNotification) {
     // Cleanup event listeners
     aBrowser.removeEventListener("DOMTitleChanged", this, true);
     aBrowser.removeEventListener("load", this, true);
     aBrowser.removeEventListener("pageshow", this, true);
     aBrowser.removeEventListener("AboutReaderContentReady", this, true);
     aBrowser.removeEventListener("input", this, true);
     aBrowser.removeEventListener("DOMAutoComplete", this, true);
-    aBrowser.removeEventListener("scroll", this, true);
-    aBrowser.removeEventListener("resize", this, true);
+    aBrowser.removeEventListener("mozvisualscroll", this,
+                                 { capture: true, mozSystemGroup: true });
+    aBrowser.removeEventListener("mozvisualresize", this,
+                                 { capture: true, mozSystemGroup: true });
 
     if (aBrowser.__SS_historyChange) {
       aWindow.clearTimeout(aBrowser.__SS_historyChange);
       delete aBrowser.__SS_historyChange;
     }
 
     delete aBrowser.__SS_data;
 
--- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ContentDelegateTest.kt
+++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ContentDelegateTest.kt
@@ -169,17 +169,18 @@ class ContentDelegateTest : BaseSessionT
 
     @WithDevToolsAPI
     @WithDisplay(width = 400, height = 400)
     @Test fun saveAndRestoreState() {
         val startUri = createTestUrl(SAVE_STATE_PATH)
         mainSession.loadUri(startUri)
         sessionRule.waitForPageStop()
 
-        mainSession.evaluateJS("$('#name').value = 'the name'; window.scrollBy(0, 100);")
+        mainSession.evaluateJS("$('#name').value = 'the name'; window.setTimeout(() => window.scrollBy(0, 100),0);")
+        sessionRule.waitUntilCalled(Callbacks.ScrollDelegate::class, "onScrollChanged")
 
         val state = sessionRule.waitForResult(mainSession.saveState())
         assertThat("State should not be null", state, notNullValue())
 
         mainSession.loadUri("about:blank")
         sessionRule.waitForPageStop()
 
         mainSession.restoreState(state)
@@ -192,17 +193,17 @@ class ContentDelegateTest : BaseSessionT
             }
         })
 
         assertThat("'name' field should match",
                 mainSession.evaluateJS("$('#name').value").toString(),
                 equalTo("the name"))
 
         assertThat("Scroll position should match",
-                mainSession.evaluateJS("window.scrollY") as Double,
+                mainSession.evaluateJS("window.visualViewport.pageTop") as Double,
                 closeTo(100.0, .5))
     }
 
     @Test fun saveStateSync() {
         val startUri = createTestUrl(SAVE_STATE_PATH)
         mainSession.loadUri(startUri)
         sessionRule.waitForPageStop()
 
--- a/mobile/android/tests/browser/chrome/head.js
+++ b/mobile/android/tests/browser/chrome/head.js
@@ -4,19 +4,20 @@
 function promiseBrowserEvent(browserOrFrame, eventType, options = {}) {
   let listenerOptions = { capture: true };
   if (options.mozSystemGroup) {
     listenerOptions.mozSystemGroup = true;
   }
   return new Promise((resolve) => {
     function handle(event) {
       // Since we'll be redirecting, don't make assumptions about the given URL and the loaded URL
-      if (event.target != (browserOrFrame.contentDocument || browserOrFrame.document) ||
-                          event.target.location.href == "about:blank") {
-        info("Skipping spurious '" + eventType + "' event" + " for " + event.target.location.href);
+      let document = browserOrFrame.contentDocument || browserOrFrame.document;
+      if (event.target != document && event.target != document.ownerGlobal.visualViewport ||
+          document.location.href == "about:blank") {
+        info("Skipping spurious '" + eventType + "' event" + " for " + document.location.href);
         return;
       }
       info("Received event " + eventType + " from browser");
       browserOrFrame.removeEventListener(eventType, handle, listenerOptions);
       if (options.resolveAtNextTick) {
         Services.tm.dispatchToMainThread(() => resolve(event));
       } else {
         resolve(event);
--- a/mobile/android/tests/browser/chrome/head_scroll.js
+++ b/mobile/android/tests/browser/chrome/head_scroll.js
@@ -36,27 +36,22 @@ function checkScroll(browser, data) {
   }
   if (zoom) {
     ok(fuzzyEquals(scrollPos.zoom, zoom), "zoom set correctly");
   }
 }
 
 function getScrollPosition(browser, data = {}) {
   let utils = getFrame(browser, data).windowUtils;
-  let visualScrollPos = data.visualScrollPos === true;
   let x = {}, y = {};
 
   let zoom = utils.getResolution();
-  if (visualScrollPos) {
-    utils.getVisualViewportOffset(x, y);
-  } else {
-    utils.getScrollXY(false, x, y);
-  }
+  utils.getVisualViewportOffset(x, y);
 
-  return { x: x.value, y: y.value, zoom, visualScrollPos };
+  return { x: x.value, y: y.value, zoom };
 }
 
 function getScrollString({ x = 0, y = 0 }) {
   return x + "," + y;
 }
 
 function presStateToCSSPx(presState) {
   // App units aren't commonly exposed to JS, so we can't just call a helper function
--- a/mobile/android/tests/browser/chrome/test_session_scroll_position.html
+++ b/mobile/android/tests/browser/chrome/test_session_scroll_position.html
@@ -144,28 +144,30 @@ https://bugzilla.mozilla.org/show_bug.cg
     let [{scrolldata}] = state;
     is(scrolldata.scroll, getScrollString(testData2), "stored scroll position is correct");
     ok(fuzzyEquals(scrolldata.zoom.resolution, ZOOM2), "stored zoom level is correct");
 
     // Restore the closed tab.
     let closedTabData = ss.getClosedTabs(chromeWin)[0];
     let browser = ss.undoCloseTab(chromeWin, closedTabData);
     let pageshow = promiseBrowserEvent(browser, "pageshow");
-    let scroll = promiseBrowserEvent(browser, "scroll");
+    let scroll = promiseBrowserEvent(browser, "mozvisualscroll",
+                                     { mozSystemGroup: true });
     await pageshow;
     await scroll;
 
     // Check the scroll position and zoom level.
     checkScroll(browser, testData2);
 
     // 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, "scroll");
+    scroll = promiseBrowserEvent(browser, "mozvisualscroll",
+                                 { mozSystemGroup: true });
     browser.goBack();
     await pageshow;
     await scroll;
 
     checkScroll(browser, testData1);
 
     // Remove the tab.
     BrowserApp.closeTab(BrowserApp.getTabForBrowser(browser));
@@ -261,19 +263,25 @@ https://bugzilla.mozilla.org/show_bug.cg
 
     // Restore the closed tab.
     let closedTabData = ss.getClosedTabs(chromeWin)[0];
     let browser = ss.undoCloseTab(chromeWin, closedTabData);
     let pageshow = promiseBrowserEvent(browser, "pageshow");
     // We can't add event listeners for the frames until we're sure that they've actually loaded.
     let load = promiseBrowserEvent(browser, "load");
     await load;
-    let scrollParent = promiseBrowserEvent(getFrame(browser, testDataParent), "scroll");
-    let scroll1 = promiseBrowserEvent(getFrame(browser, testData1), "scroll");
-    let scroll2 = promiseBrowserEvent(getFrame(browser, testData2), "scroll");
+    let scrollParent = promiseBrowserEvent(getFrame(browser, testDataParent),
+                                           "mozvisualscroll",
+                                           { mozSystemGroup: true });
+    let scroll1 = promiseBrowserEvent(getFrame(browser, testData1),
+                                      "mozvisualscroll",
+                                      { mozSystemGroup: true });
+    let scroll2 = promiseBrowserEvent(getFrame(browser, testData2),
+                                      "mozvisualscroll",
+                                      { mozSystemGroup: true });
     await pageshow;
     await scrollParent;
     await scroll1;
     await scroll2;
 
     // Check the scroll position and zoom level.
     checkScroll(browser, testDataParent);
     checkScroll(browser, testData1);
--- a/mobile/android/tests/browser/chrome/test_session_scroll_visual_viewport.html
+++ b/mobile/android/tests/browser/chrome/test_session_scroll_visual_viewport.html
@@ -91,76 +91,82 @@ https://bugzilla.mozilla.org/show_bug.cg
         setScrollPosition(browser, zoomIn);
         await scrolled;
 
         // Check that we've actually zoomed.
         checkScroll(browser, zoomIn);
 
         scrolled = promiseTabEvent(browser, "SSTabScrollCaptured");
         await scrollRight(browser.contentWindow);
-
-        // The above scroll only scrolled the visual viewport, not the layout
-        // viewport. As long as we don't have a scroll event for the visual
-        // viewport, we need to fake a scroll event here in order to induce
-        // the session store to record the scroll position again (bug 1500554).
-        browser.dispatchEvent(new browser.contentWindow.UIEvent("scroll"));
         await scrolled;
 
-        scrollPos1 = getScrollPosition(browser, {visualScrollPos: true});
+        scrollPos1 = getScrollPosition(browser);
         isnot(scrollPos1.x, 0, "we should be scrolled to the right");
         is(scrollPos1.y, 0, "we scrolled horizontally");
 
         // Navigate to a different page and scroll/zoom there as well.
         browser.loadURI(URL2);
         await promiseBrowserEvent(browser, "pageshow");
 
         scrolled = promiseTabEvent(browser, "SSTabScrollCaptured");
         setScrollPosition(browser, zoomIn);
         await scrolled;
         checkScroll(browser, zoomIn);
 
         scrolled = promiseTabEvent(browser, "SSTabScrollCaptured");
         await scrollRight(browser.contentWindow);
-
-        // Fake a scroll event (see above).
-        browser.dispatchEvent(new browser.contentWindow.UIEvent("scroll"));
         await scrolled;
 
-        scrollPos2 = getScrollPosition(browser, {visualScrollPos: true});
+        scrollPos2 = getScrollPosition(browser);
         isnot(scrollPos2.x, 0, "we should be scrolled to the right");
         is(scrollPos2.y, 0, "we scrolled horizontally");
 
         // Remove the tab.
         let closed = promiseTabEvent(browser, "SSTabCloseProcessed");
         BrowserApp.closeTab(tabScroll);
         await closed;
     }
 
     await createAndRemoveTab();
 
     // Check the live scroll data for the current history entry...
     let tabData = ss.getClosedTabs(chromeWin)[0];
     let {scrolldata} = tabData;
-    todo_is(scrolldata.scroll, getScrollString(scrollPos2), "stored scroll position is correct");
+    is(scrolldata.scroll, getScrollString(scrollPos2), "stored scroll position is correct");
     ok(fuzzyEquals(scrolldata.zoom.resolution, scrollPos2.zoom), "stored zoom level is correct");
 
     // ... and the presState from the previous history entry.
     let {index} = tabData;
     index -= 1; // session history uses 1-based index
     let {entries} = tabData;
     let prevPage = entries[index - 1];
     todo(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");
       ok(fuzzyEquals(presState.res, scrollPos1.zoom), "stored zoom level for previous page is correct");
     }
+
+    // Restore the closed tab.
+    let browser = ss.undoCloseTab(chromeWin, tabData);
+    tabScroll = BrowserApp.getTabForBrowser(browser);
+    let pageshow = promiseBrowserEvent(browser, "pageshow");
+    let scroll = promiseBrowserEvent(browser, "mozvisualscroll",
+                                     { mozSystemGroup: true });
+    await pageshow;
+    await scroll;
+
+    // Check the scroll position and zoom level.
+    checkScroll(browser, scrollPos2);
+
+    // Remove the tab.
+    cleanupTabs();
   });
 
   </script>
 </head>
 <body>
 <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1498812">Mozilla Bug 1498812</a>
 <p id="display"></p>
 <div id="content" style="display: none">
--- a/toolkit/components/sessionstore/nsSessionStoreUtils.cpp
+++ b/toolkit/components/sessionstore/nsSessionStoreUtils.cpp
@@ -249,22 +249,17 @@ nsSessionStoreUtils::CollectScrollPositi
                                            nsACString& aRet) {
   aRet.Truncate();
 
   nsIPresShell* presShell = aDocument->GetShell();
   if (!presShell) {
     return NS_OK;
   }
 
-  nsIScrollableFrame* frame = presShell->GetRootScrollFrameAsScrollable();
-  if (!frame) {
-    return NS_OK;
-  }
-
-  nsPoint scrollPos = frame->GetScrollPosition();
+  nsPoint scrollPos = presShell->GetVisualViewportOffset();
   int scrollX = nsPresContext::AppUnitsToIntCSSPixels(scrollPos.x);
   int scrollY = nsPresContext::AppUnitsToIntCSSPixels(scrollPos.y);
 
   if ((scrollX != 0) || (scrollY != 0)) {
     const nsPrintfCString position("%d,%d", scrollX, scrollY);
     aRet.Assign(position);
   }