Open Bug 490002 Opened 11 years ago Updated 4 years ago

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

Categories

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

All
macOS
defect

Tracking

()

REOPENED
mozilla1.9.2a1

People

(Reporter: jmgregory, Unassigned)

References

Details

(Keywords: polish, Whiteboard: [polish-easy][polish-interactive][polish-p3], tpi:+)

Attachments

(2 obsolete files)

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
Closely related are bug 313718 and bug 469390, which deal with mouse button (middle-click) behavior.
Attachment #374478 - Flags: superreview?(roc)
Attachment #374478 - Flags: review?(joshmoz)
Attachment #374478 - Flags: review?(joshmoz) → review+
Attachment #374478 - Flags: superreview?(roc) → superreview+
Assignee: nobody → jmgregory
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
http://hg.mozilla.org/mozilla-central/rev/4113fe30352d

Thanks Justin!
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.2a1
Attachment #374478 - Flags: approval1.9.1?
Whiteboard: [polish-easy]
Comment on attachment 374478 [details] [diff] [review]
Provides keyboard modifier support for clicked native menu bar links in OS X

a191=beltzner
Attachment #374478 - Flags: approval1.9.1? → approval1.9.1+
Keywords: checkin-needed
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
The chances of this patch causing any kind of performance regression are nil.
Depends on: 494884
This seems to have caused a blocking regression in flash behaviour on Mac - bug 494884.
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #374478 - Attachment is obsolete: true
Attachment #379934 - Flags: superreview?(roc)
Attachment #379934 - Flags: review?(joshmoz)
(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.
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 on attachment 374478 [details] [diff] [review]
Provides keyboard modifier support for clicked native menu bar links in OS X

(clearing flag for query sanity)
Attachment #374478 - Flags: approval1.9.1+
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...
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.
Attachment #379934 - Attachment is obsolete: true
Attachment #379934 - Flags: superreview?(roc)
Attachment #379934 - Flags: review?(joshmoz)
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.
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.
This isn't going to make Firefox 3.5. Let's wait for the real fix in bug 491141. Thanks for trying.
Depends on: 491141
No longer depends on: 494884
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.
Whiteboard: [polish-easy] → [polish-easy][polish-interactive][polish-p3]
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.  :)
I'm too busy with other things these days... better assign this bug to someone else.
Assignee: jmgregory → nobody
Duplicate of this bug: 660454
Duplicate of this bug: 649327
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
This bug (and the related bug 313718) have been a problem for over seven years. Is nobody able to sort it out !?
Priority: -- → P4
Whiteboard: [polish-easy][polish-interactive][polish-p3] → [polish-easy][polish-interactive][polish-p3], tpi:+
You need to log in before you can comment on or make changes to this bug.