Fix focus change from OS Settings sidebar navigation

Previously, when navigating from the left hand nav in the narrow window
size, focus did not move properly to the correct section since the
drawer closing causes focus to move to the full settings page. By
delaying the route change until the drawer closes in the narrow case.

Fixed: 1026019
Change-Id: I280c9b91660221a7363baf2e0a914002bdb31fbb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1929980
Commit-Queue: Bailey Berro <baileyberro@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721276}

chrome/browser/resources/settings/chromeos/os_settings_menu/os_settings_menu.js

@@ -84,12 +84,6 @@ Polymer({
    */
   onSelectorActivate_: function(event) {
     this.setSelectedUrl_(event.detail.selected);
-
-    const path = new URL(event.detail.selected).pathname;
-    const route = settings.getRouteForPath(path);
-    assert(route, 'os-settings-menu has an entry with an invalid route.');
-    settings.navigateTo(
-        route, /* dynamicParams */ null, /* removeSearch */ true);
   },
 
   /**

chrome/browser/resources/settings/chromeos/os_settings_ui/os_settings_ui.js

@@ -102,6 +102,13 @@ Polymer({
     'refresh-pref': 'onRefreshPref_',
   },
 
+  /**
+   * The route of the selected element in os-settings-menu. Stored here to defer
+   * navigation until drawer animation completes.
+   * @private {settings.Route}
+   */
+  activeRoute_: null,
+
   /** @override */
   created: function() {
     settings.initializeRouteFromUrl();
@@ -277,10 +284,23 @@ Polymer({
 
   /**
    * Called when a section is selected.
+   * @param {!Event} e
    * @private
    */
-  onIronActivate_: function() {
-    this.$.drawer.close();
+  onIronActivate_: function(e) {
+    const section = e.detail.selected;
+    const path = new URL(section).pathname;
+    const route = settings.getRouteForPath(path);
+    assert(route, 'os-settings-menu has an entry with an invalid route.');
+    this.activeRoute_ = route;
+
+    if (this.isNarrow) {
+      // If the onIronActivate event came from the drawer, close the drawer and
+      // wait for the menu to close before navigating to |activeRoute_|.
+      this.$.drawer.close();
+      return;
+    }
+    this.navigateToActiveRoute_();
   },
 
   /** @private */
@@ -288,6 +308,20 @@ Polymer({
     this.$.drawer.toggle();
   },
 
+
+  /**
+   * Navigates to |activeRoute_| if set. Used to delay navigation until after
+   * animations complete to ensure focus ends up in the right place.
+   * @private
+   */
+  navigateToActiveRoute_: function() {
+    if (this.activeRoute_) {
+      settings.navigateTo(
+          this.activeRoute_, /* dynamicParams */ null, /* removeSearch */ true);
+      this.activeRoute_ = null;
+    }
+  },
+
   /**
    * When this is called, The drawer animation is finished, and the dialog no
    * longer has focus. The selected section will gain focus if one was selected.
@@ -299,7 +333,8 @@ Polymer({
   onMenuClose_: function() {
     if (!this.$.drawer.wasCanceled()) {
       // If a navigation happened, MainPageBehavior#currentRouteChanged handles
-      // focusing the corresponding section.
+      // focusing the corresponding section when we call settings.NavigateTo().
+      this.navigateToActiveRoute_();
       return;
     }
 

chrome/test/data/webui/settings/chromeos/os_settings_menu_test.js

@@ -59,22 +59,6 @@ suite('OSSettingsMenu', function() {
     Polymer.dom.flush();
     assertNotEquals(openIcon, ironIconElement.icon);
   });
-
-  // Test that navigating via the paper menu always clears the current
-  // search URL parameter.
-  test('clearsUrlSearchParam', function() {
-    // As of iron-selector 2.x, need to force iron-selector to update before
-    // clicking items on it, or wait for 'iron-items-changed'
-    const ironSelector = settingsMenu.$$('iron-selector');
-    ironSelector.forceSynchronousItemUpdate();
-
-    const urlParams = new URLSearchParams('search=foo');
-    settings.navigateTo(settings.routes.BASIC, urlParams);
-    assertEquals(
-        urlParams.toString(), settings.getQueryParameters().toString());
-    settingsMenu.$.people.click();
-    assertEquals('', settings.getQueryParameters().toString());
-  });
 });
 
 suite('OSSettingsMenuReset', function() {

chrome/test/data/webui/settings/chromeos/os_settings_ui_browsertest.js

@@ -216,6 +216,24 @@ TEST_F('OSSettingsUIBrowserTest', 'AllJsTests', () => {
       urlParams = settings.getQueryParameters();
       assertFalse(urlParams.has('search'));
     });
+
+    // Test that navigating via the paper menu always clears the current
+    // search URL parameter.
+    test('clearsUrlSearchParam', function() {
+      const settingsMenu = ui.$$('os-settings-menu');
+
+      // As of iron-selector 2.x, need to force iron-selector to update before
+      // clicking items on it, or wait for 'iron-items-changed'
+      const ironSelector = settingsMenu.$$('iron-selector');
+      ironSelector.forceSynchronousItemUpdate();
+
+      const urlParams = new URLSearchParams('search=foo');
+      settings.navigateTo(settings.routes.BASIC, urlParams);
+      assertEquals(
+          urlParams.toString(), settings.getQueryParameters().toString());
+      settingsMenu.$.people.click();
+      assertEquals('', settings.getQueryParameters().toString());
+    });
   });
 
   mocha.run();