[OsSettingsSearch] Route change when a row is clicked or keydown 'Enter'

* Keydown 'Enter' causes the selected row to trigger a route change.
* If the selected row is focused, keydown 'Enter' triggers route change.
* Clicking on specific row causes the row to trigger a route change
  (iron-list also causes the row to become the selected row on click).
* If a user deselects a row, re-select the row (one row should always be
  selected). Deselection is an <iron-list> behavior we must work around.
* Add test to confirm <os-search-result-row> has the correct behaviors.
* A route change won't cause the cr-input to lose its text, nor will it
  cause the results to be lost, nor the selected item to be deselected.

Bug: 1056909
Change-Id: I00c151acd7439b916b4b01bec188cb48197c775d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2123787
Commit-Queue: Regan Hsu <hsuregan@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#755102}

chrome/browser/resources/settings/chromeos/os_settings_search_box/BUILD.gn

@@ -13,6 +13,7 @@ js_type_check("closure_compile") {
 
 js_library("os_settings_search_box") {
   deps = [
+    ":os_search_result_row",
     "..:metrics_recorder",
     "//chrome/browser/ui/webui/settings/chromeos/search:mojo_bindings_js_library_for_compile",
     "//third_party/polymer/v1_0/components-chromium/iron-dropdown:iron-dropdown-extracted",
@@ -24,7 +25,8 @@ js_library("os_settings_search_box") {
 
 js_library("os_search_result_row") {
   deps = [
-    "//ui/webui/resources/js/cr/ui:focus_row",
+    "..:os_route",
+    "../..:router",
     "//ui/webui/resources/js/cr/ui:focus_row_behavior",
   ]
 }

chrome/browser/resources/settings/chromeos/os_settings_search_box/os_search_result_row.html

 <link rel="import" href="chrome://resources/html/polymer.html">
 
 <link rel="import" href="chrome://resources/html/cr/ui/focus_row_behavior.html">
+<link rel="import" href="../os_route.html">
+<link rel="import" href="../../router.html">
 <link rel="import" href="../../settings_shared_css.html">
 
 <dom-module id="os-search-result-row">
@@ -12,27 +14,29 @@
         height: 40px;
       }
 
-      :host([selected]) .search-result-container {
+      :host([selected]) #searchResultContainer {
         background-color: var(--cros-menu-button-bg-color-active);
       }
 
-      :host(:not([selected])) .search-result-container:hover {
+      :host(:not([selected])) #searchResultContainer:hover {
         background-color: var(--cros-menu-button-bg-color-hover);
       }
 
-      .search-result-container {
+      #searchResultContainer {
         display: flex;
         flex-basis: 100%;
       }
 
-      .text {
+      #resultText {
         flex-basis: 100%;
       }
     </style>
-    <div class="search-result-container" focus-row-container>
+    <div id="searchResultContainer" on-click="navigateToSearchResultRoute"
+        focus-row-container>
       <!-- TODO(crbug/1056909): Focus right hand icon instead; move
            focus-row-control to that icon when available-->
-      <div class="text" focus-type="rowWrapper" focus-row-control selectable>
+      <div id="resultText" focus-row-control selectable
+          on-keypress="onKeyPress_" focus-type="rowWrapper">
         [[getResultText_(searchResult)]]
       </div>
     </div>

chrome/browser/resources/settings/chromeos/os_settings_search_box/os_search_result_row.js

@@ -30,4 +30,39 @@ Polymer({
     // so it must be converted to a JS String.
     return String.fromCharCode.apply(null, this.searchResult.resultText.data);
   },
+
+  /**
+   * Only relevant when the focus-row-control is focus()ed. This keypress
+   * handler specifies that pressing 'Enter' should cause a route change.
+   * TODO(crbug/1056909): Add test for this specific case.
+   * @param {!KeyboardEvent} e
+   * @private
+   */
+  onKeyPress_(e) {
+    if (e.key === 'Enter') {
+      e.stopPropagation();
+      this.navigateToSearchResultRoute();
+    }
+  },
+
+  navigateToSearchResultRoute() {
+    assert(this.searchResult.urlPathWithParameters, 'Url path is empty.');
+
+    // |this.searchResult.urlPathWithParameters| separates the path and params
+    // by a '?' char.
+    const pathAndOptParams = this.searchResult.urlPathWithParameters.split('?');
+
+    // There should be at most 2 items in the array (the path and the params).
+    assert(pathAndOptParams.length <= 2, 'Path and params format error.');
+
+    const route = assert(
+        settings.Router.getInstance().getRouteForPath(
+            '/' + pathAndOptParams[0]),
+        'Supplied path does not map to an existing route.');
+    const params = pathAndOptParams.length == 2 ?
+        new URLSearchParams(pathAndOptParams[1]) :
+        undefined;
+    settings.Router.getInstance().navigateTo(route, params);
+    this.fire('navigated-to-result-route');
+  },
 });

chrome/browser/resources/settings/chromeos/os_settings_search_box/os_settings_search_box.html

@@ -8,7 +8,9 @@
 <link rel="import" href="chrome://resources/polymer/v1_0/iron-list/iron-list.html">
 <link rel="import" href="os_search_result_row.html">
 <link rel="import" href="../metrics_recorder.html">
+<link rel="import" href="../os_route.html">
 <link rel="import" href="../../settings_shared_css.html">
+<link rel="import" href="../../router.html">
 
 <dom-module id="os-settings-search-box">
   <template>
@@ -56,9 +58,9 @@
       <!--  As part of iron-dropdown's behavior, the slot 'dropdown-content' is
             hidden until iron-dropdown's opened attribute is set true, or when
             iron-dropdown's open() is called on the JS side. -->
-      <iron-list id="searchResultList" slot="dropdown-content"
-          selection-enabled items="[[searchResults_]]"
-          selected-item="{{selectedItem_}}">
+      <iron-list id="searchResultList" slot="dropdown-content" selection-enabled
+          items="[[searchResults_]]" selected-item="{{selectedItem_}}"
+          on-selected-item-changed="onSelectedItemChanged_">
         <!-- TODO(crbug/1056909): Use template dom-if if searchResults_ is
              empty, show 'No Results' UI -->
         <template>
@@ -66,6 +68,7 @@
               selected="[[isItemSelected_(item, selectedItem_)]]"
               tabindex$="[[getRowTabIndex_(item, selectedItem_)]]"
               iron-list-tab-index$="[[getRowTabIndex_(item, selectedItem_)]]"
+              on-navigated-to-result-route="onNavigatedtoResultRowRoute_"
               last-focused="{{lastFocused_}}"
               list-blurred="{{listBlurred_}}"
               focus-row-index="[[index]]"

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

@@ -46,16 +46,16 @@ function fakeSettingsSearchHandlerSearch(query) {
 
   const Icon = mojom.SearchResultIcon;
   const fakeRandomResults = [
-    ['bluetooth devices', '/bluetoothDevices', Icon.kWifi],
-    ['wifi', '/networks?type=WiFi', Icon.kWifi],
-    ['languages', '/languages/details', Icon.kWifi],
-    ['people', '/people', Icon.kWifi],
-    ['security', '/privacy', Icon.kWifi],
-    ['personalization', '/personalization', Icon.kWifi],
-    ['keyboard', '/keyboard-overlay', Icon.kWifi],
-    ['touchpad', '/pointer-overlay', Icon.kWifi],
-    ['lock screen', '/lockScreen', Icon.kWifi],
-    ['time zone', '/dateTime/timeZone', Icon.kWifi],
+    ['bluetooth devices', 'bluetoothDevices', Icon.kWifi],
+    ['wifi', 'networks?type=WiFi', Icon.kWifi],
+    ['languages', 'languages/details', Icon.kWifi],
+    ['people', 'people', Icon.kWifi],
+    ['security', 'privacy', Icon.kWifi],
+    ['personalization', 'personalization', Icon.kWifi],
+    ['keyboard', 'keyboard-overlay', Icon.kWifi],
+    ['touchpad', 'pointer-overlay', Icon.kWifi],
+    ['lock screen', 'lockScreen', Icon.kWifi],
+    ['time zone', 'dateTime/timeZone', Icon.kWifi],
   ].map(result => generateFakeResult(result));
 
   fakeRandomResults.sort(() => Math.random() - 0.5);
@@ -101,6 +101,16 @@ Polymer({
       type: Object,
     },
 
+    /**
+     * Prevent user deselection by tracking last item selected. This item must
+     * only be assigned to an item within |this.$.searchResultList|, and not
+     * directly to |this.selectedItem_| or an item within |this.searchResults_|.
+     * @private {!mojom.SearchResult}
+     */
+    lastSelectedItem_: {
+      type: Object,
+    },
+
     /**
      * Passed into <iron-list>. Exactly one result is the selectedItem_.
      * @private {!Array<!mojom.SearchResult>}
@@ -163,6 +173,17 @@ Polymer({
     this.searchHandler_ = searchHandler;
   },
 
+  /**
+   * @return {!OsSearchResultRowElement} The <os-search-result-row> that is
+   *     associated with the selectedItem.
+   * @private
+   */
+  getSelectedOsSearchResultRow_() {
+    return assert(
+        this.$.searchResultList.querySelector('os-search-result-row[selected]'),
+        'No OsSearchResultRow is selected.');
+  },
+
   /**
    * @return {boolean}
    * @private
@@ -217,7 +238,21 @@ Polymer({
   },
 
   /** @private */
-  onBlur_() {
+  onNavigatedtoResultRowRoute_() {
+    // Settings has navigated to another page; close search results dropdown.
+    this.$.searchResults.close();
+
+    // Blur search input to prevent blinking caret.
+    this.$.search.blur();
+  },
+
+  /**
+   * @param {!Event} e
+   * @private
+   */
+  onBlur_(e) {
+    e.stopPropagation();
+
     // The user has clicked a region outside the search box or the input has
     // been blurred; close the dropdown regardless if there are searchResults_.
     this.$.searchResults.close();
@@ -268,13 +303,34 @@ Polymer({
     this.selectedItem_ = this.searchResults_[0];
   },
 
+  /**
+   * |selectedItem| is not changed by the time this is called. The value that
+   * |selectedItem| will be assigned to is stored in
+   * |this.$.searchResultList.selectedItem|.
+   * TODO(crbug/1056909): Add test for this specific case.
+   * @private
+   */
+  onSelectedItemChanged_() {
+    // <iron-list> causes |this.$.searchResultList.selectedItem| to be null if
+    // tapped a second time.
+    if (!this.$.searchResultList.selectedItem && this.lastSelectedItem_) {
+      // In the case that the user deselects a search result, reselect the item
+      // manually by altering the list. Setting |selectedItem| will be no use
+      // as |selectedItem| has not been assigned at this point. Adding an
+      // observer on |selectedItem| to address a value change will also be no
+      // use as it will perpetuate an infinite select and deselect chain in this
+      // case.
+      this.$.searchResultList.selectItem(this.lastSelectedItem_);
+    }
+    this.lastSelectedItem_ = this.$.searchResultList.selectedItem;
+  },
+
   /**
    * @param {string} key The string associated with a key.
    * @private
    */
   selectRowViaKeys_(key) {
-    assert(key === 'ArrowDown' || key === 'ArrowUp' || key === 'Tab');
-
+    assert(key === 'ArrowDown' || key === 'ArrowUp', 'Only arrow keys.');
     assert(!!this.selectedItem_, 'There should be a selected item already.');
 
     // Select the new item.
@@ -284,19 +340,18 @@ Polymer({
     const indexOfNewRow = (numRows + selectedRowIndex + delta) % numRows;
     this.selectedItem_ = this.searchResults_[indexOfNewRow];
 
-    // If a row is focused, ensure it is the selectedResult's row.
     if (this.lastFocused_) {
-      const rowEls = Array.from(
-          this.$.searchResultList.querySelectorAll('os-search-result-row'));
+      // If a row was previously focused, focus the currently selected row.
       // Calling focus() on a <os-search-result-row> focuses the element within
       // containing the attribute 'focus-row-control'.
-      rowEls[indexOfNewRow].focus();
+      this.getSelectedOsSearchResultRow_().focus();
     }
   },
 
   /**
    * Keydown handler to specify how enter-key, arrow-up key, and arrow-down-key
-   * interacts with search results in the dropdown.
+   * interacts with search results in the dropdown. Note that 'Enter' on keyDown
+   * when a row is focused is blocked by cr.ui.FocusRowBehavior behavior.
    * @param {!KeyboardEvent} e
    * @private
    */
@@ -307,16 +362,7 @@ Polymer({
     }
 
     if (e.key === 'Enter') {
-      // TODO(crbug/1056909): Take action on selected row.
-      return;
-    }
-
-    if (e.key === 'Tab') {
-      if (this.lastFocused_) {
-        // Prevent continuous tabbing from leaving search result
-        e.preventDefault();
-        this.selectRowViaKeys_(e.key);
-      }
+      this.getSelectedOsSearchResultRow_().navigateToSearchResultRoute();
       return;
     }
 

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

@@ -37,13 +37,15 @@ suite('OSSettingsSearchBox', () => {
 
   /**
    * @param {string} resultText Exact string of the result to be displayed.
+   * @param {string} path Url path with optional params.
    * @return {!chromeos.settings.mojom.SearchResult} A search result.
    */
-  function fakeResult(resultText) {
+  function fakeResult(resultText, urlPathWithParameters) {
     return /** @type {!mojom.SearchResult} */ ({
       resultText: {
         data: Array.from(resultText, c => c.charCodeAt()),
       },
+      urlPathWithParameters: urlPathWithParameters,
     });
   }
 
@@ -64,6 +66,7 @@ suite('OSSettingsSearchBox', () => {
 
     userActionRecorder = new settings.FakeUserActionRecorder();
     settings.setUserActionRecorderForTesting(userActionRecorder);
+    settings.Router.getInstance().navigateTo(settings.routes.BASIC);
   });
 
   teardown(async () => {
@@ -177,4 +180,50 @@ suite('OSSettingsSearchBox', () => {
     searchBox.dispatchEvent(arrowRightEvent);
     assertEquals(resultList.selectedItem, resultList.items[0]);
   });
+
+  test('Keydown Enter causes route change on selected row', async () => {
+    settingsSearchHandler.setFakeResults(
+        [fakeResult('WiFi Settings', 'networks?type=WiFi')]);
+    await simulateSearch('query');
+
+    // Adding two flushTasks ensures that all events are fully handled, so that
+    // the iron-list is fully rendered and that the physical
+    // <os-search-result-row>s are available to dispatch 'Enter' on.
+    await test_util.flushTasks();
+    await test_util.flushTasks();
+
+    const enterEvent = new KeyboardEvent(
+        'keydown', {cancelable: true, key: 'Enter', keyCode: 13});
+
+    // Clicking on the row correctly changes the route and dropdown to close.
+    searchBox.dispatchEvent(enterEvent);
+    Polymer.dom.flush();
+    assertFalse(dropDown.opened);
+    const router = settings.Router.getInstance();
+    assertEquals(router.getCurrentRoute().path, '/networks');
+    assertEquals(router.getQueryParameters().get('type'), 'WiFi');
+  });
+
+  test('Route change when result row is clicked', async () => {
+    settingsSearchHandler.setFakeResults(
+        [fakeResult('WiFi Settings', 'networks?type=WiFi')]);
+    await simulateSearch('query');
+
+    // Adding two flushTasks ensures that all events are fully handled, so that
+    // the iron-list is fully rendered and that the physical
+    // <os-search-result-row>s are available to dispatch 'Enter' on.
+    await test_util.flushTasks();
+    await test_util.flushTasks();
+
+    const searchResultRow = searchBox.getSelectedOsSearchResultRow_();
+
+    // Clicking on the searchResultContainer of the row correctly changes the
+    // route and dropdown to close.
+    searchResultRow.$.searchResultContainer.click();
+
+    assertFalse(dropDown.opened);
+    const router = settings.Router.getInstance();
+    assertEquals(router.getCurrentRoute().path, '/networks');
+    assertEquals(router.getQueryParameters().get('type'), 'WiFi');
+  });
 });