Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(6137)

Unified Diff: chrome/browser/resources/settings/settings_page/main_page_behavior.js

Issue 2259163002: MD Settings: Fix scrolling bugs and hacks caused by undefined load behavior (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@TransitionsNoTestsFakeRebase
Patch Set: bug fixes Created 4 years, 4 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
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 ad4e84687e280c6d6ff6e910efb27491ee1d34a2..a8723ce554b254874658f76f5688c540012b9699 100644
--- a/chrome/browser/resources/settings/settings_page/main_page_behavior.js
+++ b/chrome/browser/resources/settings/settings_page/main_page_behavior.js
@@ -3,23 +3,6 @@
// found in the LICENSE file.
/**
- * Calls |readyTest| repeatedly until it returns true, then calls
- * |readyCallback|.
- * @param {function():boolean} readyTest
- * @param {!Function} readyCallback
- */
-function doWhenReady(readyTest, readyCallback) {
- // TODO(dschuyler): Determine whether this hack can be removed.
- // See also: https://github.com/Polymer/polymer/issues/3629
- var intervalId = setInterval(function() {
- if (readyTest()) {
- clearInterval(intervalId);
- readyCallback();
- }
- }, 10);
-}
-
-/**
* Responds to route changes by expanding, collapsing, or scrolling to sections
* on the page. Expanded sections take up the full height of the container. At
* most one section should be expanded at any given time.
@@ -42,20 +25,11 @@ var MainPageBehaviorImpl = {
* @param {settings.Route} oldRoute
*/
currentRouteChanged: function(newRoute, oldRoute) {
- if (oldRoute || !newRoute.section) {
+ // Allow the page to load before expanding the section.
+ if (!oldRoute)
+ setTimeout(this.tryTransitionToSection_.bind(this));
+ else
this.tryTransitionToSection_();
- return;
- }
-
- // Wait for the page to really be ready before expanding or scrolling to the
- // section. TODO(michaelpg): Remove this workaround with a global workaround
- // for crbug.com/638074.
- doWhenReady(function() {
- if (newRoute != settings.getCurrentRoute())
- return;
- var section = this.getSection(newRoute.section);
- return section && (!newRoute.isSubpage() || section.canAnimateExpand());
- }.bind(this), this.tryTransitionToSection_.bind(this));
},
/**
@@ -91,6 +65,10 @@ var MainPageBehaviorImpl = {
}
} else if (currentSection) {
// Expand the section into a subpage or scroll to it on the main page.
+ if (!currentSection.clientHeight) {
+ setTimeout(this.tryTransitionToSection_.bind(this));
+ return;
+ }
if (currentRoute.isSubpage())
promise = this.expandSection_(currentSection);
else
@@ -212,15 +190,15 @@ var MainPageBehaviorImpl = {
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);
+ var currentRoute = settings.getCurrentRoute();
+ var newSection =
+ currentRoute.section && this.getSection(currentRoute.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?
+ // Scroll to the new section if it's on this page. If the route has no
+ // section (e.g., a root page), return to the original position.
if (newSection)
newSection.scrollIntoView();
- else
+ else if (!currentRoute.section)
this.scroller.scrollTop = this.origScrollTop_;
this.currentAnimation_ = section.animateCollapse(
@@ -243,17 +221,10 @@ var MainPageBehaviorImpl = {
/** @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));
+ assert(this.scrollHeight > 0);
+ var section = this.getSection(settings.getCurrentRoute().section);
+ assert(section);
+ section.scrollIntoView();
},
/**

Powered by Google App Engine
This is Rietveld 408576698