Bug 1678765 - Fix broken "Search with" action text for heuristic results that are restyled to look like search results. r=harry

This was a little harder than it seemed like it should be because we show/hide
the title separator in two places, in `UrlbarView.updateRow` and in the CSS.
This patch gets rid of the show/hide in `updateRow`, so now we show/hide only in
the CSS. The decision to show/hide in `updateRow` was based on whether the
result has action text (`actionSetter`) or is a URL (`setURL`). We already had a
`has-url` attribute that corresponds directly to `setURL`, so adding a similar
`has-action` attribute that corresponds to `actionSetter` lets us show/hide only
in the CSS.

The second part of this patch is to actually fix the bug. For that, the existing
`show-action-text` attribute does part of what we want in the CSS: It forces the
action to be shown when a one-off is selected and there's no selected row. In
addition to that, we need to show the action when both a restyled search and a
one-off are selected, so this adds a new `restyled-search` attribute. We need
both attributes because we do not want to show the action for a restyled search
when some other row plus a one-off are selected (to match the current behavior).

Depends on D97843

Differential Revision: https://phabricator.services.mozilla.com/D98429

browser/components/urlbar/UrlbarView.jsm

@@ -1372,12 +1372,14 @@ class UrlbarView {
     if (actionSetter) {
       actionSetter();
       item._originalActionSetter = actionSetter;
+      item.setAttribute("has-action", "true");
     } else {
       item._originalActionSetter = () => {
         action.removeAttribute("data-l10n-id");
         action.textContent = "";
       };
       item._originalActionSetter();
+      item.removeAttribute("has-action");
     }
 
     if (!title.hasAttribute("isurl")) {
@@ -1385,8 +1387,6 @@ class UrlbarView {
     } else {
       title.removeAttribute("dir");
     }
-
-    item._elements.get("titleSeparator").hidden = !actionSetter && !setURL;
   }
 
   /**
@@ -2002,14 +2002,23 @@ class UrlbarView {
         continue;
       }
 
-      // If the result is the heuristic, update its title to reflect the search
-      // string. This means we restyle it to look like a search result. We
+      // If the result is the heuristic and a one-off is selected (i.e.,
+      // localSearchMode || engine), then restyle it to look like a search
+      // result; otherwise, remove such styling. For restyled results, we
       // override the usual result-picking behaviour in UrlbarInput.pickResult.
       if (this.oneOffsRefresh && result.heuristic) {
         title.textContent =
           localSearchMode || engine
             ? this._queryContext.searchString
             : result.title;
+
+        // Set the restyled-search attribute so the action text and title
+        // separator are shown or hidden via CSS as appropriate.
+        if (localSearchMode || engine) {
+          item.setAttribute("restyled-search", "true");
+        } else {
+          item.removeAttribute("restyled-search");
+        }
       }
 
       // Update result action text.

browser/components/urlbar/tests/browser/browser_oneOffs_heuristicRestyle.js

@@ -86,6 +86,17 @@ async function heuristicIsNotRestyled(expectedType, resultDetails) {
     "The result has the expected non-styled action text."
   );
 
+  Assert.equal(
+    BrowserTestUtils.is_visible(resultDetails.element.separator),
+    !!actionText,
+    "The title separator is " + (actionText ? "visible" : "hidden")
+  );
+  Assert.equal(
+    BrowserTestUtils.is_visible(resultDetails.element.action),
+    !!actionText,
+    "The action text is " + (actionText ? "visible" : "hidden")
+  );
+
   Assert.equal(
     resultDetails.image,
     data.icon,
@@ -153,6 +164,15 @@ async function heuristicIsRestyled(
     "The restyled result's title should be equal to the search string."
   );
 
+  Assert.ok(
+    BrowserTestUtils.is_visible(resultDetails.element.separator),
+    "The restyled result's title separator should be visible"
+  );
+  Assert.ok(
+    BrowserTestUtils.is_visible(resultDetails.element.action),
+    "The restyled result's action text should be visible"
+  );
+
   if (engine) {
     Assert.equal(
       resultDetails.image,

browser/themes/shared/urlbarView.inc.css

@@ -495,7 +495,8 @@
 }
 
 .urlbarView-row:is([type=remotetab], [sponsored]):not([selected], :hover) > .urlbarView-row-inner > .urlbarView-url,
-.urlbarView-row[type=search]:not([selected], :hover, [show-action-text]) > .urlbarView-row-inner > .urlbarView-no-wrap > .urlbarView-title-separator,
+.urlbarView-row:is([type=search], [restyled-search]):not([selected], [show-action-text], :hover) > .urlbarView-row-inner > .urlbarView-no-wrap > .urlbarView-title-separator,
+.urlbarView-row:not([has-action], [has-url], [restyled-search]) > .urlbarView-row-inner > .urlbarView-no-wrap > .urlbarView-title-separator,
 .urlbarView:not([actionoverride]) .urlbarView-row[type=switchtab] > .urlbarView-row-inner > .urlbarView-url {
   /* Use visibility:collapse instead of display:none since the latter can
      confuse the overflow state of title and url elements when their content
@@ -504,7 +505,8 @@
 }
 
 .urlbarView-row:is([type=remotetab], [sponsored]):is([selected], :hover) > .urlbarView-row-inner > .urlbarView-no-wrap > .urlbarView-action,
-.urlbarView-row[type=search]:not([selected], :hover, [show-action-text]) > .urlbarView-row-inner > .urlbarView-no-wrap > .urlbarView-action,
+.urlbarView-row:is([type=search], [restyled-search]):not([selected], [show-action-text], :hover) > .urlbarView-row-inner > .urlbarView-no-wrap > .urlbarView-action,
+.urlbarView-row:not([has-action], [has-url], [restyled-search]) > .urlbarView-row-inner > .urlbarView-no-wrap > .urlbarView-action,
 .urlbarView[actionoverride] .urlbarView-row[type=switchtab] > .urlbarView-row-inner > .urlbarView-no-wrap > .urlbarView-action {
   display: none;
 }