Settings: show search bubbles on some <select> tags

Because of various technical details, we can show search bubbles only in
cases in which the UI is fairly stably visible (i.e. not subpages).

Screenshots: https://bugs.chromium.org/p/chromium/issues/detail?id=355446#c14

Bug: 355446
Change-Id: I732e64da8916e183655e406b18f06f0a0b02ed31
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1962797
Commit-Queue: Dan Beam <dbeam@chromium.org>
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Auto-Submit: Dan Beam <dbeam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#727536}

chrome/browser/resources/print_preview/ui/highlight_utils.js

@@ -40,20 +40,19 @@ export function updateHighlights(element, query) {
       if (query.test(textContent)) {
         // Don't highlight <select> nodes, yellow rectangles can't be
         // displayed within an <option>.
-        if (node.parentNode.nodeName !== 'OPTION') {
-          result.highlights.push(highlight(node, textContent.split(query)));
-        } else {
-          const selectNode = node.parentNode.parentNode;
+        if (node.parentNode.nodeName === 'OPTION') {
           // The bubble should be parented by the select node's parent.
           // Note: The bubble's ::after element, a yellow arrow, will not
           // appear correctly in print preview without SPv175 enabled. See
           // https://crbug.com/817058.
           const bubble = highlightControlWithBubble(
-              /** @type {!HTMLElement} */ (assert(selectNode.parentNode)),
-              textContent.match(query)[0]);
+              /** @type {!HTMLElement} */ (assert(node.parentNode.parentNode)),
+              textContent.match(query)[0], /*horizontallyCenter=*/ true);
           if (bubble) {
             result.bubbles.push(bubble);
           }
+        } else {
+          result.highlights.push(highlight(node, textContent.split(query)));
         }
       }
     });

chrome/browser/resources/settings/BUILD.gn

@@ -179,8 +179,10 @@ js_library("page_visibility") {
 
 js_library("search_settings") {
   deps = [
+    "//ui/webui/resources/js:assert",
     "//ui/webui/resources/js:cr",
     "//ui/webui/resources/js:search_highlight_utils",
+    "//ui/webui/resources/js:util",
   ]
   externs_list = [ "$externs_path/pending_polymer.js" ]
 }

chrome/browser/resources/settings/search_settings.js

@@ -103,11 +103,26 @@ cr.define('settings', function() {
             bubbles.push(bubble);
           }
 
-          // Don't highlight <select> nodes, yellow rectangles can't be
-          // displayed within an <option>.
-          // TODO(dpapad): highlight <select> controls with a search bubble
-          // instead.
-          if (node.parentNode.nodeName != 'OPTION') {
+          if (node.parentNode.nodeName === 'OPTION') {
+            const select = node.parentNode.parentNode;
+            assert(select.nodeName === 'SELECT');
+
+            // TODO(crbug.com/355446): support showing bubbles inside subpages.
+            // Currently, they're incorrectly positioned and there's no great
+            // signal at which to know when to reposition them (because every
+            // page asynchronously loads/renders things differently).
+            const isSubpage = n => n.nodeName === 'SETTINGS-SUBPAGE';
+            if (findAncestor(select, isSubpage, true)) {
+              return;
+            }
+
+            const bubble = cr.search_highlight_utils.highlightControlWithBubble(
+                select, textContent.match(request.regExp)[0],
+                /*horizontallyCenter=*/ true);
+            if (bubble) {
+              bubbles.push(bubble);
+            }
+          } else {
             request.addTextObserver(node);
             highlights.push(cr.search_highlight_utils.highlight(
                 node, textContent.split(request.regExp)));

chrome/test/data/webui/settings/search_settings_test.js

@@ -66,8 +66,8 @@ cr.define('settings_test', function() {
 
     /**
      * Tests that a search hit within a <select> node causes the parent
-     * settings-section to be shown, but the DOM of the <select> is not
-     * modified.
+     * settings-section to be shown and the <select> to be highlighted by a
+     * bubble.
      */
     test('<select> highlighting', function() {
       document.body.innerHTML = `<settings-section hidden-by-search>
@@ -87,8 +87,8 @@ cr.define('settings_test', function() {
             assertFalse(section.hiddenBySearch);
 
             const highlightWrapper =
-                select.querySelector('.search-highlight-wrapper');
-            assertFalse(!!highlightWrapper);
+                section.querySelector('.search-highlight-wrapper');
+            assertTrue(!!highlightWrapper);
 
             // Check that original DOM structure is present even after search
             // highlights are cleared.

ui/webui/resources/html/search_highlight_utils.html

+<link rel="import" href="assert.html">
 <script src="../js/search_highlight_utils.js"></script>

ui/webui/resources/js/BUILD.gn

@@ -59,6 +59,7 @@ js_library("event_tracker") {
 
 js_library("search_highlight_utils") {
   deps = [
+    ":assert",
     ":cr",
   ]
 }

ui/webui/resources/js/search_highlight_utils.js

@@ -95,11 +95,26 @@ cr.define('cr.search_highlight_utils', function() {
    * should already be visible or the bubble will render incorrectly.
    * @param {!HTMLElement} element The element to be highlighted.
    * @param {string} rawQuery The search query.
+   * @param {boolean=} horizontallyCenter Whether or not to horizontally center
+   *     the shown search bubble (if any) based on |element|'s left and width.
    * @return {?Node} The search bubble that was added, or null if no new bubble
    *     was added.
    */
-  /* #export */ function highlightControlWithBubble(element, rawQuery) {
-    let searchBubble = element.querySelector(`.${SEARCH_BUBBLE_CSS_CLASS}`);
+  /* #export */ function highlightControlWithBubble(
+      element, rawQuery, horizontallyCenter) {
+    let anchor = element;
+    if (element.tagName === 'SELECT') {
+      anchor = element.parentNode;
+    }
+    // NOTE(dbeam): this is theoretically only possible in the select case, but
+    // callers are often fast and loose with regard to casting |element| from a
+    // parentNode (which can easily be a shadow root). So we leave this if for
+    // all branches (rather than solely inside the select branch).
+    if (anchor instanceof ShadowRoot) {
+      anchor = anchor.host.parentNode;
+    }
+
+    let searchBubble = anchor.querySelector(`.${SEARCH_BUBBLE_CSS_CLASS}`);
     // If the element has already been highlighted, there is no need to do
     // anything.
     if (searchBubble) {
@@ -112,13 +127,18 @@ cr.define('cr.search_highlight_utils', function() {
     innards.classList.add('search-bubble-innards');
     innards.textContent = rawQuery;
     searchBubble.appendChild(innards);
-    element.appendChild(searchBubble);
+    anchor.appendChild(searchBubble);
 
     const updatePosition = function() {
+      assert(typeof element.offsetTop === 'number');
       searchBubble.style.top = element.offsetTop +
           (innards.classList.contains('above') ? -searchBubble.offsetHeight :
                                                  element.offsetHeight) +
           'px';
+      if (horizontallyCenter) {
+        const width = element.offsetWidth - searchBubble.offsetWidth;
+        searchBubble.style.left = element.offsetLeft + width / 2 + 'px';
+      }
     };
     updatePosition();
 
@@ -126,6 +146,9 @@ cr.define('cr.search_highlight_utils', function() {
       innards.classList.toggle('above');
       updatePosition();
     });
+    // TODO(crbug.com/355446): create a way to programmatically update these
+    // bubbles (i.e. call updatePosition()) when outer scope knows they need to
+    // be repositioned.
     return searchBubble;
   }