336925 - Middle Click Search Button should open new tab

Status: RESOLVED FIXED

(Firefox :: Search, enhancement)

Description

[Melinda McBeath]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060506 BonEcho/2.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060506 BonEcho/2.0a1

The new search button should open a new tab when middle clicked, like other toolbar buttons. 

currently it only selects the text when middle click, like left clicking the favicon.

Reproducible: Always

Comment 1

[Adam Kowalczyk]

Most probably, that's how it should work. Confirming.

Comment 2

[Melinda McBeath]

This could be implemented the same way the main navigation toolbuttons (back forward)

line 1082 of browser.xul has the definition of back button 
line 1086 has the attribute - onclick="checkForMiddleClick(this, event);"

this function is defined in utilityOverlay.js on line 207

i don't have a great understanding of xbl. but search.xml doesn't define a oncommand attribute for the search-go-button. So the function above wouldn't work.

requesting blocking firefox 2.0

Comment 3

[Mike Connor [:mconnor]]

This would be pretty useful, and trivial enough to implement that we should just do it.  (There's a bug somewhere on the Go button not doing this, if someone can find/file that I'll get that on radar too, since we'll be mucking with Go as well)

Comment 4

[Peter]

I am voting for this bug.

Comment 5

[pal-moz]

[about:config]
set "browser.search.openintab" to true

you can open search results in a new tab by click Enter/search button.

Comment 6

[Pam Greene]

(In reply to comment #5)

Yes, or by alt-clicking without setting that option.  Adding a middle-click still seems worthwhile, though.

Comment 7

[Melinda McBeath]

(In reply to comment #5)
> [about:config]
> set "browser.search.openintab" to true
> 
> you can open search results in a new tab by click Enter/search button.
> 

this bug is about consistency in the ui of firefox, not whether there is a setting to change the behavior.

control-click
 - the back button
 - the home button
 - an item on your personal toolbar

what happens? 

now control-click the search button. What happens?

alt-click
 - the back button
 - the home button
 - an item on your personal toolbar

what happens?

now alt-click the search button. What happens?

middle-click
 - the back button
 - the home button
 - an item on your personal toolbar

what happens?

now middle-click on the search button. What happens?

I feel like I'm watching sesame street, which one of these doesn't belong?

Comment 8

[Pam Greene]

(In reply to comment #3)
>(There's a bug somewhere on the Go button not doing this, if
> someone can find/file that I'll get that on radar too, since we'll be mucking
> with Go as well)

Bug 279687

Comment 9

[Pam Greene]

Attachment: Middle-clicking Search opens results in new tab

In addition, the browser.search.openintab pref still makes searches default to opening in a new tab, and alt-clicking still reverses the sense of that pref for a particular click.  But all of that is overridden for a middle-click.

Comment 10

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

Comment on attachment 222066 [details] [diff] [review]
Middle-clicking Search opens results in new tab

>Index: components/search/content/search.xml

>-          var newTab = textBox._prefBranch.getBoolPref("browser.search.openintab");
>-          this.doSearch(textValue, ((aEvent && aEvent.altKey) ^ newTab));
>+          // Always open in a new tab on a middle-click; otherwise examine the
>+          // preference and the alt key.
>+          var newTab = (aEvent && (aEvent.button == 1));
>+          if (! newTab) {
>+            var newTabPref = textBox._prefBranch.getBoolPref("browser.search.openintab");
>+            newTab = ((aEvent && aEvent.altKey) ^ newTabPref);
>+          }

I think we should remove the aEvent null checks. I added it in bug 306358 because extensions were calling onTextEntered without an event param, but I don't think it's a good idea to continue supporting that, especially since the method has been renamed and is "private". People who want to piggy back on the search bar code like that can just call searchBar.doSearch(searchBar.value, useNewTab). nit: no space after "!".

Comment 11

[Pam Greene]

Attachment: Patch addressing Gavin's comments

(In reply to comment #10)

> I think we should remove the aEvent null checks.

> nit: no space after "!".

Done.

Comment 12

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

Comment on attachment 222504 [details] [diff] [review]
Patch addressing Gavin's comments

r=me, but please do get review from a browser peer.

Comment 13

[Pam Greene]

Attachment: Fresh patch, same code

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 223863 [details] [diff] [review]
Fresh patch, same code

sr=[e-mail]

Comment 15

[Pam Greene]

Checked in on trunk and 1.8 branch.