Closed Bug 133117 Opened 18 years ago Closed 14 years ago

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

Categories

(SeaMonkey :: UI Design, enhancement, P3)

enhancement

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: nagi, Unassigned)

References

Details

(Keywords: fixed-seamonkey1.1a, polish, Whiteboard: see comment #8 for patch)

Attachments

(3 files, 4 obsolete files)

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
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
*** Bug 142025 has been marked as a duplicate of this bug. ***
OS is set to Win98 but I see the same results with Linux
Severity: minor → enhancement
OS: Windows 98 → All
Hardware: PC → All
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
Assignee: jaggernaut → sgehani
Status: UNCONFIRMED → NEW
Component: Tabbed Browser → Search
Ever confirmed: true
QA Contact: sairuh → claudius
Priority: -- → P3
Target Milestone: --- → mozilla1.2alpha
*** Bug 154740 has been marked as a duplicate of this bug. ***
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
Summary: Search Google for should be open in a new Tab → 'Web Search for...' results should open in new tab rather than new window
*** Bug 130247 has been marked as a duplicate of this bug. ***
OHTUBO Junpei made the patch for this request.
http://bugzilla.mozilla.gr.jp/showattachment.cgi?attach_id=950
The patch works great for me (Moz 1.0 on W2K).  Thanks so much!
Will the patch of #8 come into the trunk? Would be nice.
*** Bug 132220 has been marked as a duplicate of this bug. ***
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.
<spam>
So, what are we waiting for? The patch has been out since July.
</spam>
bugzilla@schiraldi.org (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
The patch is linked to by comment #8.
Right, sorry I missed that...

sgehani, could you please have a look at the patch mentioned in comment 8 , thanks
Keywords: patch
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 drivers@mozilla.org
> +<!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.
How about just

``Open tabs instead of windows for context menu searchs''
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.
Keywords: polish, ui
Whiteboard: see comment #8 for patch
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 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.
Attachment #110875 - Flags: review?
Flags: blocking1.4a?
Attachment #110875 - Flags: review? → review?(sgehani)
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.
Attachment #110875 - Flags: review?(sgehani) → review?(shliang)
Flags: blocking1.4a? → blocking1.4a-
See also bug #212629.
It seems this was fixed in Firbird recently.
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.
Attachment #110875 - Flags: review?(shliang) → review?(cbiesinger)
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.
Attachment #110875 - Flags: review?(cbiesinger) → review?(neil.parkwaycc.co.uk)
*** Bug 273299 has been marked as a duplicate of this bug. ***
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.
Assignee: samir_bugzilla → guifeatures
Component: Search → XP Apps: GUI Features
Product: Core → Mozilla Application Suite
QA Contact: claudius
Target Milestone: mozilla1.2alpha → ---
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.
Here's a new patch that fixes the bitrot and moves the checkbox to the Internet Search category of the prefs window.
Attachment #231706 - Flags: review?(iann_bugzilla)
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.
Attachment #231706 - Flags: review?(iann_bugzilla) → review-
(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).
(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.
Attached patch Updated patch v2 (obsolete) — Splinter Review
(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."
Attachment #231706 - Attachment is obsolete: true
Attachment #232060 - Flags: review?(iann_bugzilla)
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!
Attachment #232060 - Flags: review?(iann_bugzilla) → review-
Attached patch Updated patch v2.1 (obsolete) — Splinter Review
(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.
Attachment #232060 - Attachment is obsolete: true
Attachment #232613 - Flags: review?(iann_bugzilla)
Attachment #232613 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 232613 [details] [diff] [review]
Updated patch v2.1

Thanks for the review.  Requesting sr.
Attachment #232613 - Flags: superreview?(neil)
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?
Attachment #232613 - Flags: superreview?(neil) → superreview+
Attached patch Updated patch v3Splinter Review
(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.
Attachment #232613 - Attachment is obsolete: true
Attachment #232888 - Flags: superreview?(neil)
Attachment #232888 - Flags: review+
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.
Attachment #232888 - Flags: superreview?(neil) → superreview+
(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?
Fix checked in.

Note: I patched both browser-prefs files and both sets of help too.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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.
Attachment #232888 - Flags: approval-seamonkey1.1a?
Attachment #232888 - Flags: approval-seamonkey1.1a?
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.
Attachment #233975 - Flags: approval-seamonkey1.1a?
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)
Attachment #233975 - Flags: approval-seamonkey1.1a? → approval-seamonkey1.1a+
landed on the branch
Status: RESOLVED → VERIFIED
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.
Attachment #110875 - Attachment is obsolete: true
Attachment #110875 - Flags: review?(neil)
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.