490002 - Keyboard modifiers should alter the behavior of clicking bookmarks in the native menu

Status: REOPENED

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

Description

[Justin Gregory]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9.2a1pre) Gecko/20090422 Minefield/3.6a1pre
Build Identifier: 3.6a1pre

Holding down the Cmd key when clicking a link in the native bookmarks menu should open the link in a new tab.  Holding down the shift key should open it in a new window, and so on for various other combinations of modifier keys.  Currently, all such combinations open the link in the current tab.

Reproducible: Always

Comment 1

[Justin Gregory]

Closely related are bug 313718 and bug 469390, which deal with mouse button (middle-click) behavior.

Comment 2

[Justin Gregory]

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

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/4113fe30352d

Thanks Justin!

Comment 4

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

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

a191=beltzner

Comment 5

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5d3dc9101db5

Comment 6

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

This checkin was in a range identified with a Ts Shutdown regression on Windows:

Regression: Ts Shutdown increase 27.64% on WINNT 5.1 Firefox3.5
   Previous results:
       357.263 from build 20090506145316 of revision 486b76052a94 at 2009-05-06 14:53:00
   New results:
       456.0 from build 20090506155401 of revision 7aa4483585bd at 2009-05-06 15:54:00
http://graphs-new.mozilla.org/graph.html#tests=[{"machine":32,"test":36,"branch":3},{"machine":33,"test":36,"branch":3},{"machine":34,"test":36,"branch":3},{"machine":35,"test":36,"branch":3},{"machine":48,"test":36,"branch":3}]&sel=1241564040,1241736840

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The chances of this patch causing any kind of performance regression are nil.

Comment 8

[Johnathan Nightingale [:johnath]]

This seems to have caused a blocking regression in flash behaviour on Mac - bug 494884.

Comment 9

[Johnathan Nightingale [:johnath]]

Backed this out on branch to fix 494884

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0e17edad9641

Comment 10

[Johnathan Nightingale [:johnath]]

14:10 < johnath> josh: backed the 490002 patch out on 191, do you have a preference about central?  I think the easiest thing, bookkeeping-wise, would be to back it out on central as well, but I also care less than 191, so if you want it to stay while you work a fix, lemme know
14:11 < josh> johnath: I'd prefer that it be taken out on m-c and the bug re-opened
14:12 < johnath> josh: okay, great.  Will do.

http://hg.mozilla.org/mozilla-central/rev/993a4c7e44af

Comment 11

[Justin Gregory]

Attachment: Includes previous KB modifier patch and fixes Flash 10 regression (bug 494884)

Flash 10 appears to use our menu code to reload a page when the user presses Cmd+R.  (See bug 494884).  This has the unfortunate side-effect of reloading the page in a new tab, due to the Command key being held down.  The new code in this patch checks if the function was called by a Cmd+R key event and drops the Command key modifier flag if that is the case, preventing a new tab from opening.

Comment 12

[[e-mail]]

(In reply to comment #11)
> Flash 10 appears to use our menu code to reload a page when the user presses
> Cmd+R.  (See bug 494884).  This has the unfortunate side-effect of reloading
> the page in a new tab, due to the Command key being held down.  The new code in
> this patch checks if the function was called by a Cmd+R key event and drops the
> Command key modifier flag if that is the case, preventing a new tab from
> opening.
Not being a Mac developer I'm wondering whether a) you'd expect other key events b) you'd want the modifier flags of those key events.

Comment 13

[Justin Gregory]

On a Mac you can get to the menu bar by pressing Ctrl+F2, and navigate it with the keyboard.  This means that the user would expect Cmd+Enter, Shift+Ctrl+Enter, or whatever else to work when he has selected the bookmark item.

So, "yes" to both (a) and (b).

The thing that concerns me is that there may be other keyboard shortcuts that Flash 10 is using the bookmark menu system to implement.  I tried Cmd+Right and Cmd+Left, but they didn't seem to have the problem of the Cmd+R bug.

Comment 14

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

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

(clearing flag for query sanity)

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it would be saner to just ignore the modifier keys in any situation where the menu code has been triggered by a key event. I really don't think we should just hack cmd-R. The case of users navigating menus with the keyboard and pressing cmd-enter etc is a very, very deep edge case IMHO...

Comment 16

[Justin Gregory]

Agreed: this is an ugly hack.  Suppose it were modified to look for modifiers on key events only if the key pressed was [Enter] or [Spacebar]?  That seems more sane to me.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That works.

Comment 18

[Josh Aas]

Comment on attachment 379934 [details] [diff] [review]
Includes previous KB modifier patch and fixes Flash 10 regression (bug 494884)

Canceling review since we're waiting on a new patch.

Comment 19

[Justin Gregory]

I've been working on this for a few days now, and I'm a bit befuddled.  When I use the keyboard to reach the menu and select a bookmark with the Enter key, [currentEvent charactersIgnoringModifiers] returns the byte code 0xef9c85, which as far as I can tell is just gibberish.  However, the key code is 120, which I have found corresponds to the F2 key.  This makes sense, as Ctrl+F2 gets you to the native menu by keyboard.  But it is also possible to use the mouse to click the "Bookmarks" menu, and then scroll down using the arrow keys and Enter to select an item.  In that case, the event received is not even a keyboard event.  (Pressing Cmd+R in the Jalopnik video issues a keyboard event with charactersIgnoringModifiers returning 'r'.)

All of that was using Cocoa methods, so I switched over to Carbon.  I used AcquireFirstMatchingEventInQueue to get the current event, and then try to check its class and kind.  Using the Enter key or Spacebar to select a menu item works, that is, a 'keyb'-class Carbon event is returned, and the kEventParamKeyMacCharCodes parameter returns 0x0D and 0x20 as expected.  However, when pressing Cmd+R on the video at the Jalopnik site, the event class is 'cgs ', which seems to be an undocumented internal Apple event class.  Unfortunately, this is the same Carbon event class that gets returned if you simply use the mouse to click on a bookmark, so I don't have any way to differentiate the Cmd+R from a mouse click.  That is, since the 'cgs ' event class is undocumented, I don't know what event parameters I can query for.

All of this to say that it seems to me that at the moment the only viable workaround to this dilemma is the previously-submitted patch, as ugly as it is.  Josh has mentioned elsewhere the right way to fix this in the long run:
https://bugzilla.mozilla.org/show_bug.cgi?id=494884#c23
Once bug 491141 is fixed, it's my understanding that this will be a non-issue.

Until such a time, I yield to the wisdom of others the question of whether to include the latest patch (ugly hack) for now or to fore-go the key modifier functionality until the long-term solution is implemented.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This isn't going to make Firefox 3.5. Let's wait for the real fix in bug 491141. Thanks for trying.

Comment 21

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P3 - Polish issue that is in a secondary interface, occasionally encountered, or is not easily identifiable.

Comment 22

[Andrew Ho]

Just to give another end user perspective, I wouldn't consider this a "polish issue that is in a secondary interface." Middle-clicking on a bookmark from the Bookmarks menu to open it in a new tab is something I do every day on non-Mac OS X platforms, and this is actually the #1 UI gotcha that I encounter almost every day since I have switched to Mac OS.

My usual "dance of shame" now is (1) open Bookmarks menu with mouse and find bookmark I want to open, (3) remember that I can't middle-click or Cmd+click it to open it in a new tab, (4) click on page to close Bookmarks menu, (5) open new tab with Cmd+T, and (6) repeat step 1, then click to open.

Cmd+click and middle click works on bookmark toolbar items and HTML links, so it is very inconsistent and unexpected for it to not work in the Bookmarks menu.

Anyway, just adding a plea in the hopes someone looks at this again.  :)

Comment 23

[Justin Gregory]

I'm too busy with other things these days... better assign this bug to someone else.

Comment 26

[Jithin Emmanuel]

I cant believe that a trivial functionality like middle clicking on bookmark menu is broken for over 2 years with no sign of a fix.
Firefox is the only browser is mac with this problem. This works perfectly fine in Chrome, Opera and Safari

Comment 27

[David Kelly]

This bug (and the related bug 313718) have been a problem for over seven years. Is nobody able to sort it out !?