Closed
Bug 1110771
Opened 10 years ago
Closed 9 years ago
One-click search buttons should have a right-click menu
Categories
(Firefox :: Search, defect, P4)
Tracking
()
VERIFIED
FIXED
Firefox 43
Tracking | Status | |
---|---|---|
firefox43 | --- | verified |
People
(Reporter: phlsa, Assigned: nhnt11)
References
(Depends on 2 open bugs)
Details
(Whiteboard: [ui][fxsearch])
Attachments
(2 files, 6 obsolete files)
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
Updated•10 years ago
|
Flags: qe-verify+
Flags: firefox-backlog+
Hardware: x86 → All
Comment 1•10 years ago
|
||
Possibly with the Engine name as header of the context menu as well.
+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.
Updated•10 years ago
|
Priority: -- → P4
Whiteboard: [fxsearch][searchui]
Updated•10 years ago
|
Rank: 45
Whiteboard: [fxsearch][searchui] → [ui][fxsearch]
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → nhnt11
Assignee | ||
Comment 3•10 years ago
|
||
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.
Attachment #8609661 -
Flags: feedback?(florian)
Assignee | ||
Comment 4•10 years ago
|
||
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.
Attachment #8609662 -
Flags: feedback?(philipp)
Assignee | ||
Comment 5•10 years ago
|
||
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?
Assignee | ||
Comment 6•10 years ago
|
||
Move the hidePopup() call above the handleSearchCommand() call (forgot to qref :( )
Attachment #8609661 -
Attachment is obsolete: true
Attachment #8609661 -
Flags: feedback?(florian)
Attachment #8609699 -
Flags: feedback?(florian)
Comment 7•10 years ago
|
||
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?
Attachment #8609699 -
Flags: feedback?(florian) → feedback+
Reporter | ||
Comment 8•10 years ago
|
||
Comment on attachment 8609662 [details]
Screenshot of initial patch
Great, thanks for working on this!
Attachment #8609662 -
Flags: feedback?(philipp) → feedback+
Comment 9•10 years ago
|
||
(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?
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•9 years ago
|
||
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.
Attachment #8609699 -
Attachment is obsolete: true
Attachment #8631349 -
Flags: review?(florian)
Comment 11•9 years ago
|
||
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.
Attachment #8631349 -
Flags: review?(florian) → feedback+
Assignee | ||
Comment 12•9 years ago
|
||
Addressed comments
Attachment #8631349 -
Attachment is obsolete: true
Attachment #8640792 -
Flags: review?(florian)
Assignee | ||
Comment 13•9 years ago
|
||
Updated comments.
Attachment #8640792 -
Attachment is obsolete: true
Attachment #8640792 -
Flags: review?(florian)
Attachment #8640793 -
Flags: review?(florian)
Comment 14•9 years ago
|
||
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.
Attachment #8640793 -
Flags: review?(florian) → review+
Assignee | ||
Comment 15•9 years ago
|
||
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
Attachment #8643246 -
Flags: review+
Assignee | ||
Comment 18•9 years ago
|
||
Forgot to mark this checkin-needed.
Green try push after unbitrotting: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8d941a4f0f0
Attachment #8640793 -
Attachment is obsolete: true
Attachment #8643246 -
Attachment is obsolete: true
Flags: needinfo?(nhnt11)
Attachment #8662982 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 19•9 years ago
|
||
Keywords: checkin-needed
Comment 20•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Comment 21•9 years ago
|
||
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
Updated•9 years ago
|
Flags: needinfo?(nhnt11)
Comment 22•9 years ago
|
||
(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•9 years ago
|
||
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•9 years ago
|
||
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!
Updated•8 years ago
|
Flags: needinfo?(nhnt11)
You need to log in
before you can comment on or make changes to this bug.
Description
•