634364 - Restore option to open context menu web search in new window (regression after SM2.0)

Status: RESOLVED FIXED

(SeaMonkey :: Search, defect)

Description

[lemon_juice]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.16) Gecko/20101123 SeaMonkey/2.0.11
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b11) Gecko/20110209 Firefox/4.0b11 SeaMonkey/2.1b2

The beta builds of SM 2.1 no longer provide an option to open web search results in a new window when I select some text on a page and use context menu option "Search Google for...". Currently, the search results always open in a new tab, which can be inconvenient for some users. I think this option is important for usability - often I have the browser open with many tabs and adding yet another tab with search results creates too much clutter, I find it much better to always start search in a new window and then - open subsequent tabs in that new window.

In other words it's about bringing back the option "Browser -> Internet Search -> Search Results -> Open a tab instead of a window for a context menu web search" or providing an equivalent.

Reproducible: Always

Steps to Reproduce:
1. Select some text on a web page
2. Right click and choose "Search Google/... for..."
Actual Results:  
The new page with search results always opens in a new tab.

Expected Results:  
Need an option to open the page with search results in a new window (instead of a tab)

Comment 1

[Philip Chee]

<http://forums.mozillazine.org/viewtopic.php?f=6&t=2103623>
From therube:
> Should be Shift (|Ctrl|Alt|...) toggle-able too IMO.
> (I think Link Behavior is? Or at least kind of remember Benoit often mentioning something like that.)

Comment 2

[Philip Chee]

We used to do this in SeaMonkey 2.0:

OpenSearch('internet', gContextMenu.searchSelected(), true, event.shiftKey)
Where the function:
function OpenSearch(tabName, searchStr, newWindowOrTabFlag, reverseBackgroundPref)
would check browser.search.opentabforcontextsearch and browser.tabs.loadInBackground

Currently we do:
BrowserSearch.loadSearch(gContextMenu.searchSelected(), true);
Signature:
loadSearch: function BrowserSearch_search(searchText, useNewTab)

It would be a SMOP to bring back those two prefs.

Comment 3

[Philip Chee]

Attachment: Patch v1.0 Restore old preference.

Originally fixed in:
  Bug 133117 'Web Search for...' results should open in new tab rather than new window.
Regressed by:
  Bug 410613 SeaMonkey should support "OpenSearch".

> +// Open context search results in either a new window or tab.
> +pref("browser.search.opentabforcontextsearch", false);

Bring back the old pref.

> +      if (where == "window")
> +        return;

I don't think it makes sense to open the search sidebar in the current window if we are going to open search results in a new window.

> +      <checkbox id="openContextSearchTab"
> +                label="&openContextSearchTab.label;"
> +                accesskey="&openContextSearchTab.accesskey;"
> +                preference="browser.search.opentabforcontextsearch"/>

> +<!ENTITY openContextSearchTab.label       "Open a tab instead of a window for a context menu web search">
> +<!ENTITY openContextSearchTab.accesskey   "t">

Bring back the old strings removed by KaiRo.

> -function MsgBrowserSearch(aSearchStr)
> +function MsgOpenSearch(aSearchStr, aEvent)

I don't know why KaiRo changed the name of the function. Changing it back.

> +  MAKESAMETYPEPREFTRANSFORM("browser.search.opentabforcontextsearch",  Bool),

I hope I got this right. Compiles without any errors.

Comment 4

[[e-mail]]

(In reply to comment #3)
> I don't think it makes sense to open the search sidebar in the current window
> if we are going to open search results in a new window.
Actually given the that new search sidebar doesn't cache search results I'm not sure it makes sense to open it automatically at all.

Comment 5

[[e-mail]]

Comment on attachment 515830 [details] [diff] [review]
Patch v1.0 Restore old preference.

>+      let newTabPref = Services.prefs.getBoolPref("browser.search.opentabforcontextsearch");
>+      let where = (aEvent && aEvent.shiftKey) ^ newTabPref ? "tabfocused" : "window";
The old code operated more along the lines of
newTabPref ? aEvent && aEvent.shiftKey ? "tabshifted" : "tab" : "window"

Comment 6

[Philip Chee]

> [e-mail]      2011-03-01 01:12:30 PST
> 
>> I don't think it makes sense to open the search sidebar in the current window
>> if we are going to open search results in a new window.
> Actually given the that new search sidebar doesn't cache search results I'm not
> sure it makes sense to open it automatically at all.

Fortunately "browser.search.opensidebarsearchpanel" defaults to false. Or should I just remove this pref?

Comment 7

[Philip Chee]

> Fortunately "browser.search.opensidebarsearchpanel" defaults to false. Or
> should I just remove this pref?
Mnyromyr is the sidebar owner. Perhaps he has an opinion?

Comment 8

[[e-mail]]

(In reply to comment #3)
> > +  MAKESAMETYPEPREFTRANSFORM("browser.search.opentabforcontextsearch",  Bool),
> I hope I got this right. Compiles without any errors.
We actually copy all browser.search.* preferences automatically already.

Comment 9

[Karsten Düsterloh]

(In reply to comment #7)
> > Fortunately "browser.search.opensidebarsearchpanel" defaults to false. Or
> > should I just remove this pref?
> Mnyromyr is the sidebar owner. Perhaps he has an opinion?

I explicitly made KaiRo not kill this pref, so it'd be a bit strange if I'd let you kill now. ;-)

Comment 10

[Philip Chee]

Attachment: Patch v1.1 fix nits.

> [e-mail]      2011-03-01 01:12:30 PST
> 
> (In reply to comment #3)
>> I don't think it makes sense to open the search sidebar in the current window
>> if we are going to open search results in a new window.
> Actually given the that new search sidebar doesn't cache search results I'm not
> sure it makes sense to open it automatically at all.

Fortunately "browser.search.opensidebarsearchpanel" defaults to false. Or should I just remove this pref?

> Karsten Düsterloh      2011-03-03 00:58:41 PST
> 
> I explicitly made KaiRo not kill this pref, so it'd be a bit strange if I'd let
> you kill now. ;-)

OK. It lives!

>>+      let newTabPref = Services.prefs.getBoolPref("browser.search.opentabforcontextsearch");
>>+      let where = (aEvent && aEvent.shiftKey) ^ newTabPref ? "tabfocused" : "window";
> The old code operated more along the lines of
> newTabPref ? aEvent && aEvent.shiftKey ? "tabshifted" : "tab" : "window"

Hmm. OK. Fixed.

> [e-mail]      2011-03-02 01:51:42 PST
> 
> (In reply to comment #3)
>>> +  MAKESAMETYPEPREFTRANSFORM("browser.search.opentabforcontextsearch",  Bool),
>> I hope I got this right. Compiles without any errors.
> We actually copy all browser.search.* preferences automatically already.
Oops. Removed.

Comment 11

[Ian Neal]

Comment on attachment 516543 [details] [diff] [review]
Patch v1.1 fix nits.

As there is a lot of common code now, could we do something like:
If this part sits in a file common to both mail and browser
function OpenSearchWindowOrTab(aSearchText, aNewWindowOrTab, aEvent) {
>     var engine;
> 
>     // If the search bar is visible, use the current engine, otherwise, fall
>     // back to the default engine.
>     if (isElementVisible(this.searchBar) ||
>         isElementVisible(this.searchSidebar))
In mailnews this will always return false so giving the correct engine of defaultEngine.

>       engine = Services.search.currentEngine;
>     else
>       engine = Services.search.defaultEngine;
> 
>+    var submission = engine.getSubmission(aSearchText); // HTML response
> 
>     // getSubmission can return null if the engine doesn't have a URL
>     // with a text/html response type.  This is unlikely (since
>     // SearchService._addEngineToStore() should fail for such an engine),
>     // but let's be on the safe side.
>     if (!submission)
>       return false;
> 
>+    if (aNewWindowOrTab) {
>+      let newTabPref = Services.prefs.getBoolPref("browser.search.opentabforcontextsearch");
>+      let where = newTabPref ? aEvent && aEvent.shiftKey ? "tabshifted" : "tab" : "window";
>+      openUILinkIn(submission.uri.spec, where, null, submission.postData);
>+      if (where == "window")
>+        return false;
>     } else {
>       loadURI(submission.uri.spec, null, submission.postData, false);
>       window.content.focus();
>     }
return true;
}
then
>+  loadSearch: function BrowserSearch_search(aSearchText, aNewWindowOrTab, aEvent) {
if (!OpenSearchWindowOrTab(aSearchText, aNewWindowOrTab, aEvent))
  return;
etc.
Then 
     <menuitem id="context-searchselect"
+              oncommand="OpenSearchWindowOrTab(gContextMenu.searchSelected(), true, event);"/>
Not sure what happens if oncommand get a true/false being returned to it though...

and MsgOpenSearch could be got rid of completely.

Comment 12

[Ian Neal]

Comment on attachment 516543 [details] [diff] [review]
Patch v1.1 fix nits.

r- but might be worth checking with Neil before creating a new version.

Comment 13

[[e-mail]]

Comment on attachment 516543 [details] [diff] [review]
Patch v1.1 fix nits.

>+function MsgOpenSearch(aSearchStr, aEvent)
>+  var engine = Services.search.defaultEngine;
>+  var submission = engine.getSubmission(aSearchStr);
>+  if (!submission)
>+    return;
>+
>+  var newTabPref = Services.prefs.getBoolPref("browser.search.opentabforcontextsearch");
>+  var where = newTabPref ? aEvent && aEvent.shiftKey ? "tabshifted" : "tab" : "window";
>+  openUILinkIn(submission.uri.spec, where, null, submission.postData);
> }
I think MsgOpenSearch is so short (you could shorten it further by eliminating "engine") that trying to share code with BrowserSearch isn't worthwhile.

Comment 14

[Philip Chee]

Attachment: Patch v1.3 fix another nit. sr=Neil

> [e-mail]      2011-03-20 14:41:44 PDT
> 
> >+function MsgOpenSearch(aSearchStr, aEvent)
> >+  var engine = Services.search.defaultEngine;
> >+  var submission = engine.getSubmission(aSearchStr);
> >+  if (!submission)
> >+    return;
> >+
> >+  var newTabPref = Services.prefs.getBoolPref("browser.search.opentabforcontextsearch");
> >+  var where = newTabPref ? aEvent && aEvent.shiftKey ? "tabshifted" : "tab" : "window";
> >+  openUILinkIn(submission.uri.spec, where, null, submission.postData);
> > }
> I think MsgOpenSearch is so short (you could shorten it further by eliminating
> "engine") that trying to share code with BrowserSearch isn't worthwhile.
Fixed.

Comment 15

[Ian Neal]

Comment on attachment 520714 [details] [diff] [review]
Patch v1.3 fix another nit. sr=Neil

>+++ b/suite/browser/navigator.js

>+    if (aNewWindowOrTab) {
>+      let newTabPref = Services.prefs.getBoolPref("browser.search.opentabforcontextsearch");
>+      let where = newTabPref ? aEvent && aEvent.shiftKey ? "tabshifted" : "tab" : "window";
>+      openUILinkIn(submission.uri.spec, where, null, submission.postData);
>+      if (where == "window")
>+        return;

>+++ b/suite/mailnews/mailWindowOverlay.js

>+  var newTabPref = Services.prefs.getBoolPref("browser.search.opentabforcontextsearch");
>+  var where = newTabPref ? aEvent && aEvent.shiftKey ? "tabshifted" : "tab" : "window";
>+  openUILinkIn(submission.uri.spec, where, null, submission.postData);

Is it worth putting a short comment in both files referencing each other saying if you update one then remember to update the other (or something along those lines)?

If help needs updating then please either do here or spin off into another bug, thanks.

r=me with those addressed.

Comment 16

[Philip Chee]

> Is it worth putting a short comment in both files referencing each other saying
> if you update one then remember to update the other (or something along those
> lines)?
Comments added.

> If help needs updating then please either do here or spin off into another bug,
> thanks.
Fortunately, someone neglected to change the help text to reflect KaiRos changes so now that I've brought the old behaviour back the help text matches again.

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/ab010a4ab9f6