MD Settings: Prevent unexpected scrolling to section on popstates.

On subpage-backs or Browser back buttons, prevent the MainPageBehavior
from scrolling to the position of the last routes' section.

This makes section routes behave more like #anchorLinks.

BUG=640523
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2297663008
Cr-Commit-Position: refs/heads/master@{#416386}

chrome/browser/resources/settings/route.js

@@ -253,6 +253,9 @@ cr.define('settings', function() {
    */
   var currentQueryParameters_ = new URLSearchParams();
 
+  /** @private {boolean} */
+  var lastRouteChangeWasPopstate_ = false;
+
   /** @private */
   var initializeRouteFromUrlCalled_ = false;
 
@@ -277,11 +280,13 @@ cr.define('settings', function() {
    * Helper function to set the current route and notify all observers.
    * @param {!settings.Route} route
    * @param {!URLSearchParams} queryParameters
+   * @param {boolean} isPopstate
    */
-  var setCurrentRoute = function(route, queryParameters) {
+  var setCurrentRoute = function(route, queryParameters, isPopstate) {
     var oldRoute = currentRoute_;
     currentRoute_ = route;
     currentQueryParameters_ = queryParameters;
+    lastRouteChangeWasPopstate_ = isPopstate;
     for (var observer of routeObservers_)
       observer.currentRouteChanged(currentRoute_, oldRoute);
   };
@@ -294,6 +299,11 @@ cr.define('settings', function() {
     return new URLSearchParams(currentQueryParameters_);  // Defensive copy.
   };
 
+  /** @return {boolean} */
+  var lastRouteChangeWasPopstate = function() {
+    return lastRouteChangeWasPopstate_;
+  };
+
   /**
    * Navigates to a canonical route and pushes a new history entry.
    * @param {!settings.Route} route
@@ -312,7 +322,7 @@ cr.define('settings', function() {
 
     // History serializes the state, so we don't push the actual route object.
     window.history.pushState(currentRoute_.path, '', url);
-    setCurrentRoute(route, params);
+    setCurrentRoute(route, params, false);
   };
 
   /**
@@ -334,7 +344,7 @@ cr.define('settings', function() {
   window.addEventListener('popstate', function(event) {
     // On pop state, do not push the state onto the window.history again.
     setCurrentRoute(getRouteForPath(window.location.pathname) || Route.BASIC,
-                    new URLSearchParams(window.location.search));
+                    new URLSearchParams(window.location.search), true);
   });
 
   return {
@@ -344,6 +354,7 @@ cr.define('settings', function() {
     initializeRouteFromUrl: initializeRouteFromUrl,
     getCurrentRoute: getCurrentRoute,
     getQueryParameters: getQueryParameters,
+    lastRouteChangeWasPopstate: lastRouteChangeWasPopstate,
     navigateTo: navigateTo,
     navigateToPreviousRoute: navigateToPreviousRoute,
   };

chrome/browser/resources/settings/settings_page/main_page_behavior.js

@@ -94,14 +94,14 @@ var MainPageBehaviorImpl = {
       if (!currentRoute.isSubpage() || expandedSection != currentSection) {
         promise = this.collapseSection_(expandedSection);
         // Scroll to the collapsed section.
-        if (currentSection)
+        if (currentSection && !settings.lastRouteChangeWasPopstate())
           currentSection.scrollIntoView();
       }
     } else if (currentSection) {
       // Expand the section into a subpage or scroll to it on the main page.
       if (currentRoute.isSubpage())
         promise = this.expandSection_(currentSection);
-      else
+      else if (!settings.lastRouteChangeWasPopstate())
         currentSection.scrollIntoView();
     }
 
@@ -234,13 +234,7 @@ var MainPageBehaviorImpl = {
         var newSection = settings.getCurrentRoute().section &&
             this.getSection(settings.getCurrentRoute().section);
 
-        // Scroll to the section if indicated by the route. TODO(michaelpg): Is
-        // this the right behavior, or should we return to the previous scroll
-        // position?
-        if (newSection)
-          newSection.scrollIntoView();
-        else
-          this.scroller.scrollTop = this.origScrollTop_;
+        this.scroller.scrollTop = this.origScrollTop_;
 
         this.currentAnimation_ = section.animateCollapse(
             /** @type {!HTMLElement} */(this.scroller));

chrome/test/data/webui/settings/route_tests.js

@@ -3,6 +3,42 @@
 // found in the LICENSE file.
 
 suite('route', function() {
+  /**
+   * Returns a new promise that resolves after a window 'popstate' event.
+   * @return {!Promise}
+   */
+  function whenPopState(causeEvent) {
+    var promise = new Promise(function(resolve) {
+      window.addEventListener('popstate', function callback() {
+        window.removeEventListener('popstate', callback);
+        resolve();
+      });
+    });
+
+    causeEvent();
+    return promise;
+  }
+
+  /**
+   * Tests a specific navigation situation.
+   * @param {!settings.Route} previousRoute
+   * @param {!settings.Route} currentRoute
+   * @param {!settings.Route} expectedNavigatePreviousResult
+   * @return {!Promise}
+   */
+  function testNavigateBackUsesHistory(previousRoute, currentRoute,
+                                       expectedNavigatePreviousResult) {
+    settings.navigateTo(previousRoute);
+    settings.navigateTo(currentRoute);
+
+    return whenPopState(function() {
+      settings.navigateToPreviousRoute();
+    }).then(function() {
+      assertEquals(expectedNavigatePreviousResult,
+                   settings.getCurrentRoute());
+    });
+  };
+
   test('tree structure', function() {
     // Set up root page routes.
     var BASIC = new settings.Route('/');
@@ -55,38 +91,6 @@ suite('route', function() {
     });
   });
 
-  /**
-   * Tests a specific navigation situation.
-   * @param {!settings.Route} previousRoute
-   * @param {!settings.Route} currentRoute
-   * @param {!settings.Route} expectedNavigatePreviousResult
-   * @return {!Promise}
-   */
-  function testNavigateBackUsesHistory(previousRoute, currentRoute,
-                                       expectedNavigatePreviousResult) {
-    /**
-     * Returns a new promise that resolves after a window 'popstate' event.
-     * @return {!Promise}
-     */
-    function whenPopState() {
-      return new Promise(function(resolve) {
-        window.addEventListener('popstate', function callback() {
-          window.removeEventListener('popstate', callback);
-          resolve();
-        });
-      });
-    }
-
-    settings.navigateTo(previousRoute);
-    settings.navigateTo(currentRoute);
-    settings.navigateToPreviousRoute();
-
-    return whenPopState().then(function() {
-      assertEquals(expectedNavigatePreviousResult,
-                   settings.getCurrentRoute());
-    });
-  };
-
   test('navigate back to parent previous route', function() {
     return testNavigateBackUsesHistory(
         settings.Route.BASIC,
@@ -121,4 +125,22 @@ suite('route', function() {
     settings.navigateToPreviousRoute();
     assertEquals(settings.Route.BASIC, settings.getCurrentRoute());
   });
+
+  test('popstate flag works', function() {
+    settings.navigateTo(settings.Route.BASIC);
+    assertFalse(settings.lastRouteChangeWasPopstate());
+
+    settings.navigateTo(settings.Route.PEOPLE);
+    assertFalse(settings.lastRouteChangeWasPopstate());
+
+    return whenPopState(function() {
+      window.history.back();
+    }).then(function() {
+      assertEquals(settings.Route.BASIC, settings.getCurrentRoute());
+      assertTrue(settings.lastRouteChangeWasPopstate());
+
+      settings.navigateTo(settings.Route.ADVANCED);
+      assertFalse(settings.lastRouteChangeWasPopstate());
+    });
+  });
 });