345031 - History>Home should be middle-clickable

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Michael Ventnor]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060714 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060714 Minefield/3.0a1

While the Home button in the toolbar graciously enjoys its ability to handle middle-clicks, the option in History>Home is lacking. History>Back and History>Forward can be middle clicked so for consistency History>Home should be too. I have made a patch and its coming up.

Reproducible: Always

Comment 1

[Michael Ventnor]

Attachment: Patch

Lets History>Home correctly handle middle clicks.

Comment 2

[Asaf Romano (gone)]

Comment on attachment 229647 [details] [diff] [review]
Patch

Again, get ui-review (from beltzner or mconnor) on this kind of changes before requesting review. Thanks.

Comment 3

[Michael Ventnor]

Sorry, I submitted this patch before you told me about the UI review thing. :)

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 229647 [details] [diff] [review]
Patch

I didn't apply the patch, but from context I'm assuming that it does the same thing on middle click that Next/Back in the History menu (ie: append tab at end with focus)

Comment 5

[Michael Ventnor]

Comment on attachment 229647 [details] [diff] [review]
Patch

It calls the exact same function as the graphical home button on the toolbar. So yes, it does append the tab at the end with focus.

Comment 6

[Asaf Romano (gone)]

Comment on attachment 229647 [details] [diff] [review]
Patch

>Index: browser/base/content/browser-menubar.inc
>===================================================================
\
>                 <menuitem label="&historyHomeCmd.label;"
>-                          command="Browser:Home"
>-                          key="historyHome"/>
>+                          oncommand="BrowserHomeClick(event);"
>+                          onclick="checkForMiddleClick(this, event);"
>+                          key="goHome"/>
>                 <menuseparator/>

BrowserHomeClick is expecting a mouse event, see the way we handle this for the home button.

Also, make sure you're not regressing pressing enter on the menuitem.

Comment 7

[Michael Ventnor]

As a matter of fact, the keyboard works great with the menu item. It isn't broken so I don't see how I should fix it.

Comment 8

[(no longer active)]

Attachment: Unbitrotted patch

This patch is an unbitrotted version of attachment 229647 [details] [diff] [review].  I've tested this with every possible use case, and it doesn't break any existing functionality, and it delivers the middle-click handling to the Home menu item (just like Back and Forward).  Re-requesting mano's take on this.

Comment 9

[Asaf Romano (gone)]

Comment on attachment 295641 [details] [diff] [review]
Unbitrotted patch

hrm, yeah, sorry. I would bother renaming browserhomeclick to something more generic (and make it support the event==null case; and probably check "button" in event), r=mano either way.

Comment 10

[Asaf Romano (gone)]

Btw, I'm sure 

Comment 11

[Asaf Romano (gone)]

...that Michael could update this patch himself.

Comment 12

[(no longer active)]

Attachment: Patch with mano's comment addressed

Addressed mano's comments in comment 9:
 * Renamed BrowserHomeClick to BrowserGoHome.
 * Modified BrowserGoHome to check the aEvent parameter as mano suggested.

Seeking approval for checkin...

Comment 13

[Asaf Romano (gone)]

mozilla/browser/base/content/browser-menubar.inc 1.125
mozilla/browser/base/content/browser.js 1.922
mozilla/browser/base/content/browser.xul 1.402

Comment 14

[tabmix.onemen]

I think this is bug ...

When you Shift+Click home button or History>Home new window open but home pasg(s) not open in the new window if browser.startup.page is not 1.
i expect home page(s) to open when i click the home button 


if browser.startup.page is 0 or 3 new window open with blank tab
if browser.startup.page is 2 new window is open with the last open tab
if browser.startup.page is 1 new window is open with hope page(s)