MD Settings: fix collapse animation once and for all

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * Calls |readyTest| repeatedly until it returns true, then calls
  * |readyCallback|.
  * @param {function():boolean} readyTest
  * @param {!Function} readyCallback
  */
@@ skipping 59 matching lines @@
       return;
     }
 
     var promise;
     var expandedSection = /** @type {?SettingsSectionElement} */(
         this.$$('settings-section.expanded'));
     if (expandedSection) {
       // 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_();
       }
     } 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
         this.scrollToSection_();
     }
@@ skipping 52 matching lines @@
     assert(this.scroller);
     assert(section.canAnimateExpand());
 
     // Save the scroller position before freezing it.
     this.origScrollTop_ = this.scroller.scrollTop;
     this.toggleScrolling_(false);
 
     // Freeze the section's height so its card can be removed from the flow.
     section.setFrozen(true);
 
+    var finished;
     this.currentAnimation_ = section.animateExpand(this.scroller);
-    var promise = this.currentAnimation_ ?
-        this.currentAnimation_.finished : Promise.resolve();
-
-    var finished;
-    return promise.then(function() {
+    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;
-      this.toggleOtherSectionsHidden_(section.section, true);
+      // 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));
   },
 
   /**
    * 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'));
 
+    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');
+    }
+
+    // Show everything.
     this.toggleOtherSectionsHidden_(section.section, false);
-    this.toggleScrolling_(false);
+    this.classList.remove('showing-subpage');
 
-    this.currentAnimation_ =
-        section.animateCollapse(this.scroller, this.origScrollTop_);
-    var promise = this.currentAnimation_ ?
-        this.currentAnimation_.finished : Promise.resolve();
+    if (!canAnimateCollapse) {
+      // Finish by restoring the section into the page.
+      section.setFrozen(false);
+      return Promise.resolve();
+    }
 
-    return promise.then(function() {
-      section.cleanUpAnimateCollapse(true);
-    }, function() {
-      section.cleanUpAnimateCollapse(false);
-    }).then(function() {
-      section.setFrozen(false);
-      section.classList.remove('collapsing');
-      this.toggleScrolling_(true);
-      this.currentAnimation_ = null;
+    // 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));
   },
 
-  /**
-   * 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);
-  },
-
   /** @private */
   scrollToSection_: function() {
     doWhenReady(
         function() {
           return this.scrollHeight > 0;
         }.bind(this),
         function() {
           // If the current section changes while we are waiting for the page to
           // be ready, scroll to the newest requested section.
           var section = this.getSection(settings.getCurrentRoute().section);
           if (section)
             section.scrollIntoView();
         }.bind(this));
   },
 
   /**
+  /**
+   * 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}
    */
   getSection: function(section) {
     if (!section)
       return null;
     return /** @type {?SettingsSectionElement} */(
         this.$$('settings-section[section="' + section + '"]'));
   },
@@ skipping 16 matching lines @@
       this.scroller.style.width = 'calc(100% - ' + scrollbarWidth + 'px)';
     }
   }
 };
 
 /** @polymerBehavior */
 var MainPageBehavior = [
   settings.RouteObserverBehavior,
   MainPageBehaviorImpl,
 ];