313718 - middle clicking bookmark menu opens link in active tab

Status: RESOLVED INCOMPLETE

(Core :: Widget: Cocoa, defect, P3)

Description

[David Kelly]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1

Middle clicking on a bookmark from the bookmarks menu opens the url in the active tab. This should open in a new tab, as it does if you middle click a bookmark in the bookmarks toolbar.

Reproducible: Always

Steps to Reproduce:
1. middle click on any bookmark in the bookmarks menu

Comment 1

[Vincent Lefevre]

I noticed the same problem under Mac OS X. I'm using Tab Mix Plus (and haven't tried without it since I thought this was a TMP feature only).

Also, the Bookmarks toolbar item was removed from the toolbars (this leads to a bug under Linux, see bug 317932, and I don't know if this is the cause here). Again, I haven't tried yet with unchanged toolbars and I don't have my Mac OS X machine here.

Comment 2

[Wayne Mery (:wsmwk)]

does this happen for you in FF 1.5?
this is WFM in FF 1.5 (on windows anyways, which I think was also broke pre-FF 1.5).

Comment 3

[David Kelly]

Bug still exists for FF 1.5.0.7 on OSX

Comment 4

[David Kelly]

Bug continues in version 2.0 for OSX

Comment 5

[Vincent Lefevre]

This bug is still there in Firefox 1.5.0.8 under OS X (with and without Tab Mix Plus disabled).

Comment 7

[Oakwine]

This bug still exists in the current Mac OS X nightlies. With the change to Open in Tabs behavior in Bug 175124, you no longer have the ability to preserve the frontmost tab when selecting "Open All in Tabs".  (You should be able to middle-click to preserve the frontmost tab).

Comment 8

[David Kelly]

Bug still present in Firefox 3.0 beta 2.

Comment 11

[Justin Gregory]

Attachment: Implements middle-click new tab in bookmarks and history menu for OS X

Uses [NSApp currentEvent] to find out if the event was a middle-click or command-left-click.  According to
http://developer.apple.com/DOCUMENTATION/Cocoa/Reference/ApplicationKit/Classes/NSApplication_Class/Reference/Reference.html#//apple_ref/doc/uid/20000012-CACCICEE
this should work for any OS X since 10.0.  It works for me with 10.5.

I have no idea where or what the code is that eventually receives the nsMouseEvent (previously nsXULCommandEvent).  For some reason, the attached combination of nsMouseEvent and NS_XUL_COMMAND worked, and using nsMouseEvent allowed me to set the button, as well.

Comment 13

[Josh Aas]

Comment on attachment 362189 [details] [diff] [review]
Implements middle-click new tab in bookmarks and history menu for OS X

This will send an nsMouseEvent for all menu item selections, which is wrong. You can select and item via keyboard and hit enter, for example. I think we need to stick with command events, at least for most things, and where we don't I'll need an explanation with the patch.

Also, the code formatting here doesn't match the rest of the file. For example, "if" statement block braces don't get indented on a new line.

Thanks for looking into this!

Comment 14

[Justin Gregory]

Yes, you are correct.  I was unaware that you could use the keyboard (Ctrl+F2) to get to the menu bar in OS X.  I will post a new patch soon.

Comment 15

[Justin Gregory]

Attachment: Provides middle-click and modifier key support to menu bar items in OS X

Here is a (hopefully) better version.  It checks for a middle click and for modifier keys on the user's selection of this menu item.  It only uses nsMouseEvent in the middle click case, as that type is required to include mouse button information.  Otherwise, it uses nsXULCommandEvent.  The modifier key inclusion means that exotic combinations like shift-middle-click result in the correct behavior, and things like command-Enter (if the user is navigating the menu with the keyboard) will work as well.

Also, code formatting has been cleaned up to better conform to guidelines and context.  This is the first  time I've contributed to an open-source project, so I'm a bit unfamiliar with the protocols.  I added my name to the contributors list in the license header, but if that is not appropriate, please feel free to remove it.

Comment 16

[Justin Gregory]

Attachment: Keyboard modifier and limited mouse support for menu-bar bookmark items in OS X

I apologize if anyone tried to build the patch I submitted in comment 15.  I was building against the Leopard 10.5 SDK, not Tiger 10.4.  The patch works beautifully against Leopard, but the build fails when built against Tiger.  The build failure is trivial to fix, but the patch no longer works anyways, since Tiger's [NSApp currentEvent] is returning something very different from Leopard's.

Attached is a new patch.  I have managed to regain most of the functionality by using a few Carbon functions.  Keyboard modifiers (CMD-click, etc.) work as expected.  However, I cannot find a way to detect middle clicks.  What can be detected, though, are mouse buttons being pressed in addition to the button that activates the menu item.  So, for instance, if you press both the left and right mouse buttons at the same time on a menu item, this patch will treat it as a middle click.  This is not totally counter-intuitive, as many systems emulate middle-clicking on two-button mice this way.

If support for OS X 10.4 is ever dropped, it would probably be best to switch over to [NSApp currentEvent], which would allow for proper middle-click support.  It is also possible that I've missed some other way to detect the mouse state in 10.4, which someone with more OS X experience would be able to find.

Comment 17

[Josh Aas]

Comment on attachment 366019 [details] [diff] [review]
Keyboard modifier and limited mouse support for menu-bar bookmark items in OS X

>+  // We cannot use Cocoa's [NSApp currentEvent] because when linked against
>+  // OS X 10.4, this gives us a useless NSSystemDefined event no matter

What does this have to do with linking? Isn't it the case that the returned event is just not as helpful on 10.4? The comment here implies that your compile time choices matter, I think you mean that when running on 10.4 you get back an event from this API that is not helpful here.

>+  UInt32 modFlags = GetCurrentEventKeyModifiers();

Any time you call a C function provided by Mac OS X frameworks please prefix it with "::", so "::GetCurrentEventKeyModifiers", same goes for the other OS framework C function(s) you use in this patch.

>+  if (GetCurrentEventButtonState() != 0) {
>+    // This was a multi-button-click, so nsMouseEvent must be used to specify
>+    // the "mouse button".  (We assume intent is middle-click).
>+    nsMouseEvent mouseEvent(PR_TRUE, NS_XUL_COMMAND,
>+                            nsnull, nsMouseEvent::eReal);
>+    mouseEvent.button = nsMouseEvent::eMiddleButton;

Here is the return value documentation for GetCurrentEventButtonState():

*  Result:
*    The queue-synchronized state of the mouse buttons. Bit zero
*    indicates the state of the primary button, bit one the state of
*    the secondary button, and so on.

Given this, why assume that "!=0" means the middle button? Wouldn't a simple right click result in a decimal value of 2, which you'd interpret as a middle click here? I have not tried this, but I'd guess that a middle button click would give you a decimal value of 4 (binary 100, the third mouse button) or 3 (binary 011, the combination of first and second buttons).

Comment 18

[Justin Gregory]

(In reply to comment #17)
> What does this have to do with linking? Isn't it the case that the returned
> event is just not as helpful on 10.4? The comment here implies that your
> compile time choices matter, I think you mean that when running on 10.4 you
> get back an event from this API that is not helpful here.
 
Here's what I mean: I have been building on a 10.5 machine.  If I link against the 10.5 SDK, [NSApp currentEvent] returns a mouse-up or keyboard event, depending on how the user selects the menu bar item.  In addition, [currentEvent modifierFlags] contains information about which keys may have been held down at the time.  If I link against the 10.4 SDK, [NSApp currentEvent] always returns an NSSystemDefined event, regardless of how the menu item was selected. [currentEvent modifierFlags] is always 0x100, regardless of the actual keyboard state when the menu item was selected.  In both cases I am running the browser on OS X 10.5.  So yeah, I guess I am saying that the compile time choices matter, at least for my system.

> Any time you call a C function provided by Mac OS X frameworks please prefix
> it with "::", so "::GetCurrentEventKeyModifiers", same goes for the other OS
> framework C function(s) you use in this patch.

Will do!

> Wouldn't a simple
> right click result in a decimal value of 2, which you'd interpret as a middle
> click here? I have not tried this, but I'd guess that a middle button click
> would give you a decimal value of 4 (binary 100, the third mouse button) or 3
> (binary 011, the combination of first and second buttons).

Yes, this is exactly what I expected when I read the documentation.  However, when I actually tried the function in the code, I found that it was always returning 0x0, regardless of which mouse button I used on the menu item.  The only time it did not return zero was when I pressed more than one button at the same time, and in that case, it seems to return the button that was pressed second.  So, for instance, clicking the left and right mouse buttons simultaneously (but perhaps with the right button slightly after the left) would produce binary 010.  Clicking the middle and right buttons together might produce binary 100 or 010.

My guess is that an event handler somewhere is stripping some information from the current event's button state.  Hence this solution, which is admittedly not ideal.  This patch allows the user to simulate a middle-click by clicking any two mouse buttons.  As I mentioned, this method is not without precedent, but it is certainly "hackish", especially when the user will not understand why they need to simulate the middle mouse button if their mouse already has a middle button!  A better solution would probably involve subclassing NSMenu (or whichever class is handling the click event and telling NSMenuItem to perform its action).  Another possible course of action is to just drop the emulated "middle-click" part for now, as the keyboard modifier part of the code does work and provides at least some of the missing functionality.

Comment 19

[Justin Gregory]

Attachment: Provides keyboard modifier support for clicked menu bar links in OS X

This patch leaves out the awkward mouse button scenarios from the attachment in comment 16.  It does not provide any kind of middle-click support, but it does provide keyboard modifier support.  In other words, command+click opens a link in a new tab, shift+click in a new window, etc.  As such, it does not completely solve this bug but does provide an alternative solution to the problem, and in doing so provides functionality that should be included in OS X Firefox anyways.

Comment 20

[Josh Aas]

Comment on attachment 370504 [details] [diff] [review]
Provides keyboard modifier support for clicked menu bar links in OS X

You can kill off the comment, that much should be super obvious from reading the code. You can also kill off the blank line after the GetCurrentEventKeyModifiers call.

Post a new patch with that and request sr from roc, thanks!

Comment 21

[Josh Aas]

You might want to post the updated patch in a new bug that is specific to modifier keys, obsolete the patch I just reviewed here and leave this bug for mouse clicking. I'll re-mark r+ on the new bug.

Comment 22

[Justin Gregory]

OK, I've created bug 490002, which deals with only keyboard support, and added the attachment there.  I'll mark the latest attachment here as obsolete.

Comment 23

[Justin Gregory]

Comment on attachment 370504 [details] [diff] [review]
Provides keyboard modifier support for clicked menu bar links in OS X

Obsoleted, handled in bug 490002.

Comment 25

[Vincent Lefevre]

Still occurs in Firefox 3.5.3 (for both bookmarks and history menus).

Comment 26

[Asher]

This is still an issue on the trunk, it's something that's highly annoying on a regular basis (should be one 'paper cut'...)  Is there some hardcore technical reason why this can't be fixed yet, or is it a resourcing issue?

Seems like it should be low-hanging fruit.

Comment 28

[Marco Castelluccio [:marco]]

I've the same problem on Linux, should I open a new bug?

Comment 29

[tomdclark]

I have the same problem with the bookmarks menu (as well as the history menu). I'm using FF 17.0.1 running under OSX 10.8.2.   The problem exist even when FF is run in safe mode. Mouse middle click behaves normally otherwise whether in FF, other browers, or in non-browser applications.

Comment 30

[Abdallh Kritm]

this problem still available on OSX

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 6 duplicates and 10 votes.
:spohl, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 32

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.

Comment 33

[Stephen A Pohl [:spohl]]

No recent reports of this issue. Tentatively closing.