203959 - "search web for <selected text>" should use the same search engine as the search bar (searchplugin box)

Status: VERIFIED FIXED

(Firefox :: Search, enhancement, P3)

Description

[Josh Peters]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4a) Gecko/20030331 Phoenix/0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4a) Gecko/20030331 Phoenix/0.5

This is a feature request, first off I love the google search box--I've added
more than a few sites to my searchplugin directory.  Currently, when a user
highlights text, and right clicks it, you can search google for that text.  I
think that you should search using whatever searchplugin you're currently using.
 For instance, I've got a search php.net file written, and I'd love to be able
to highlight a function name from a script I'm perusing on groups.google.com and
search php.net using this functionality.  Right now, I have to copy the text
into the box and search that way--not too inconvenient, but could be better.

Reproducible: Always

Steps to Reproduce:
1. Go to a website with text
2. Highlight some text
3. Right-click and choose "Web Search for..."

Actual Results:  
Searched google.com for the text

Expected Results:  
Used the searchplugin site for finding the text

Comment 1

[Mike Connor [:mconnor]]

This makes a lot of sense, I wouldn't mind this at all (even though I use google
there anyway).

-->  All/All, NEW

Comment 2

[Stefan Eskelid]

If this is implemented, shouldn't it be indicated somehow that the searchplugin
in searchbar is actually used?! It would be cool if the contextmenu said Search
<plugin name> for "...", i.e. Search Google for "..." or Search dmoz.org for "..."

Comment 3

[johncallahan]

I'm quite the novice, but may I make a suggestion?

It seems to me that what you really want to do is link the contextual menu
searches with your *Quick Searches*.

In other words, first you define your quick searches in the Bookmarks manager as
normal. Then, when you're reading a web page, you highlight text and right-click
it. Currently, one of the items in the contextual menu is "Web Search for
'<highlighted text>'." I think what you want instead is a contextual menu item
that says: Search for "<highlighted text>" at ...

Clicking that item in the contextual menu would bring up a submenu listing the
sites you already defined in your quicksearches.

Comment 4

[Alexei Varganov]

By the way, last time I checked, Mozilla suit had this feature working properly.
In Mozilla suit it's not always google that does your web searches when you
click on highlighted text. 

Comment 5

[PikeUK]

Attachment: Patch: Web Search uses current engine

Here's my go at this, the original function is so full of unused suite stuff
that I just started from scratch.
Basically it does this:
* If selected text is an URL, open it.
* If not and the search bar is available and the current engine is not "Find in
this Page" use that engine.
* Else use default search (i.e. google).

Comment 6

[Steffen Wilberg]

Comment on attachment 137151 [details] [diff] [review]
Patch: Web Search uses current engine

>+    var currentEngine = searchBar ? searchBar.getAttribute("searchengine") : null;
Can't you just say var currentEngine = searchBar.getAttribute("searchengine");
?
Because you check for searchBar here again:
>+    if (searchBar && currentEngine != "__PhoenixFindInPage") {


>+      var defaultSearchURL = null;
>+      try {
>+        defaultSearchURL = gPrefService.getComplexValue("browser.search.defaulturl",
>+                                        Components.interfaces.nsIPrefLocalizedString).data;
>+      } catch (ex) {
>       }
>+      openURL = defaultSearchURL + escapedSearchStr;
I've just fixed a broken profile, where browser.search.defaulturl didn't exist
(bug 227946).
When searching for "foo", it tried to open http://nullfoo/, which couldn't be
found of course.

Comment 7

[Steffen Wilberg]

->Pike. Pike, please have a loook at comment 6.

Comment 8

[PikeUK]

> Can't you just say var currentEngine = searchBar.getAttribute("searchengine");
But if the search bar has been removed from the toolbar, you would get an error
like "searchBar has no properties", though maybe there's a better way to write
that section to avoid the double check of searchBar, I'm not sure.

> When searching for "foo", it tried to open http://nullfoo/, which couldn't be
> found of course.
Oh, whenever I have had this problem I get an error with navigatorRegionBundle
not loading properly, which breaks the function. So it looks like I still need a
secondary fallback to the URL from the stringbundle in case the pref fails.

Comment 9

[Steffen Wilberg]

Pike, you're right, I didn't try it with a removed searchbar.

As for the second issue, you can simulate that by removing
US.jar/locale/US/browser-region/region.properties.
You can solve that by inserting a return in the curly brackets of the catch
clause. This way just nothing happens.
If you want to make some noise instead, insert a
throw("browser.search.defaulturl can't be queried from
chrome://browser-region/locale/region.properties."); that replaces one exception
by another, but a more readable one.

Comment 10

[PikeUK]

Attachment: Updated patch

Ah I see what you mean now, okay here is a version with the throw.

Comment 11

[Steffen Wilberg]

Comment on attachment 137318 [details] [diff] [review]
Updated patch

Fine! Is it ready for review?

Comment 12

[PikeUK]

I thought after creating that last patch the maybe there should be a pref to
enable or disable this, i.e.:

  var escapedSearchStr = encodeURIComponent(searchStr);
  var useDefaultSearch = false;
  try {
    useDefaultSearch = gPrefService.getBoolPref("browser.search.usedefault");
  } catch (ex) {
  }

  // If the search bar is available and "Find in this Page" is not selected then
use it
  if (!useDefaultSearch && currentEngine && currentEngine !=
"__PhoenixFindInPage") {

Then adding this to all.js:

  // always use the default search for Web Search
  pref("browser.search.usedefault", false);

Is this a good idea or do you think I should just submit the existing patch for
review?

Comment 13

[Steffen Wilberg]

I don't think we need a pref. Why should web search use a different (default)
search engine than the search bar? I'd expect that web search always uses the
engine shown in the search bar. Using another engine is confusing, and that's
what this bug is about. But let the devs decide. :)

I'd suggest Pierre ([e-mail]) since he made the last changes to this code:
> //XXXpch: this routine needs to be cleaned up.

Comment 14

[Ben Basson]

The idea of using the currently selected plugin is good, but wouldn't it be
better (and a lot simpler) to follow the "submenu" concept outlined here:
http://forums.mozillazine.org/viewtopic.php?p=322320#322320

Comment 15

[Mike Connor [:mconnor]]

Not really, that would be a more complex user experience (having to select which
plugin to use).  As an extension it has potential, though.

Comment 16

[PikeUK]

Comment on attachment 137318 [details] [diff] [review]
Updated patch

This patch no longer applies, I'll update it shortly.

While I'm at it, should I change the menu item to say "Open %S" when a plain
URL is selected so it is obvious what will happen when clicked or is it more
consistent to always keep it as "Search Web for %S"?

Comment 17

[Mike Connor [:mconnor]]

Pike, I haven't even looked at the patch yet, but if highlighted text is going 
to open in as a URL, and not be a search term, there should absolutely be an 
option like "Open Selected Text as Link"

What's the result if Find in this Page is selected?  Do we just fail with 
feedback, or use Google, or what?

Comment 18

[PikeUK]

(In reply to comment #17)
> Pike, I haven't even looked at the patch yet, but if highlighted text is going 
> to open in as a URL, and not be a search term, there should absolutely be an 
> option like "Open Selected Text as Link"

The code to open plain links is already built into the existing web search
feature, it's just broken at the moment so I fixed it in this patch. Now, I'm
wondering whether instead I should have split that part out into a separate bug
since this bug doesn't explicitly cover that.

The other issue is whether to have a single menu item and rename as appropriate
(i.e. "Search Web for" or "Open") or to have two items and hide one (I'm
assuming you'd never want to search against a plain text URL, only open it).
I'll have to check the existing code to see if there are any similar situations
and see what they do.

> What's the result if Find in this Page is selected?  Do we just fail with 
> feedback, or use Google, or what?

If Find in Page is selected the code falls back to using the default search
engine as defined in the current locale package (i.e. Google).

In terms of feedback I did think about changing the menu item to say "Search
Google for %S" and so on, the only obvious problem is that for the life of me I
can't find a way to find the name of the currently selected search engine (I
suppose I could tweak search.xml to save the value somewhere when one is
selected) or maybe I'm just missing something obvious.

Comment 19

[Steffen Wilberg]

I'd go for "Open Selected Text as Link" as well.

> In terms of feedback I did think about changing the menu item to say "Search
> Google for %S" and so on,
That would be really nice since a lot of search plugins don't search the "entire
web", but only a certain site, e.g. bugzilla.m.o.

Comment 20

[PikeUK]

Attachment: Patch: Web Search uses current engine v2

This patch is updated to the latest version of the source tree and also
improves it in a couple of ways.
1. Now shows an "Open Selected Text as Link" menu item in place of the search
item when a plain URL is selected.
2. The "Search Web for %S" item now includes the name of the current search
engine, e.g. "Search IMDB for %S" (it defaults back to "Web" anytime the
current search engine name is unknown).

One thing to note is that because the name of the current Search Engine is not
currently stored anywhere (I couldn't find it anyway), when you first apply the
patch it will still say "Search Web". The first time that you select a
different engine in the search box the correct name will be stored and from
then on it will work as expected (p.s. this is not a problem on new profiles
only existing ones).

Comment 21

[PikeUK]

Attachment: Patch: Web Search uses current engine v2 (updated)

Updated to latest source version again.

Comment 22

[Mike Connor [:mconnor]]

Comment on attachment 148012 [details] [diff] [review]
Patch: Web Search uses current engine v2 (updated)

moving the review request to myself so I don't forget about this.

in my cursory look it seems to be good

Comment 23

[Jaap A. Haitsma]

I just filed a bug ( bug 248173 ) to request the incorporation of context search
extension http://forums.mozillazine.org/viewtopic.php?t=75294 in FF. Isn't that
a better option than the patch that is proposed overhere???

Selecting the search engine in the search bar and then dueing the context search
does not seem really intuitive to me

Comment 24

[Josh Peters]

(In reply to comment #23)
> I just filed a bug ( bug 248173 ) to request the incorporation of context search
> extension http://forums.mozillazine.org/viewtopic.php?t=75294 in FF. Isn't that
> a better option than the patch that is proposed overhere???
> 
> Selecting the search engine in the search bar and then dueing the context search
> does not seem really intuitive to me

This was discussed above and at least I agree with the notion that a context
menu here is unnecessary.  My vote would be to keep the extension as it is, and
finish implementing the feature laid out here.

Comment 25

[Mike Connor [:mconnor]]

I really expect to have this reviewed tomorrow night, i haven't forgotten, work
is my main focus at this moment though.

Comment 26

[PikeUK]

Comment on attachment 148012 [details] [diff] [review]
Patch: Web Search uses current engine v2 (updated)

This needs a bit of work now with the Find Bar/Locale reshuffle, not sure if it
also needs to support Jesse's tab opening patch.

Comment 27

[Worcester12345]

I like the idea of this as a context menu. Maybe a few ideas can be borrowed
from the extension, and let the extension handle the extras?

Comment 28

[Mike Connor [:mconnor]]

Comment on attachment 148012 [details] [diff] [review]
Patch: Web Search uses current engine v2 (updated)

pike, if this is going to go into 1.0, that update needs to happen soon, due to
the l10n impact.

Comment 29

[PikeUK]

Attachment: Patch: Web Search uses current engine v2 (updated again)

Updated to latest Aviary (FastFind, localisation changes, etc), sorry about the
delay.

So are the peers happy about this change considering there are quite a few
people against it (or who'd prefer alternate systems)?

p.s. I've left worrying about whether this should use the new tab/window
opening system to another bug.

Comment 30

[PikeUK]

Attachment: Patch: Web Search uses current engine v2 (updated again)

Comment 31

[Worcester12345]

(In reply to comment #27)
> I like the idea of this as a context menu. Maybe a few ideas can be borrowed
> from the extension, and let the extension handle the extras?

This was the extension I was referring to:
http://forums.mozillazine.org/viewtopic.php?t=75294

Comment 32

[Ben Basson]

Sas, read comment 14 and comment 15... a full incorporation of Context Search is
unlikely to happen, and I am quite happy to keep it open as an extension for the
forseeable future.

Also, I'd like to raise a point which I think could improve the intuitiveness of
this patch... Taken from bug 248173#7, I agree that it might be a good idea to
use the favicon supplied by the search plugin in the context menu.

Comment 33

[Steffen Wilberg]

Pike, you should've requested review back in July (and kept reminding mconnor).
Now the patch is bitrotten again. :(
We've only got around 2 weeks left until the localization freeze if we want to
do it before 1.0. Can you update it once more, please?

Comment 34

[PikeUK]

(In reply to comment #33)
> Pike, you should've requested review back in July (and kept reminding mconnor).
> Now the patch is bitrotten again. :(
> We've only got around 2 weeks left until the localization freeze if we want to
> do it before 1.0. Can you update it once more, please?

Not sure if it's too late, but will attach updated patch sometime later today or
tomorrow.

Comment 35

[PikeUK]

Attachment: Patch: Web Search uses current engine v2.1

Comment 36

[Zarco Zwier]

Why let it search the currently selected search engine? Why not add a cascaded
context menu with all installed search plugins? That would be of even more use.

Comment 37

[Worcester12345]

(In reply to comment #36)
> Why let it search the currently selected search engine? Why not add a cascaded
> context menu with all installed search plugins? That would be of even more use.

See comment 31.

Comment 38

[Zarco Zwier]

Oops, sorry for the spam ;)

Comment 39

[José Jeria]

*** Bug 287444 has been marked as a duplicate of this bug. ***

Comment 40

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

*** Bug 294681 has been marked as a duplicate of this bug. ***

Comment 41

[Mike Connor [:mconnor]]

pike, is this still working or does it need an update?  yes, I suck!

Comment 42

[Pharis]

Why not add an additional entry to the context menu of seleceted text. One would
be "Search Web for '<text>'", where <text> stands in for the text that is
currently selected. This could be linked to Google by default. This is what is
currently implemented in FF.
Underneath this in the context menu, there could be another entry, "Search ***
for '<text.'", where *** stands for the currently active search engine in the
search toolbar.  It is importnat to maintain a distinction between a general web
search, like a normal Google search, and the oftentimes more specialized
searches done in the search toolbar. I.e., many people don't use the search
toolbar for general web searches, but have like the Google toolbar or Yahoo
Companion extnesions where they do such searches, and leave the search toolbar
for more speciality searches.  The functionality of the context menu should
mimic this user behabior, having one entry to a general web search of selected
text, and another for a "speciality" search of selected text, by whatever search
engine is currently active in the search bar.

Comment 43

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

Comment on attachment 160177 [details] [diff] [review]
Patch: Web Search uses current engine v2.1

>Index: mozilla/browser/base/content/browser.js

>-function OpenSearch(tabName, searchStr, newTabFlag)

Nice cleanup here, this function has been bloated for a while now :).

>+  var currentEngine = searchBar ? searchBar.getAttribute("searchengine") : null;
...
>+  // If the search bar is available then use it
>+  if (currentEngine) {
>+    var iSearchService = Components.classes["@mozilla.org/rdf/datasource;1?name=internetsearch"]
>+                                   .getService(Components.interfaces.nsIInternetSearchService);
>+    openUrl = iSearchService.GetInternetSearchURL(currentEngine, escapedSearchStr, 0, 0, {value: 0});
>+  } else { // Else use default search engine

Why not always just use the browser.search.selectedEngine pref? Especially
since once bug 235204 is fixed, using the default when the searchbar is hidden
won't be right.

>+          var searchBar = document.getElementById("searchbar");
>+          var searchEngineName = searchBar ? searchBar.getAttribute("enginename") : null;
>+          if (!searchEngineName)
>+            searchEngineName = gNavigatorBundle.getString("defaultSearchName");
>+          var searchSelectText = gNavigatorBundle.getFormattedString("searchText", [searchEngineName, selectText]);

Same point about using currentlySelected.

>+        var selectStr = focusedWindow.__proto__.getSelection.call(focusedWindow);
>+        selectStr = selectStr.toString();

The __proto__ hack isn't necessary thanks to the new built in XPCNativeWrapper.

>Index: mozilla/browser/base/content/search.xml

Some of the changes here somewhat conflict with the patch in bug 235204.
Depending on which goes in first, changes will need to be made to one or the
other. If you'd like look over and comment on that patch, I'd appreciate the
feedback :).

Also, I'm guessing this no longers applies cleanly to the trunk, so a fresh
patch would probably be a good idea. Mike should be able to get to it a little
quicker now, so hopefully it doesn't sit around rotting as it has for a while
now.

Comment 44

[PikeUK]

(In reply to comment #43)
> Why not always just use the browser.search.selectedEngine pref? Especially
> since once bug 235204 is fixed, using the default when the searchbar is hidden
> won't be right.

AFAIK that pref didn't exist when the patch was attached.

> >+        var selectStr =
focusedWindow.__proto__.getSelection.call(focusedWindow);
> >+        selectStr = selectStr.toString();
> 
> The __proto__ hack isn't necessary thanks to the new built in XPCNativeWrapper.

That line is moved, not added by the patch so would be fixed in an updated version.

> Also, I'm guessing this no longers applies cleanly to the trunk, so a fresh
> patch would probably be a good idea. Mike should be able to get to it a little
> quicker now, so hopefully it doesn't sit around rotting as it has for a while
> now.

It sounds like it might be better if this were handled together with bug 235204,
please feel free to take over this bug, I'd rather leave it to someone who knows
what they're doing. :-)

Comment 45

[Mike Connor [:mconnor]]

not a blocker, but hopefully we'll sneak in some more search improvements before
beta

Comment 46

[PikeUK]

Comment on attachment 160177 [details] [diff] [review]
Patch: Web Search uses current engine v2.1

Obsoleting rotted patch.

Comment 47

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

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.522&mark=2775#2771

Indeed! ;)

Comment 48

[pal-moz]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060323 Firefox/1.6a1 ID:2006032305

now these are same.

Comment 49

[Steffen Wilberg]

This has been fixed by bug 317107.

Comment 50

[Tracy Walker [:tracy]]

Verified fixed in recent Bon echo builds