133117 - 'Web Search for...' results should open in new tab rather than new window

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, enhancement, P3)

Description

[Nagi Peters]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.9+) Gecko/20020322
BuildID:    2002032203

Search Google for in the Contextmenu opens a new Window, i would be great if i
could choose to open it in a new tab.

Reproducible: Always
Steps to Reproduce:
1. Mark a text on a page
2. rigth click on the text
3. choose "search google for"


Actual Results:  New Window will be opened with the search page

Expected Results:  New tab will be opened with the search page

Comment 1

[Greg L.]

I agree. Once I decide that I want to keep all my pages in tabs I don't want the
search results to pop up in a new window.

Maybe the Edit>pref>Navigator>Tabbed browsing could have an additional checkbox
under "Open tabs instead of windows for:"

X Search results from context menu

Comment 2

[R.K.Aa.]

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

Comment 3

[Jeff Lightfoot]

OS is set to Win98 but I see the same results with Linux

Comment 4

[David Illsley]

Confirming 'cos it happens on my current CVS linux build and I can see how it
could be annoying and I can't see an obvious dupe. Also over to Search as the
component description suggests they might own the menu item so its probably them
who opens the window/tab, not the tabbrowser widget

Component --> Search
CONFIRM

Comment 5

[Alex Bishop]

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

Comment 6

[Alex Bishop]

Tweaking summary to aid searchability.

Old: Search Google for should be open in a new Tab
New: 'Web Search for...' results should open in new tab rather than new window

Comment 7

[Bill Mason]

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

Comment 8

[Koike Kazuhiko]

OHTUBO Junpei made the patch for this request.
http://bugzilla.mozilla.gr.jp/showattachment.cgi?attach_id=950

Comment 9

[Alex Halderman]

The patch works great for me (Moz 1.0 on W2K).  Thanks so much!

Comment 10

[Nagi Peters]

Will the patch of #8 come into the trunk? Would be nice.

Comment 11

[Erich 'Ricky' Iseli]

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

Comment 12

[S-O Westberg]

I would like to have to option of selecting a new tab when I do search with the 
search button. I do not open a new bug since I think they are all realated.

Comment 13

[Mike Schiraldi]

<spam>
So, what are we waiting for? The patch has been out since July.
</spam>

Comment 14

[Erich 'Ricky' Iseli]

[e-mail] (Mike Schiraldi), please can you tell me where I can
found this patch? I see nothing attached to this bug, and this bug is not marked
relating to any other, susceptible to contain a patch. Also, all the duplicated
bugs don't contain any patch. So obviously, the first thing to do is attach this
patch here and let it go through review, super-review then let's get this in
after the tree opens again for 1.2

Comment 15

[Mike Schiraldi]

The patch is linked to by comment #8.

Comment 16

[Erich 'Ricky' Iseli]

Right, sorry I missed that...

sgehani, could you please have a look at the patch mentioned in comment 8 , thanks

Comment 17

[Erich 'Ricky' Iseli]

And of course, we will have to wait until the tree opens for 1.3alpha (not 1.2
as stated above) OR we will have to get approval from [e-mail]

Comment 18

[Samir Gehani]

> +<!ENTITY webSearch.label "Open web search result">

We need to fix this verbiage:
``Open tabs instead of windows for Open web search results'' should read
``Open tabs instead of windows for context menu web search results''

However, I'm not sure that ``context menu web search results'' is comprehensible
phrasing.  CC'ing Jatin for wording help.

Comment 19

[Karl Palsson]

How about just

``Open tabs instead of windows for context menu searchs''

Comment 20

[Vidar Haarr (not reading bugmail)]

The patch works fine here.
I'll attach the patch to this bug, so that we can request a review on it.
IMO, this bug qualifies for the 'polish' keyword. If you disagree, please remove it.

Comment 21

[Vidar Haarr (not reading bugmail)]

Attachment: The same patch as on b.m.gr.jp, with the wording changed.

This is the same patch as the
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=950&action=view, only the
wording on 'webSearch.label' has been changed to 'Context menu web-search
results'.

Comment 22

[Vidar Haarr (not reading bugmail)]

Comment on attachment 110875 [details] [diff] [review]
The same patch as on b.m.gr.jp, with the wording changed.

Requesting review. It just struck me that the pref-wording
('b.t.opentabfor.webSearch') should probably change as well.. Thats up to the
reviewers.

Comment 23

[Samir Gehani]

Comment on attachment 110875 [details] [diff] [review]
The same patch as on b.m.gr.jp, with the wording changed.

Shuehan now works on search so chaging review requestee to be her.

Comment 24

[ralph]

See also bug #212629.

Comment 25

[Phillip Stewart :P]

It seems this was fixed in Firbird recently.

Comment 26

[Prognathous]

By default, Search should not open a new window nor tab - this is just
inconsistent and unexpected. 

The search page should open in the same window, unless the user holds the Ctrl
button pressed, in which case it should follow
browser.tabs.opentabfor.middleclick - if this value is True -> open in tab, if
False -> open in new window.

Prog.

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 110875 [details] [diff] [review]
The same patch as on b.m.gr.jp, with the wording changed.

+	     if (!pref.getBoolPref("browser.tabs.opentabfor.webSerch")){

there's a typo here... this should've caused the call to throw an exception,
how did you test the patch?

anyway... an xpfe peer needs to review this, I'm not one.

Comment 28

[Frank Wein [:mcsmurf]]

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

Comment 29

[Chris Campbell]

I apologize for the comment spam, but this seems to have been done, more or
less, about a year ago, but not quite done?  Can someone give this a shove?  I
would loooove to be able to open context menu searches in new tabs.

Comment 30

[Krang]

Attachment: Tabbed Browsing prefs with additional option added

The patch for this bug has been somewhat bitrotted.  However, when updating it, I noticed that the new option will no longer fit (at least on Windows) because additional options have been added since the patch was originally written (see attached screenshot).  I think it would be overkill to propose resizing the entire prefs window over this, so maybe the option could be reworded and moved to the Internet Search category.

Comment 31

[Krang]

Attachment: Updated patch with moved checkbox

Here's a new patch that fixes the bitrot and moves the checkbox to the Internet Search category of the prefs window.

Comment 32

[Ian Neal]

Comment on attachment 231706 [details] [diff] [review]
Updated patch with moved checkbox

r=- for several reasons
1) patch is bitrotted, most pref stuff is now under /suite
2) "T" is not for the accesskey as there is no upppercase "T" instead use "n" perhaps?
3) You need to patch the help file too so this new pref is explained. In the prefs section, also I cannot see a section about context menu web searches, perhaps something needs adding to the search the web section (or at least a bug raised if it does not already exist)
4) We should also making sure this pref pane is the best place for this pref and not just because there is no space in where it should be.

Comment 33

[Krang]

(In reply to comment #32)
> (From update of attachment 231706 [details] [diff] [review] [edit])
> r=- for several reasons
> 1) patch is bitrotted, most pref stuff is now under /suite

Does that also apply to the branch?  The patch was made against the branch, but I could make a separate one for the trunk as well if the prefs are in a different location.

> 2) "T" is not for the accesskey as there is no upppercase "T" instead use "n"
> perhaps?

Oops, I'll fix that in the next patch.

> 3) You need to patch the help file too so this new pref is explained.

Sure, I'll add that in the next patch as well.

> In the
> prefs section, also I cannot see a section about context menu web searches,
> perhaps something needs adding to the search the web section (or at least a bug
> raised if it does not already exist)

There is a quick mention of it under the "Searching on Selected Words in a Web Page" heading under "Fast Searches."

> 4) We should also making sure this pref pane is the best place for this pref
> and not just because there is no space in where it should be.

I can see it working both ways.  It's appropriate for the Tabbed Browsing section because it deals with tab-related preferences, but it's also appropriate for the Internet Search section because it is directly related to the options there (particularly the default search engine).

Comment 34

[Ian Neal]

(In reply to comment #33)
> (In reply to comment #32)
> > (From update of attachment 231706 [details] [diff] [review] [edit] [edit])
> > r=- for several reasons
> > 1) patch is bitrotted, most pref stuff is now under /suite
> 
> Does that also apply to the branch?  The patch was made against the branch, but
> I could make a separate one for the trunk as well if the prefs are in a
> different location.
Most patches go on trunk first then onto branch once they have baked a while.

Comment 35

[Krang]

Attachment: Updated patch v2

(In reply to comment #34)
> Most patches go on trunk first then onto branch once they have baked a while.

Sorry about that, this patch is for the trunk.  I ended up using "t" for the accesskey since "n" underlines the "n" in "Open" instead of "new."

Comment 36

[Ian Neal]

Comment on attachment 232060 [details] [diff] [review]
Updated patch v2

>--- suite/locales/en-US/chrome/common/pref/pref-search.dtd.bak	2006-08-03 20:17:25.173731200 -0500
>+++ suite/locales/en-US/chrome/common/pref/pref-search.dtd	2006-08-03 20:23:15.978163200 -0500
>@@ -10,6 +10,9 @@
> <!ENTITY openSidebarSearchPanel.label     "Open the Search tab in the Sidebar when a search is invoked">
> <!ENTITY openSidebarSearchPanel.accesskey "O">
> 
>+<!ENTITY openContextSearchTab.label       "Open browser tabs instead of windows for context menu Web searches">
"browser" is probably surplus to requirements and typically it would be singular so "Open tab instead of window for a context menu Web search". I'm still debating whether it should be "Web" or "web"
The above changes also need to be reflected in the help pages diffs.

Almost there!

Comment 37

[Krang]

Attachment: Updated patch v2.1

(In reply to comment #36)
> "browser" is probably surplus to requirements and typically it would be
> singular so "Open tab instead of window for a context menu Web search".

Fixed in the latest patch.

> I'm
> still debating whether it should be "Web" or "web"

Looking through the different pref panes, it seems that "web" is used more than "Web," so I went ahead and made that change.

> The above changes also need to be reflected in the help pages diffs.
> 
> Almost there!

This has also been fixed.  I also cleared up the wording a little on cs_nav_prefs_navigator.xhtml.

Comment 38

[Krang]

Comment on attachment 232613 [details] [diff] [review]
Updated patch v2.1

Thanks for the review.  Requesting sr.

Comment 39

[[e-mail]]

Comment on attachment 232613 [details] [diff] [review]
Updated patch v2.1

>-        if (!newWindowFlag)
>+        if (!newWindowOrTabFlag) {
>           loadURI(defaultSearchURL);
>-        else
>-          window.open(defaultSearchURL, "_blank");
>+        } else {
>+          if (!pref.getBoolPref("browser.search.opentabforcontextsearch")) {
>+            window.open(defaultSearchURL, "_blank");
>+          } else {
>+            var newTab = getBrowser().addTab(defaultSearchURL);
>+            if (!pref.getBoolPref("browser.tabs.loadInBackground"))
>+              getBrowser().selectedTab = newTab;
>+          }
>+        }
You don't need all these extra {}s. sr=me with that fixed, something like:
if (!newWindowOrTabFlag)
  loadURI(defaultSearchURL);
else if (!pref.getBoolPref("browser.search.opentabforcontextsearch"))
  window.open(defaultSearchURL, "_blank");
else {
  var newTab = gBrowser.addTab(defaultSearchURL);
  if (!pref.getBoolPref("browser.tabs.loadInBackground"))
    gBrowser.selectedTab = newTab;
}
Nit: Ideally we'd respect the shift key to toggle the pref. Followup patch?

Comment 40

[Krang]

Attachment: Updated patch v3

(In reply to comment #39)
> You don't need all these extra {}s. sr=me with that fixed, something like:
> if (!newWindowOrTabFlag)
>   loadURI(defaultSearchURL);
> else if (!pref.getBoolPref("browser.search.opentabforcontextsearch"))
>   window.open(defaultSearchURL, "_blank");
> else {
>   var newTab = gBrowser.addTab(defaultSearchURL);
>   if (!pref.getBoolPref("browser.tabs.loadInBackground"))
>     gBrowser.selectedTab = newTab;
> }

Fixed in the newest patch.

> Nit: Ideally we'd respect the shift key to toggle the pref. Followup patch?

I added detection of the shift key to this patch.  Re-requesting sr due to the new code, but carrying over Ian's r+ since none of the prefs code was modified.

Comment 41

[[e-mail]]

Comment on attachment 232888 [details] [diff] [review]
Updated patch v3

>+<!ENTITY openContextSearchTab.label       "Open tab instead of window for a context menu web search">
>+<!ENTITY openContextSearchTab.accesskey   "t">
Sorry I didn't notice earlier, but I think this should say
"Open a tab instead of a window for a context menu web search"
(i.e. with "a tab" and "a window"). sr=me with that fixed.

Comment 42

[Krang]

(In reply to comment #41)
> (From update of attachment 232888 [details] [diff] [review] [edit])
> >+<!ENTITY openContextSearchTab.label       "Open tab instead of window for a context menu web search">
> >+<!ENTITY openContextSearchTab.accesskey   "t">
> Sorry I didn't notice earlier, but I think this should say
> "Open a tab instead of a window for a context menu web search"
> (i.e. with "a tab" and "a window"). sr=me with that fixed.

Oops, you're right, that does sound better.  With that change in mind, could this be checked in on the trunk?

Comment 43

[[e-mail]]

Fix checked in.

Note: I patched both browser-prefs files and both sets of help too.

Comment 44

[Krang]

Comment on attachment 232888 [details] [diff] [review]
Updated patch v3

(In reply to comment #43)
> Fix checked in.
> 
> Note: I patched both browser-prefs files and both sets of help too.

Thanks for the checkins.  Requesting branch approval.

Comment 45

[Krang]

Attachment: Updated patch v3 for branch

Here's a new patch for my branch approval request.  This is the same as patch v3 for the trunk, but the paths now reflect the correct file locations for the branch, and the change in comment #41 has been applied.

Comment 46

[Robert Kaiser]

Comment on attachment 233975 [details] [diff] [review]
Updated patch v3 for branch

a=me for 1.1a (as long as it gets checked in before the freeze)

Comment 47

[Andrew Schultz]

landed on the branch

Comment 48

[Krang]

Comment on attachment 110875 [details] [diff] [review]
The same patch as on b.m.gr.jp, with the wording changed.

Clearing old review request.  Sorry for the bugspam.