Highlight search engine keywords in awesomebar

VERIFIED FIXED in Firefox 62

Status

()

P3
enhancement
VERIFIED FIXED
4 months ago
2 months ago

People

(Reporter: k88hudson, Assigned: adw)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62+ verified, firefox63 verified)

Details

(Whiteboard: [fxsearch])

User Story

https://mozilla.invisionapp.com/share/U7N6JJGZ4RB#/screens

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

4 months ago
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.
(Reporter)

Updated

4 months ago
Severity: normal → enhancement
Priority: -- → P2
(Reporter)

Updated

4 months ago
Priority: P2 → P3
(Reporter)

Comment 1

4 months ago
This is not required for MVP but it is desired if possible.
(Reporter)

Updated

4 months ago
Whiteboard: [fx-search]
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.
Flags: needinfo?(abenson)
Iteration: --- → 63.4 - Aug 20

Comment 3

4 months ago
Created attachment 8997139 [details]
mockup of "switch to tab" -> "@amazon"

Comment 4

4 months ago
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

4 months ago
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.
Flags: needinfo?(abenson)

Comment 6

4 months ago
I believe adw was asking for 62 uplift. I doubt showing a list let alone the other interactions will be in scope.
Flags: needinfo?(abenson)

Comment 7

4 months ago
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?
Flags: needinfo?(abenson)
(Reporter)

Updated

4 months ago
Whiteboard: [fx-search] → [fxsearch]
Assignee: nobody → adw
Status: NEW → ASSIGNED
Created attachment 8998415 [details]
Bug 1480505 - Highlight special internal search engine keywords in awesomebar

Add a new selection style for internal search keywords
(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 11

4 months ago
Created attachment 8998427 [details]
screenshot of try selection

Comment 12

4 months ago
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.
Flags: needinfo?(bugs)
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?
Oh, looks like that's what nsTextPaintStyle is for?

Comment 15

4 months ago
Suggestion (for separate bug): add the same kind of highlighting to user-defined search keywords.
This doesn't look like something which should be uplifted, unless there is some strong reason.
Flags: needinfo?(bugs)

Comment 17

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

Updated

4 months ago
Flags: needinfo?(bugs)
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.)
Flags: needinfo?(bugs)

Comment 19

4 months ago
(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?
Flags: needinfo?(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.
Flags: needinfo?(adw)

Comment 21

4 months ago
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

4 months ago
Created attachment 9001751 [details]
quick hack video
Created attachment 9001772 [details]
Bug 1480505 - Highlight special internal search engine keywords in awesomebar
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

4 months ago
[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
tracking-firefox62: --- → ?

Comment 26

4 months ago
Created attachment 9001795 [details]
screenshot linux dark and light theme

Comment 27

4 months ago
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.
Attachment #9001772 - Flags: review+
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.
status-firefox62: --- → affected
tracking-firefox62: ? → +
Flags: qe-verify+
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.
Attachment #9001772 - Flags: review+

Comment 31

4 months ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dc054d9df7a5
Highlight special internal search engine keywords in awesomebar r=timdream,Mardak

Comment 32

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dc054d9df7a5
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63

Comment 33

4 months ago
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
Attachment #9001772 - Flags: approval-mozilla-beta?

Comment 34

4 months ago
Created attachment 9002290 [details]
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.

Updated

4 months ago
Summary: Highlight special internal search engine keywords in awesomebar → Highlight search engine keywords in awesomebar
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.
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?
Flags: needinfo?(edilee)
Flags: needinfo?(adw)
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.
Attachment #9001772 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 38

4 months ago
(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.
Flags: needinfo?(edilee)
Flags: needinfo?(adw)

Updated

4 months ago
Blocks: 1484737
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.
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified
status-firefox63: fixed → verified
Flags: qe-verify+
Attachment #8998415 - Attachment is obsolete: true

Comment 41

4 months ago
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.
Blocks: 1496772
You need to log in before you can comment on or make changes to this bug.