MD Settings: Prevent unexpected scrolling to section on popstates.

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

Index: chrome/browser/resources/settings/settings_page/main_page_behavior.js
diff --git a/chrome/browser/resources/settings/settings_page/main_page_behavior.js b/chrome/browser/resources/settings/settings_page/main_page_behavior.js
index 78bfe20f7512cb76e14f133ccba95ad8b8bd513a..b3f652a5e1b51cf44ff9e95376c2b83abc390767 100644
--- a/chrome/browser/resources/settings/settings_page/main_page_behavior.js
+++ b/chrome/browser/resources/settings/settings_page/main_page_behavior.js
@@ -45,18 +45,19 @@ var MainPageBehaviorImpl = {
   /**
    * @param {!settings.Route} newRoute
    * @param {settings.Route} oldRoute
+   * @param {boolean} isPopstate
    */
-  currentRouteChanged: function(newRoute, oldRoute) {
+  currentRouteChanged: function(newRoute, oldRoute, isPopstate) {
     // Allow the page to load before expanding the section. TODO(michaelpg):
     // Time this better when refactoring settings-animated-pages.
     if (!oldRoute && newRoute.isSubpage()) {
-      setTimeout(this.tryTransitionToSection_.bind(this));
+      setTimeout(this.tryTransitionToSection_.bind(this, isPopstate));
     } else {
       doWhenReady(
         function() {
           return this.scrollHeight > 0;
         }.bind(this),
-        this.tryTransitionToSection_.bind(this));
+        this.tryTransitionToSection_.bind(this, isPopstate));
     }
   },
 
@@ -66,9 +67,10 @@ var MainPageBehaviorImpl = {
    * that one, then schedules this function again. This ensures the current
    * section is quickly shown, without getting the page into a broken state --
    * if currentRoute changes in between calls, just transition to the new route.
+   * @param {boolean} isPopstate
    * @private
    */
-  tryTransitionToSection_: function() {
+  tryTransitionToSection_: function(isPopstate) {
     var currentRoute = settings.getCurrentRoute();
     var currentSection = this.getSection(currentRoute.section);
 
@@ -88,21 +90,21 @@ var MainPageBehaviorImpl = {
       if (!currentRoute.isSubpage() || expandedSection != currentSection) {
         promise = this.collapseSection_(expandedSection);
         // Scroll to the collapsed section.
-        if (currentSection)
+        if (currentSection && !isPopstate)
           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 (!isPopstate)
         currentSection.scrollIntoView();
     }
 
     // When this animation ends, another may be necessary. Call this function
     // again after the promise resolves.
     if (promise)
-      promise.then(this.tryTransitionToSection_.bind(this));
+      promise.then(this.tryTransitionToSection_.bind(this, isPopstate));

[michaelpg, 2016/09/02 19:37:42]

tryTransitionToSection transitions to whatever the current route is, so I'm not
sure we should be passing isPopstate like this. isPopstate should be based on
whether the most recent route was a popstate, not whether the route that happens
to be the first to call tryTransitionToSection_ after an animation was a
popstate.

Something like settings.isCurrentRoutePopstate() would work, if that would make
sense to have.

[tommycli, 2016/09/02 20:53:43]

Done.

   },
 
   /**
@@ -228,13 +230,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));