MD Settings: Fix scroll to section

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.
 
 // Fast out, slow in.
 var EASING_FUNCTION = 'cubic-bezier(0.4, 0, 0.2, 1)';
 var EXPAND_DURATION = 350;
 
 /**
  * Provides animations to expand and collapse individual sections in a page.
@@ skipping 328 matching lines @@
   scrollToSection_: function() {
     // TODO(dschuyler): Determine whether this setTimeout can be removed.
     // See also: https://github.com/Polymer/polymer/issues/3629
     setTimeout(function pollForScrollHeight() {
       // If the current section changes while we are waiting for the page to be
       // ready, scroll to the newest requested section.
       var element = this.getSection_(this.currentRoute.section);
       if (!element)
         return;
 
-      if (element.parentNode.host.scrollHeight == 0) {
+      if (this.scrollHeight == 0) {
         setTimeout(pollForScrollHeight.bind(this), 100);
         return;
       }
 
       element.scrollIntoView();
     }.bind(this));
   },
 
   /** @private */
   currentRouteChanged_: function(newRoute, oldRoute) {
@@ skipping 13 matching lines @@
 
     if (!newRouteIsSubpage && oldRouteIsSubpage) {
       var section = this.getSection_(oldRoute.section);
       if (section)
         this.collapseSection(section);
     } else if (newRouteIsSubpage &&
                (!oldRouteIsSubpage || newRoute.section != oldRoute.section)) {
       var section = this.getSection_(newRoute.section);
       if (section)
         this.expandSection(section);
-    } else if (newRoute && newRoute.section) {
+    } else if (newRoute && newRoute.section && !oldRouteIsSubpage) {

[dschuyler, 2016/06/06 23:51:42]

What is this addition addressing?

[michaelpg, 2016/06/07 02:55:05]

Warning: wall of text that I'm publishing for my notes, probably not worth
reading because implementing the very last line may nullify everything else.

here, oldRouteIsSubpage implies newRouteIsSubpage which implies newRoute.section
== oldRoute.section

so: old route and new route are both subpages within the same section, so it
does NOT make sense to change the scroll position (ie, we shouldn't[1] change
the scroll position of the page from whatever it was before entering the first
subpage).

[1] it doesn't actually, because of implementation details elsewhere, so this is
just preventative (and a very minor optimization) rather than a bug fix.

Equivalently, we shouldn't scroll to the section when showing a subpage
(!oldRouteIsSubpage also implies !newRouteIsSubpage due to 378-379). We may want
to add this behavior, but we should do it at line 380 where it belongs.

However, there's another fix we need: 377 should also scroll to the section IF
we got here by clicking a menu entry, or a link to some other section, but NOT
if we used the back button or just hit the back arrow in the subpage... crap.

[dschuyler, 2016/06/08 00:35:05]

That's a lot of indirect logic for folks following along at home, please
consider one of the following:
- rework the logic flow here (lines 374 to 385) to make it easier to follow
- or add a bug with your notes above and comment around line 374 that this logic
should be reworked, referencing the new bug.

       this.scrollToSection_();
     }
   },
 
   /**
    * Helper function to get a section from the local DOM.
    * @param {string} section Section name of the element to get.
    * @return {?SettingsSectionElement}
    * @private
    */
   getSection_: function(section) {
     return /** @type {?SettingsSectionElement} */(
         this.$$('[section=' + section + ']'));
   },
 };
 
 
 /** @polymerBehavior */
 var RoutableBehavior = [
   MainPageBehavior,
   RoutableBehaviorImpl
 ];