MD Settings: fix collapse animation once and for all

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 7eeedf2318f8baba885c85a8e24b2ff6e9d94707..5931217f1a821e80b31f156a4a093acc90ddebd1 100644
--- a/chrome/browser/resources/settings/settings_page/main_page_behavior.js
+++ b/chrome/browser/resources/settings/settings_page/main_page_behavior.js
@@ -77,10 +77,7 @@ var MainPageBehaviorImpl = {
       // If the section shouldn't be expanded, collapse it.
       if (!currentRoute.isSubpage() || expandedSection != currentSection) {
         promise = this.collapseSection_(expandedSection);
-        // Scroll to the collapsed section. TODO(michaelpg): This can look weird
-        // because the collapse we just scheduled calculated its end target
-        // based on the current scroll position. This bug existed before, and is
-        // fixed in the next patch by making the card position: absolute.
+        // Scroll to the collapsed section.
         if (currentSection)
           this.scrollToSection_();
       }
@@ -153,21 +150,22 @@ var MainPageBehaviorImpl = {
     // Freeze the section's height so its card can be removed from the flow.
     section.setFrozen(true);
 
-    this.currentAnimation_ = section.animateExpand(this.scroller);
-    var promise = this.currentAnimation_ ?
-        this.currentAnimation_.finished : Promise.resolve();
-
     var finished;
-    return promise.then(function() {
-      this.scroller.scrollTop = 0;
+    this.currentAnimation_ = section.animateExpand(this.scroller);
+    return this.currentAnimation_.finished.then(function() {
+      // Hide other sections and scroll to the top of the subpage.
+      this.classList.add('showing-subpage');
       this.toggleOtherSectionsHidden_(section.section, true);
+      this.scroller.scrollTop = 0;
+      // Notify that the page is fully expanded.
+      this.fire('subpage-expand');
       finished = true;

[Dan Beam, 2016/08/17 03:01:02]

could we maybe add some new lines in this code?

[michaelpg, 2016/08/19 21:55:01]

Done.

     }.bind(this), function() {
-      // The animation was canceled; restore the section.
+      // The animation was canceled; restore the section and scroll position.
       section.setFrozen(false);
+      this.scroller.scrollTop = this.origScrollTop_;
       finished = false;
-    }).then(function() {
-      section.cleanUpAnimateExpand(finished);
+    }.bind(this)).then(function() {
       this.toggleScrolling_(true);
       this.currentAnimation_ = null;
     }.bind(this));
@@ -177,43 +175,62 @@ var MainPageBehaviorImpl = {
    * Animates the card in |section|, collapsing it back into its section.
    * @param {!SettingsSectionElement} section
    * @return {!Promise} Resolved when the transition is finished or canceled.
-   * @private
    */
   collapseSection_: function(section) {
     assert(this.scroller);
-    assert(section.canAnimateCollapse());
+    assert(section.classList.contains('expanded'));
 
-    this.toggleOtherSectionsHidden_(section.section, false);
-    this.toggleScrolling_(false);
+    var canAnimateCollapse = section.canAnimateCollapse();
+    if (canAnimateCollapse) {
+      this.toggleScrolling_(false);
+      // Do the initial collapse setup, which takes the section out of the flow,
+      // before showing everything.
+      section.setUpAnimateCollapse(this.scroller);
+    } else {
+      section.classList.remove('expanded');
+    }
 
-    this.currentAnimation_ =
-        section.animateCollapse(this.scroller, this.origScrollTop_);
-    var promise = this.currentAnimation_ ?
-        this.currentAnimation_.finished : Promise.resolve();
+    // Show everything.
+    this.toggleOtherSectionsHidden_(section.section, false);
+    this.classList.remove('showing-subpage');
 
-    return promise.then(function() {
-      section.cleanUpAnimateCollapse(true);
-    }, function() {
-      section.cleanUpAnimateCollapse(false);
-    }).then(function() {
+    if (!canAnimateCollapse) {
+      // Finish by restoring the section into the page.
       section.setFrozen(false);
-      section.classList.remove('collapsing');
-      this.toggleScrolling_(true);
-      this.currentAnimation_ = null;
-    }.bind(this));
-  },
+      return Promise.resolve();
+    }
 
-  /**
-   * Hides or unhides the sections not being expanded.
-   * @param {string} sectionName The section to keep visible.
-   * @param {boolean} hidden Whether the sections should be hidden.
-   * @private
-   */
-  toggleOtherSectionsHidden_: function(sectionName, hidden) {
-    var sections = Polymer.dom(this.root).querySelectorAll(
-        'settings-section');
-    for (var section of sections)
-      section.hidden = hidden && (section.section != sectionName);
+    // Play the actual collapse animation.
+    return new Promise(function(resolve, reject) {
+      // Wait for the other sections to show up so we can scroll properly.
+      setTimeout(function() {
+        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.currentAnimation_ = section.animateCollapse(
+            /** @type {!HTMLElement} */(this.scroller));
+
+        this.currentAnimation_.finished.catch(function() {
+          // The collapse was canceled, so the page is showing a subpage still.
+          this.fire('subpage-expand');
+        }.bind(this)).then(function() {
+          // Clean up after the animation succeeds or cancels.
+          section.setFrozen(false);
+          section.classList.remove('collapsing');
+          this.toggleScrolling_(true);
+          this.currentAnimation_ = null;
+          resolve();
+        }.bind(this));
+      }.bind(this));
+    }.bind(this));
   },
 
   /** @private */
@@ -232,6 +249,20 @@ var MainPageBehaviorImpl = {
   },
 
   /**
+  /**
+   * Hides or unhides the sections not being expanded.
+   * @param {string} sectionName The section to keep visible.
+   * @param {boolean} hidden Whether the sections should be hidden.
+   * @private
+   */
+  toggleOtherSectionsHidden_: function(sectionName, hidden) {
+    var sections = Polymer.dom(this.root).querySelectorAll(
+        'settings-section');
+    for (var section of sections)
+      section.hidden = hidden && (section.section != sectionName);
+  },
+
+  /**
    * Helper function to get a section from the local DOM.
    * @param {string} section Section name of the element to get.
    * @return {?SettingsSectionElement}