1480505 - Highlight search engine keywords in awesomebar

Status: VERIFIED FIXED

(Firefox :: New Tab Page, enhancement, P3)

Description

[Kate Hudson :k88hudson]

When we implement special search engine keywords (e.g. @amazon, @google) in Bug 1480504, we would like some special UI where the keyword is highlighted.

Comment 1

[Kate Hudson :k88hudson]

This is not required for MVP but it is desired if possible.

Comment 2

[Drew Willcoxon :adw]

Question for UX: How can the user interact with these keywords?  e.g., can they move the caret into them, can they delete characters from them, can they select characters in them -- IOW in terms of interaction, are they part of the normal editable text in the text box?  Or are they separate tokens?  Can the user delete them via the keyboard (e.g. backspace)?

Some possible interactions, in increasing order of implementation difficulty:

(1) The user can't interact at all with these keywords, like the "Switch to tab:" text that appears between the magnifying glass and the editable text when you highlight a switch to tab result in the urlbar popup.  That seems possible for 62.

(2) Same as (1), but the user can delete them if the caret is at the beginning of the editable text and they press backspace.  That seems possible for 62.

(3) The keywords don't behave like editable/selectable text in text boxes, but they're separate tokens that can be selected, copied, moved, etc. as a whole.  Probably not possible for 62.

(4) The keywords do behave like editable text, with special styling.  Probably not possible for 62.  But we do style the text in the urlbar when it's not focused -- maybe I'm wrong and that's possible when the urlbar is focused too, and if so, this might actually be easier than I'm thinking.

Comment 3

[Ed Lee :Mardak]

Attachment: mockup of "switch to tab" -> "@amazon"

Comment 4

[Ed Lee :Mardak]

If we do switch into a "search keyword mode" as in suggestions 1 or 2, do we trigger on when the first popup autocomplete result comes back as a search completion? E.g., the user starts typing "@ a m a z o n" then after the "n" it switches?

Comment 5

[Aaron Benson]

I would point to the @mention behavior in Slack as a reference for this. It's probably more helpful than me explaining the nuances. But I'll try... 

The behavior should work as follows (using '@amazon' as the example):

- Typing the "@" symbol opens a list of suggestions and as you type that list is filtered. 

- Clicking on, or using up/down arrows to go-to and select one of the suggestions in the list would transform "@am" to "@amazon" with the background blue highlight.

- Pressing [tab] would transform the selected item in the list to the highlighted (keyword) version, in this case "@amazon" with an added space for you to start your query 

- You can click into or arrow back over the highlighted text but as soon as you modify it to anything other than an acceptable search shortcut item, the highlight goes away as an indication that it's no longer recognized as a keyword.

Comment 6

[Ed Lee :Mardak]

I believe adw was asking for 62 uplift. I doubt showing a list let alone the other interactions will be in scope.

Comment 7

[Aaron Benson]

Oh .. yeah that's why I think that part was left out of this first phase. I would think the @keyword would just be plain text in the input field. Deleting it or editing it in any way would break the behavior. We have something similar in product now if you go to the Library > Bookmarks menu. Select "Search Bookmarks" and a "* " is placed in the address bar with no other hints (outside of the list beneath). 

Sorry for the misunderstanding, hopefully this answers the question?

Comment 8

[Drew Willcoxon :adw]

Attachment: Bug 1480505 - Highlight special internal search engine keywords in awesomebar

Add a new selection style for internal search keywords

Comment 9

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #2)
> (4) The keywords do behave like editable text, with special styling. 
> Probably not possible for 62.  But we do style the text in the urlbar when
> it's not focused -- maybe I'm wrong and that's possible when the urlbar is
> focused too, and if so, this might actually be easier than I'm thinking.

I was wrong, this isn't too hard.  The WIP patch I just posted basically 100% works.  It modifies Gecko though so I think this is probably not a good candidate for uplift to beta... not sure that's my call.

I just added a new selection style, like how we have special selection styles solely for URL highlighting in the urlbar.

I'll post a try build so people can play with it if they want.

Comment 10

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f6fd4fac3c25c0e94760bb34547989cb75c4f5f

Comment 11

[Ed Lee :Mardak]

Attachment: screenshot of try selection

Comment 12

[Ed Lee :Mardak]

smaug, could you comment on the code uplift risk for the approach of adding a new selection type as in attachment 8998415 [details]? It looks like additional special types of selection are not that uncommon of a pattern, although this would be an idl change (does that matter much anymore?) to be uplifted in the next couple weeks before 62 beta 20.

Comment 13

[Drew Willcoxon :adw]

An important point about the patch is that it hardcodes colors, which is probably bad because it doesn't play well with non-standard OS themes (thinking GTK dark theme especially) and accessibility (thinking Windows high-contrast modes).  I would need some guidance on how we could handle that.  Do we set up a mapping between some new symbolic colors (which would be used in nsTextFrame.cpp) and the actual colors, which would depend on the theme and whatever else?

Comment 14

[Drew Willcoxon :adw]

Oh, looks like that's what nsTextPaintStyle is for?

Comment 15

[BelFox]

Suggestion (for separate bug): add the same kind of highlighting to user-defined search keywords.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

This doesn't look like something which should be uplifted, unless there is some strong reason.

Comment 17

[Aaron Benson]

From a UX perspective there's a strong reason to uplift this. The feature doesn't look complete without the formatting.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

I would not be ok to uplift the patch which has hard coded color. It would most probably be broken on some themes (OS or FF).
But that should be fixable by using Selection.setColor etc.

But I guess adding new selection type could be ok. (a followup bug could be filed to make the setup more flexible so that one wouldn't need to add new types for every niche reason.)

Comment 19

[Aaron Benson]

(In reply to Olli Pettay [:smaug] from comment #18)
> I would not be ok to uplift the patch which has hard coded color. It would
> most probably be broken on some themes (OS or FF).
> But that should be fixable by using Selection.setColor etc.
> 
> But I guess adding new selection type could be ok. (a followup bug could be
> filed to make the setup more flexible so that one wouldn't need to add new
> types for every niche reason.)

Drew, do you think we can address these concerns and get an uplift?

Comment 20

[Drew Willcoxon :adw]

Olli and I talked on IRC -- there's a simpler way to do this entirely (I think) from the front-end: Selection.SetColor, as he mentioned here in the bug.  I'm playing with that now.  If that works, it'll be waaaaay easier to uplift than the approach in the patch I posted.

Comment 21

[Ed Lee :Mardak]

Here's my quick hack to highlight on typing or clicking from activity stream:

diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml
--- a/browser/base/content/urlbarBindings.xml
+++ b/browser/base/content/urlbarBindings.xml
@@ -1656,6 +1656,24 @@ file, You can obtain one at http://mozilla.org/MPL/2.0/.
           this.controller.startSearch(value);
         ]]></body>
       </method>
+
+      <method name="highlightAlias">
+        <parameter name="alias"/>
+        <body><![CDATA[
+          let s = this.editor.selectionController.getSelection(8);
+          if (!alias) {
+            s.removeAllRanges();
+            return;
+          }
+
+          let t = this.editor.rootElement.firstChild;
+          let r = document.createRange();
+          r.setStart(t, 0);
+          r.setEnd(t, alias.length);
+          s.addRange(r);
+          s.setColors("#2362D7", "#D2E6FD", "#2362D7", "#D2E6FD");
+        ]]></body>
+      </method>
     </implementation>
 
     <handlers>
@@ -2334,6 +2352,12 @@ file, You can obtain one at http://mozilla.org/MPL/2.0/.
       <method name="onResultsAdded">
         <body>
           <![CDATA[
+            if (this.input.mController.matchCount > 0) {
+              const url = this.input.mController.getFinalCompleteValueAt(0);
+              const action = this.input._parseActionUrl(url);
+              this.input.highlightAlias(action && action.params.alias || "");
+            }
+
             // If nothing is selected yet, select the first result if it is a
             // pre-selected "heuristic" result.  (See UnifiedComplete.js.)
             if (this.selectedIndex == -1 && this._isFirstResultHeuristic) {

Comment 22

[Ed Lee :Mardak]

Attachment: quick hack video

Comment 23

[Drew Willcoxon :adw]

Attachment: Bug 1480505 - Highlight special internal search engine keywords in awesomebar

Comment 24

[Drew Willcoxon :adw]

This still needs a test, and I need to check how it looks on Linux.  I'm working on those things now.  We should be able to uplift this if we were able to uplift all the other bugs.

Comment 25

[Ed Lee :Mardak]

[Tracking Requested - why for this release]: UX comment 17 says the feature would not look complete without this highlighting, e.g., attachment 9001751 [details]

I've pushed the phabricator patch rebased against beta to see if anything breaks there:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4e73781aeb17f2b08a3cc10b6b961f15ac8000a

Comment 26

[Ed Lee :Mardak]

Attachment: screenshot linux dark and light theme

Comment 27

[Ed Lee :Mardak]

Comment on attachment 9001772 [details]
Bug 1480505 - Highlight special internal search engine keywords in awesomebar

Ed Lee :Mardak (PTO Aug 11-26) has approved the revision.

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Once this lands on m-c please go ahead and request uplift. We would want QA to have an extra look to verify the changes in beta.

Comment 29

[Drew Willcoxon :adw]

m-c: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1fd371ad9a02cf432ebdb34eedd77084f73d92a

beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e524a27b852461a567ad9c881b2669c190a1c30e

Comment 30

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 9001772 [details]
Bug 1480505 - Highlight special internal search engine keywords in awesomebar

Tim Guan-tin Chien [:timdream] (please needinfo) has approved the revision.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc054d9df7a5
Highlight special internal search engine keywords in awesomebar r=timdream,Mardak

Comment 32

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/dc054d9df7a5

Comment 33

[Ed Lee :Mardak]

Comment on attachment 9001772 [details]
Bug 1480505 - Highlight special internal search engine keywords in awesomebar

Approval Request Comment
[Feature/Bug causing the regression]: Activity Stream top search bug 1479806 mostly uplifted in bug 1482398
[User impact if declined]: Potentially confusing address bar behavior without visual indication
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: Yes, type "@amazon" into the address bar and seeing it highlight.
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: Slightly
[Why is the change risky/not risky?]: This adds just a little bit of extra conditional javascript logic in the address bar.
[String changes made/needed]: None

Comment 34

[Sören Hentzschel]

Attachment: screenshot

If you have configured a own keyword for one of the built-in search engines and enter your own keyword in the address bar this is also highlighted, not only the new "special" keyword. Is this expected? The bug descriptions doesn't sound like it should affect the normal keywords.

I attached a screenshot showing the issue. In my case "g" is a keyword for Google.

Comment 35

[Drew Willcoxon :adw]

I think Ed basically answered by updating the bug summary, but for context, yes, it's expected -- I asked Ed about it the other day and he asked Aaron about it, and yes, we want to highlight all search aliases.  Thanks for asking, it's good to clarify that here in the bug.

Comment 36

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I can confirm that the implementation of the highlight search engine keywords was successfully made for awesomebar accross platforms (Windows 10 x64, Ubuntu 18.04 x64, macOS 10.13.4), using 63.0a1 (2018-08-19).   
There is still one potential issue here: when introducing a search keyword in the location bar, its highlight disappear after moving down to a search suggestion and then coming back to it. Is this expected?

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9001772 [details]
Bug 1480505 - Highlight special internal search engine keywords in awesomebar

One more tweak for search keywords to highlight them in the address bar. 
Taking this for beta 19.  If there is further followup from comment 36, please open a new bug.

Comment 38

[Ed Lee :Mardak]

(In reply to Iulia Cristescu, QA [:JuliaC] from comment #36)
> There is still one potential issue here: when introducing a search keyword
> in the location bar, its highlight disappear after moving down to a search
> suggestion and then coming back to it. Is this expected?
We'll want to get some design feedback, but I would guess the highlight should return when moving down and back up to the address bar. Please file a followup, and I would guess we won't fix it for 62.

Comment 39

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/402e376b0b09

Comment 40

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Thank you, Ed, for your quick answer! 
I also can confirm that the highlight search engine keywords implementation is successfully made on 62.0b19 build1 (20180820172315) across platforms (Windows 10 x64, Ubuntu 16.04 x86, macOS 10.12.6). The functionality works as expected on Default, Light and Dark themes, and also on specific high contrast platform settings.  Apart from bug 1484737, there were no side issues found. As this will be assessed separately, I'll set the flags accordingly.

Comment 41

[Gleb]

Is it possible to disable it or change the colors? I didn't have problems with seeing the keyword before new change was implemented, but now I barely see the keyword at all. I wonder who was the person responsible for choosing these colors (light blue on blue background, they barely matches other colors used in interface as well). Did he or she ever thought of people with vision problems? Very disappointing that you make such poor decision and add too much noise to the interface.