From 14225c101472da4ba7d6a0326d73c6accefed470 Mon Sep 17 00:00:00 2001 From: Drew Willcoxon Date: Mon, 7 Dec 2020 22:43:02 +0000 Subject: [PATCH] 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 | 17 ++++++++++++---- .../browser_oneOffs_heuristicRestyle.js | 20 +++++++++++++++++++ browser/themes/shared/urlbarView.inc.css | 6 ++++-- 3 files changed, 37 insertions(+), 6 deletions(-) diff --git a/browser/components/urlbar/UrlbarView.jsm b/browser/components/urlbar/UrlbarView.jsm index 5776a3b798b0..daf2de226b22 100644 --- a/browser/components/urlbar/UrlbarView.jsm +++ b/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. diff --git a/browser/components/urlbar/tests/browser/browser_oneOffs_heuristicRestyle.js b/browser/components/urlbar/tests/browser/browser_oneOffs_heuristicRestyle.js index db4f14a234b4..4e2ad3162dc6 100644 --- a/browser/components/urlbar/tests/browser/browser_oneOffs_heuristicRestyle.js +++ b/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, diff --git a/browser/themes/shared/urlbarView.inc.css b/browser/themes/shared/urlbarView.inc.css index bb5af69bfce5..86cf39417548 100644 --- a/browser/themes/shared/urlbarView.inc.css +++ b/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; }