219532 - Add pref to make search bar results always open in a new tab

Status: RESOLVED FIXED

(Firefox :: Search, enhancement)

Description

[thesh_bugs]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4) Gecko/20030624

When searching in the sidebar, I don't really like that the search results open
in both the sidebar and the main browser window. I would prefer that it opens up
in the sidebar only; could you add an option to search only in the sidebar?
Also, can you add an option to open the search engine in a new tab when using
the search button on the navigation bar, and then have the option for that to
override the "load links in background" option for tabbed browsing?

-Scott

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Igor Sipka]

This bug report is actually two different issues,

I would like to support the second issue of opening the search bar results in a
new tab by default. This keeps the current page you're browsing and uses the
tabs properly.

Comment 2

[thesh_bugs]

Yeah, your right - this really should only be one bug. I don't really care about
the searching only in the sidebar part (I have gotten used to it showing the
results in the window). I'll change the description so this is _only_ for the
opening new tabs for a search part - I would like to see it for the sidebar as
well, though.

Comment 3

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

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

Comment 4

[Kelley Cook]

Please, note that pressing ALT-ENTER in the search toolbox does in fact open up
the results in a new tab, but I think that should be configurable option.

I personally would like to switch it so ENTER opens up the new tab, while
ALT-ENTER puts it in the current window.

Comment 5

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

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

Comment 6

[Christopher Butcher]

Attachment: Search bar results always open in new tab

Comment 7

[Christopher Butcher]

Attachment: Patch revision 2

Comment 8

[Christopher Butcher]

Attachment: Patch revision 3

I think I got it this time!

Search bar results open in a new tab, unless the currently viewed tab was just
created, and is blank.

Comment 9

[thesh_bugs]

Out of curiosity, why has this been changed to firefox instead of for the core
or suite (which is what it was intended for)?

Comment 10

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

(In reply to comment #9)
> Out of curiosity, why has this been changed to firefox instead of for the core
> or suite (which is what it was intended for)?

Oops. Note that the patches that were submitted by Chris are Firefox-specific.

(In reply to comment #8)
> Created an attachment (id=169345) [edit]
> Patch revision 3

Chris, you should not be setting that pref to true by default. Adding a pref is
one thing, changing default behavior is another.

Comment 11

[Christopher Butcher]

> Chris, you should not be setting that pref to true by default. Adding a pref is
> one thing, changing default behavior is another.

No problem, I will change that.
This is my first code contribution to Mozilla, and I'm enjoying it.

I'll revise it right away. :D

Comment 12

[Christopher Butcher]

Attachment: Patch revision 4

Default pref value made false.

Comment 13

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

Comment on attachment 169381 [details] [diff] [review]
Patch revision 4

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

>   if (gBrowser.localName == "tabbrowser" &&
>       aTriggeringEvent && 'altKey' in aTriggeringEvent &&
>-      aTriggeringEvent.altKey) {
>+      aTriggeringEvent.altKey || gPrefService.getBoolPref("browser.tabs.opentabfor.searchbar") && getWebNavigation().currentURI.spec != "about:blank") {

This needs proper indentation/wrapping. All files should wrap at 80, although
browser.js is kinda bad at following that rule. Also, you can use the
getBoolPref helper function defined at
http://lxr.mozilla.org/mozilla/source/browser/base/content/utilityOverlay.js#10
6 (also not commonly used, but handles failure better).

Comment 14

[Christopher Butcher]

Attachment: Patch Revision 5

Thanks Gavin for your continued help with this patch.
I enjoy using Firefox, and want to contribute in any way that I can, I'd like
to see this preference changes go into Firefox for those people that want it.

Comment 15

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

Comment on attachment 186280 [details] [diff] [review]
Patch Revision 5

Changing to a proper review target.

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 186280 [details] [diff] [review]
Patch Revision 5

I don't think this is the right prefname, the opentabfor prefs are open tab
(instead of window) for prefs, so logically flipping this pref would make
alt-enter open the search in a new window.

I think this should be a browser.search.* pref, possibly sharing the pref for
the search dialog, but that could be confusing.

Comment 17

[Christopher Butcher]

Attachment: Patch Revision 6

Comment 18

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

Trunk:
mozilla/browser/app/profile/firefox.js; new revision: 1.80;
mozilla/browser/base/content/search.xml; new revision: 1.31;

Comment 19

[Christopher Butcher]

Attachment: Patch Revision 7

Further enhancement:
In a case where the current tab is blank, make use of it, and load the search
results there.

Comment 20

[Christopher Butcher]

Attachment: Checking for about:config was unnecessary

Comment 21

[Mike Connor [:mconnor]]

Comment on attachment 196487 [details] [diff] [review]
Checking for about:config was unnecessary

>           var openInTab = this.prefService.getBoolPref("browser.search.openintab");
>+          var url = getBrowser().currentURI.spec;
>+          // if the current tab is blank, load the search results into it
>+          if (url == "about:blank")
>+            openInTab = false;
>           SearchLoadURL(searchURL, (evt && evt.altKey || openInTab));

a) we only need this code block if openInTab is true
b) the more useful case is if the last tab is about:blank, which is covered by
the generic bug for making addTab reuse an existing blank tab instead of
appending

Comment 22

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

Comment on attachment 196396 [details] [diff] [review]
Patch Revision 6

315034 already has approval, it depends on this landing.

Comment 23

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

Checked in on the 1.8 branch.
mozilla/browser/base/content/search.xml; new revision: 1.25.2.9;
mozilla/browser/app/profile/firefox.js; new revision: 1.71.2.17;