Closed Bug 634364 Opened 9 years ago Closed 9 years ago

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

Categories

(SeaMonkey :: Search, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: michal-ok, Assigned: philip.chee)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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)
Version: unspecified → Other Branch
<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.)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Version: Other Branch → Trunk
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.
Summary: Add option to open context menu web search in new window (regression after SM2.0) → Restore option to open context menu web search in new window (regression after SM2.0)
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.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #515830 - Flags: superreview?(neil)
Attachment #515830 - Flags: review?(iann_bugzilla)
(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 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"
> neil@parkwaycc.co.uk      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?
> Fortunately "browser.search.opensidebarsearchpanel" defaults to false. Or
> should I just remove this pref?
Mnyromyr is the sidebar owner. Perhaps he has an opinion?
(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.
(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. ;-)
Attached patch Patch v1.1 fix nits. (obsolete) — Splinter Review
> neil@parkwaycc.co.uk      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.

> neil@parkwaycc.co.uk      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.
Attachment #515830 - Attachment is obsolete: true
Attachment #516543 - Flags: superreview?(neil)
Attachment #516543 - Flags: review?(iann_bugzilla)
Attachment #515830 - Flags: superreview?(neil)
Attachment #515830 - Flags: review?(iann_bugzilla)
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 on attachment 516543 [details] [diff] [review]
Patch v1.1 fix nits.

r- but might be worth checking with Neil before creating a new version.
Attachment #516543 - Flags: review?(iann_bugzilla) → review-
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.
Attachment #516543 - Flags: superreview?(neil) → superreview+
> neil@parkwaycc.co.uk      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.
Attachment #516543 - Attachment is obsolete: true
Attachment #520714 - Flags: superreview+
Attachment #520714 - Flags: review?(iann_bugzilla)
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.
Attachment #520714 - Flags: review?(iann_bugzilla) → review+
> 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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.