1136131 - Provide access to all search providers in context menu selection

Status: NEW

(Firefox :: Search, enhancement, P5)

Description

[Ash Grigas]

Build ticket for UI bug https://bugzilla.mozilla.org/show_bug.cgi?id=1107194

Comment 1

[Ash Grigas]

See attachment on bug 1107194. Sub-menu is used to list providers. If user has more providers than default set, native scroll pattern for each platform (Mac OS, Windows, etc) should be used to support scrolling and navigation.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

So this is essentially "implement attachment 8568096 [details], ignoring the bit about scrolling behavior being different than standard menu behavior".

Comment 3

[Ash Grigas]

yes

Comment 5

[Steph]

Hi,

Could you guys consider rising the priority of this ? Usability has been hampered for me with the new search, and this feature is the one thing that will finally make the new search system better than the old :)

Thanks!

Comment 6

[Drew Willcoxon :adw]

I've asked my intern, Nihanth, to work on this.

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch

I modified the _loadSearch API to accept an optional engine parameter so that allows it to be used with any search engine. Not sure if this approach is best - let me know!

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

This patch generates the list of alternate providers every time the context menu is opened, by the way, because I figured it was overkill to populate the menuitems once and then add/remove items when the user modifies the list of search engines.

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v1.1

Added a comment where we return early when there's only one visible provider.

Comment 10

[Drew Willcoxon :adw]

Comment on attachment 8607944 [details] [diff] [review]
Patch v1.1

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

This looks great.  I would r+ it but there are some tests we need to fix as part of this patch.  We should also check with Mike that using the "contextmenu" search purpose for this new menu is OK with our search partners.  (I should have done that yesterday but didn't think of it, sorry.)

::: browser/base/content/browser-context.inc
@@ +316,5 @@
>                  accesskey="&keywordfield.accesskey;"
>                  oncommand="AddKeywordForSearchField();"/>
>        <menuitem id="context-searchselect"
>                  oncommand="BrowserSearch.loadSearchFromContext(this.searchTerms);"/>
> +      <menu id="context-otherProviders" label="&otherProvidersMenu.label;"/>

I think both the ID and the DTD entity should have "search" in them.  How about context-searchselect-otherproviders and searchOtherProvidersMenu.label?

::: browser/base/content/browser.js
@@ +3534,5 @@
>     * This should only be called from the context menu. See
>     * BrowserSearch.loadSearch for the preferred API.
>     */
> +  loadSearchFromContext: function (terms, engine) {
> +    if (BrowserSearch._loadSearch(terms, true, "contextmenu", engine))

Let's keep the braces on this if since they're already there.

(You'll find that different people on the team have different opinions on this, but most of the code I've seen recently uses braces.  However, generally reviewers on our team don't make a big deal about it -- from what I've seen anyway.  FYI, there is a Mozilla-wide style guide that I guess we generally follow, but people definitely do deviate from it sometimes: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures)

::: browser/base/content/nsContextMenu.js
@@ +1782,5 @@
> +    let listener = event => {
> +      let name = event.target.getAttribute("label");
> +      BrowserSearch.loadSearchFromContext(selectedText,
> +          Services.search.getEngineByName(name));
> +    }

Nit: };

@@ +1787,5 @@
> +    for (let engine of engines) {
> +      if (engine == currentEngine)
> +        continue;
> +      let item = document.createElementNS(
> +        "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "menuitem");

Please put this string in a variable where you first use it above, and then use the variable in both places.

Comment 11

[Drew Willcoxon :adw]

Comment on attachment 8607944 [details] [diff] [review]
Patch v1.1

Mike, this patch adds a new "Search other providers" submenu in the context menu.  See comment 2 for the UX spec.  It uses the existing "contextmenu" search purpose for clicks on those providers.  Is that cool?

Comment 12

[Mike Connor [:mconnor]]

Based on IRC discussion, I'd like us to hold off on this change.  Adding a 7-10 item submenu to a 6 item menu feels super heavy.  This isn't frequently-used UI, so giving it even more UI affordance feels like a step in the wrong direction.

In addition, there are competing plans in conjunction with partners to explore a richer, more DWIM-y alternative, so I'd like to avoid churn until that's sorted.  cc-ing Kev to make sure this is on his radar as well.

Comment 13

[Drew Willcoxon :adw]

Mike, one reason you gave in IRC for this UX feeling heavy is that context menus shouldn't have submenus as a general rule.  It should be pointed out that our context menu already has submenus for some contexts: media playback, video casting, frames, dictionaries, and the links and pages marking feature of the social API: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-context.inc#318

That's not a rebuttal to the idea that submenus are bad in the context menu.  It's just to say there's precedent for it.  Those contexts are probably much less frequent for most people than the search context would be, though.

Comment 14

[(inactive) Nihanth Subramanya [:nhnt11]]

Unassigning myself. I don't think this is WONTFIX (there's an alternative planned right?) so I'm leaving it as NEW.

Comment 15

[Steph]

If the competing plans you are talking about are discussed in an open bug, would you mind putting the link here so we can follow what's going on ? Thanks :)

Comment 16

[quasicomputational]

I'd really like this still.

What about allowing search providers to be toggled into or out of the context menu? By default, only the default search provider could be listed and the default UI wouldn't be any heavier, but, if you want others to be available, you can go to about:preferences#search and flip their bits.

Comment 18

[Worcester12345]

Source code for the extension which did this very nicely is located here:

https://github.com/benbasson/contextsearch

Comment 19

[Worcester12345]

(In reply to Worcester12345 from comment #18)

Source code for the extension which did this very nicely is located here:

https://github.com/benbasson/contextsearch

So, I found this, and it is pretty simple, so copying it here:

<?xml version="1.0" encoding="UTF-8"?>

<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
xmlns:em="http://www.mozilla.org/2004/em-rdf#">

<Description about="urn:mozilla:install-manifest">

<em:id>{902D2C4A-457A-4EF9-AD43-7014562929FF}</em:id>
<em:type>2</em:type>
<em:version>0.6.4</em:version>

<em:targetApplication>
  <Description>
    <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
    <em:minVersion>45.0</em:minVersion>
    <em:maxVersion>47.*</em:maxVersion>
  </Description>
</em:targetApplication>

<em:name>Context Search</em:name>
<em:description>Expands the context menu's 'Search for' item into a list of search engines.</em:description>
<em:creator>Ben Basson</em:creator>
<em:homepageURL>http://www.basson.at/firefox-addons/contextsearch</em:homepageURL>

<em:contributor>Sebastian Lisken</em:contributor>
<em:contributor>David Pi (zh-CN locale)</em:contributor>
<em:contributor>Tim Babych (uk-UA locale)</em:contributor>
<em:contributor>Marcelo Alejandro Soto Rodríguez (es-CL, es-ES locales)</em:contributor>
<em:contributor>Julien Vaux (fr-FR locale)</em:contributor>
<em:contributor>Maxim Yanchenko (ru-RU locale)</em:contributor>
<em:contributor>Maja Zor (de-DE, sl-SI locales)</em:contributor>
<em:contributor>eking (cs locale)</em:contributor>
<em:contributor>Jiipee (fi-FI locale)</em:contributor>
<em:contributor>barryoni (he, he-IL locales) </em:contributor>
<em:contributor>Ghelman (pt-BR locale)</em:contributor>
<em:contributor>Lakrits (sv-SE locale)</em:contributor>
<em:contributor>omrakin (tr locale)</em:contributor>
<em:contributor>markh (nl locale)</em:contributor>
<em:contributor>kobachi (ja-JP locale)</em:contributor>
<em:contributor>on9123 (zh-TW locale)</em:contributor>
<em:contributor>mdr.ksk (et-EE locale)</em:contributor>
<em:contributor>James Abbatiello (e10s compatibility)</em:contributor>

</Description>
</RDF>