1110771 - One-click search buttons should have a right-click menu

Status: VERIFIED FIXED

(Firefox :: Search, defect, P4)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

There are a couple of advanced actions that users should be able to perform from a one-click button. We can expose them in a right click menu.

Than menu should initially include the following two items:
- Search in a new tab
- Set %enginname as default search engine

Comment 1

[Tim Nguyen :ntim]

Possibly with the Engine name as header of the context menu as well.

Comment 2

[Blutin]

+1

right-click menu + "Set as default search engine" would allow to select a new default search engine in 3 clicks (instead of 5 with new search bar and 2 with former search).

Also it would be better to display the search engines when the magnifying glass is clicked, even when nothing has been typed; instead of only have the "Change Search Settings" menu in that case.

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Initial patch

This patch adds a context menu for the one off buttons.
Some notes:
- There's a weird issue where if you open a context menu, and then click a one off button to search (even if you didn't click any of the menu items), the suggestions popup doesn't get closed. I worked around this by adding a manual hidePopup() which doesn't seem to change any existing behavior - but if there's a better fix that'd be great. Please see the comment in the code.
- This does not (yet) add a context menu to the suggestions list to allow opening them in a new tab.
- I wonder if there's a better alternative to checking the target of all the popup* events (since context menus also trigger these events).
- The l10n is basic at the moment and doesn't include the engine name in the label or anything.

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Screenshot of initial patch

Just wondering if this matches what you had in mind. Note that I haven't yet included a provision to add the engine name to the set-default label. I'll do that for the next patch, though I'm wondering if it'll make the label a little too long.

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Also, I haven't updated the code that records the one-off search in a new tab from the context menu in telemetry. I'd like to confirm what the type should be first. Do we need a new one (or does one exist already) for context menu searches?

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Initial patch v1.1

Move the hidePopup() call above the handleSearchCommand() call (forgot to qref :( )

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8609699 [details] [diff] [review]
Initial patch v1.1

Review of attachment 8609699 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like good progress :-).

(In reply to Nihanth Subramanya [:nhnt11] from comment #3)

> - I wonder if there's a better alternative to checking the target of all the
> popup* events (since context menus also trigger these events).

Maybe setting the phase="target" attribute (see https://developer.mozilla.org/en-US/docs/XBL/XBL_1.0_Reference/Event_Handlers) on the <handler> elements would help.

::: browser/base/content/urlbarBindings.xml
@@ +1102,5 @@
> +      <xul:menupopup anonid="search-one-offs-context-menu">
> +        <xul:menuitem anonid="search-one-offs-context-open-in-new-tab"
> +                      label="&searchInNewTab.label;"/>
> +        <xul:menuitem anonid="search-one-offs-context-set-default"
> +                      label="&searchSetAsDefault.label;"/>

I think these 2 labels should have accesskeys.

@@ +1169,5 @@
> +            target.classList.contains("dummy")) {
> +          event.preventDefault();
> +          return;
> +        }
> +        this._contextEngine = target.engine;

Should this be cleared in the popuphidden event?

Comment 8

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8609662 [details]
Screenshot of initial patch

Great, thanks for working on this!

Comment 9

[Drew Willcoxon :adw]

(In reply to Nihanth Subramanya [:nhnt11] from comment #5)
> Also, I haven't updated the code that records the one-off search in a new
> tab from the context menu in telemetry. I'd like to confirm what the type
> should be first. Do we need a new one (or does one exist already) for
> context menu searches?

Flo can weigh in too, but I would think that we would want to be able to distinguish these new searches from others.  Looks like that would require a new `source`.  oneoff-contextmenu?

Comment 10

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch

Addressed feedback comments and added a new "oneoff-context" search source.

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> Comment on attachment 8609699 [details] [diff] [review]
> Initial patch v1.1
> 
> Review of attachment 8609699 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks like good progress :-).
> 
> (In reply to Nihanth Subramanya [:nhnt11] from comment #3)
> 
> > - I wonder if there's a better alternative to checking the target of all the
> > popup* events (since context menus also trigger these events).
> 
> Maybe setting the phase="target" attribute (see
> https://developer.mozilla.org/en-US/docs/XBL/XBL_1.0_Reference/
> Event_Handlers) on the <handler> elements would help.


This did not work. The autocomplete binding doesn't behave well with popups (including context menus). I had trouble with this on bug 1113747 too. My new solution is to stopPropagation() of popup events on the menupopup.

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8631349 [details] [diff] [review]
Patch

Review of attachment 8631349 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/urlbarBindings.xml
@@ +1496,5 @@
> +        if (anonid == "search-one-offs-context-set-default") {
> +          let currentEngine = Services.search.currentEngine;
> +
> +          // Make the target button of the context menu reflect the current
> +          // search engine first.

After doing this, the order of the one-off buttons won't match the order of the engines set in the preference dialog. I assume you wrote this hack to avoid a flickering of the panel if we just completely rebuilt the one-off engine list? I would suggest expanding the comment a bit.

::: browser/components/search/content/search.xml
@@ +492,5 @@
> +          else if (aForceNewTab) {
> +            where = "tab";
> +            if (Services.prefs.getBoolPref("browser.tabs.loadInBackground")) {
> +              where += "-background";
> +            }

nit: The rest of the code in this method doesn't seem to have {} for single line ifs.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +452,5 @@
>       consider translating it as if it said only "Search Settings". -->
>  <!ENTITY changeSearchSettings.button  "Change Search Settings">
>  
> +<!ENTITY searchInNewTab.label         "Search in a new tab">
> +<!ENTITY searchInNewTab.accesskey     "T">

I wonder if it should be Search in New Tab (ie. remove the 'a' and capitalize New Tab) to match "Open Link in New Tab". Anyway, the capitalization of the accesskey should match what you end up doing for the label.

@@ +454,5 @@
>  
> +<!ENTITY searchInNewTab.label         "Search in a new tab">
> +<!ENTITY searchInNewTab.accesskey     "T">
> +<!ENTITY searchSetAsDefault.label     "Set as default search engine">
> +<!ENTITY searchSetAsDefault.accesskey "D">

same here.

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v2

Addressed comments

Comment 13

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v2.1

Updated comments.

Comment 14

[Florian Quèze [:florian]]

Comment on attachment 8640793 [details] [diff] [review]
Patch v2.1

Review of attachment 8640793 [details] [diff] [review]:
-----------------------------------------------------------------

Have you considered writing a test for this?

::: browser/base/content/urlbarBindings.xml
@@ +1195,5 @@
> +        menu.addEventListener("popupshowing", listener);
> +        menu.addEventListener("popuphiding", listener);
> +        menu.addEventListener("popupshown", aEvent => {
> +            this._ignoreMouseEvents = true;
> +            aEvent.stopPropagation();

nit: 2 space indent.

@@ +1198,5 @@
> +            this._ignoreMouseEvents = true;
> +            aEvent.stopPropagation();
> +        });
> +        menu.addEventListener("popuphidden", aEvent => {
> +            this._ignoreMouseEvents = false;

same here.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +451,5 @@
>  <!ENTITY changeSearchSettings.button  "Change Search Settings">
>  
> +<!ENTITY searchInNewTab.label         "Search in New Tab">
> +<!ENTITY searchInNewTab.accesskey     "T">
> +<!ENTITY searchSetAsDefault.label     "Set As Default Search Engine">

I don't think 'as' should be capitalized here.

Comment 15

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v3

Sorry about the nits, had some editor issues :(

There was precedent for capitalizing "As": https://dxr.allizom.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#497

Comment 16

[Florian Quèze [:florian]]

What's the status of this bug?

Comment 18

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v3.1

Forgot to mark this checkin-needed.
Green try push after unbitrotting: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8d941a4f0f0

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e2df07d3f7ee

Comment 20

[Phil Ringnalda (:philor)]

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

Comment 21

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

Checked the contextual menu for one-click search engines using Developer Edition 43.0a2 2015-09-23 under Win 7 64-bit, Mac OS X 10.9.5 and Ubuntu 12.04 32-bit and found some possible issues. Can someone please review them and let me know if they worth filling?

1. Right click on a one-click search engine and without selecting anything, right click on another engine:
 Win: The search panel is closed while the contextual menu remains visible
 Ubuntu: The contextual menu for the first selected engine is closed
 Mac: The contextual menu for the second search engine opens (This is, I believe, the correct behavior)

2. Minimize the browser while the search engine contextual menu is opened:
 Win: The contextual menu is closed first. The minimize icon needs to be selected twice in order to minimize the browser.
 Ubuntu: The contextual menu is closed first, selecting minimize twice closes the Search panel. 
 Mac: The browser is minimized

3. All platforms: Due to bug 1204241, a search engine can be set as default using keyboard shortcuts and still appear in one-click list so one can also use the contextual menu to do that

Comment 22

[arni2033]

(In reply to Petruta Rasa [QA] [:petruta] from comment #21)
> 1. Right click on a one-click search engine and without selecting anything, right click on another engine
Oh wow, it's already mentioned (I only checked blocking bugs in the header before reporting).
However, I think you can try another steps on Ubuntu (I described them in bug 1171394):
> Before right-clicking another engine: move mouse over one of non-disabled menuitems,
> press left mouse button, then move mouse away from the menu. Then right-click another engine.
I believe that you'll get Win behavior as a result:  "only second context menu is visible".

All connected bugs have 1 thing in common (at least on Win): 'mousedown' event is being canceled.
BTW in bug 1205619 Neil Deakin seem to already found the root of the problem.

Comment 23

[Md. Rahimul Islam]

I have successfully reproduce this bug on firefox nightly 37.0a1 (2014-12-12)
with windows 10 (32 bit)
Mozilla/5.0 (Windows NT 10.0; rv:37.0) Gecko/20100101 Firefox/37.0

I found this fix on latest aurora  43.0a2 (2015-10-15)

Mozilla/5.0 (Windows NT 10.0; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID : 20151015004024

[bugday-20151014]

Comment 24

[Nazir Ahmed Sabbir [:NaSb]]

Reproduced this bug on Nightly 37.0a1 (2014-12-12) in Linux,64 bit

This Bug is now verified as fixed on 

Latest Developer Edition 43.0a2 (2015-10-15)
Build ID: 20151015004024
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0

As it is also verified on Windows (Comment 23), Marking it as verified!