418091 - [OS X] keyboard shortcuts with symbols not shown in menus

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[computerjoe]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-GB; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-GB; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11

In Firefox/2.0.0.11 there isn't a Mac keyboard shortcut for 'Clear Private Data'. When one is assigned via System Preferences, it is useless (it doesn't clear private data when pressed).

Reproducible: Always

Steps to Reproduce:
Assigning via system preferences (Leopard)

1. Go to System Prefs
2. Keyboard and Mouse
3. Keyboard Shortcuts
4. +
5. Application: Firefox
6. Menu title: Clear Private Data
7. Any shortcut
8. Go into Firefox, use that shortcut
9. Go to History, observe how it is not clear.
Actual Results:  
Nothing!

Expected Results:  
Private data should be deleted

My Firefox is set to remember how I want my private data cleared. Profile imported from WinXP.

Comment 1

[Thomas D. (:thomas8)]

This bug was reported against Firefox 2.0, which is now outdated.
Computerjoe (or anyone with a mac), do you still see this problem in the most recent build of FF 3?

After checking this out on a Mac (I can't), perhaps this bug can be closed.

Comment 2

[computerjoe]

(In reply to comment #1)
> This bug was reported against Firefox 2.0, which is now outdated.
> Computerjoe (or anyone with a mac), do you still see this problem in the most
> recent build of FF 3?
> 
> After checking this out on a Mac (I can't), perhaps this bug can be closed.

I experience this bug in

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-GB; rv:1.9.0.5) Gecko/2008120121 Firefox/3.0.5 Ubiquity/0.1.5

too.

It's worth noting that 'tools' highlights when the keys are pressed but the history still isn't deleted. Very odd.

Comment 3

[Nickolay_Ponomarev]

There *is* a keyboard shortcut (Cmd+Shift+Delete), it works, but isn't shown in the menu.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I wonder if this is similar to bug 404422. Both shortcuts have one item in common. They don't use a letter but a special key next to the shift and accel key. Could that be the problem?

Comment 5

[Steven Michaud [:smichaud] (Retired)]

> They don't use a letter but a special key next to the shift and
> accel key. Could that be the problem?

Off the top of my head, I don't know.

Comment 6

[Markus Stange [:mstange]]

Setting these shortcuts on a menu in Interface Builder is no problem, so it should be possible for Firefox, too. Maybe some keys aren't translated into their Cocoa equivalents correctly in our menu implementation.

Comment 7

[Markus Stange [:mstange]]

In our Cocoa menu implementation, I meant. But that's only guessing.

Comment 8

[Neil Deakin]

The code in nsMenuItemX::Create appears to only look at the 'key' attribute on a key. It should also be looking at the 'keycode' attribute.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

But the 'sanitizeItem' menu item in the Tools menu (in
browser/base/content/browser-menubar.inc) doesn't have a keycode
property:

  <menuitem id="sanitizeItem"
            accesskey="&clearRecentHistory.accesskey;"
            label="&clearRecentHistory.label;"
            key="key_sanitize"
            command="Tools:Sanitize"/>

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Oops, should have looked for the 'key_sanitize" key.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

And yes, this (in browser/base/content/browser-sets.inc) does have a
keycode property:

  <key id="key_sanitize" command="Tools:Sanitize" keycode="VK_DELETE"
       modifiers="accel,shift"/>

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Looks like we should dupe bug 404422 then because it's the identical situation there.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Yes:

  <key id="goHome" keycode="VK_HOME" command="Browser:Home" modifiers="alt"/>

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Markus, do you have time to deal with this?

If not, I can do it.  But I suspect you're more familiar with this sort of thing than I am.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

By the way, Neil, thanks for figuring out what causes this bug!

By the way, Neil, thanks for figuring out what causes this bug!

Do you know if a 'key' property's 'keycode' property is always set to
the string equivalent of one of the "virtual keys"?
E.g. '"VK_DELETE"' or '"VK_HOME"'?

Comment 17

[Steven Michaud [:smichaud] (Retired)]

(Oops, didn't mean to say it twice!)

Comment 18

[Neil Deakin]

It should always be a VK_HOME or similar VK_ constant.

It might be useful also to know that the non-Mac menu code uses the translations in http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/keys.properties to map the keys into localized strings for display in the menu. I'm mentioning this in case the Mac platform code doesn't do this itself.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

I've started looking into this.  But now I've run into a problem for
which there may be no good solution.  In other words, there may be no
good (general) way to fix this bug.

OS X uses a special set of symbols to indicate that a menu item has a
keyboard shortcut (aka a key equivalent) -- one symbol per key.  So
command+shift+delete (or more correctly command+shift+backspace) would
be indicated by three symbols -- each a special Unicode character.
(This is different from Windows or Linux, where each key in a keyboard
shortcut can be represented by a whole word.)

The OS automatically finds the symbols for the shortcut's modifier
keys (e.g. command or shift).  But you need to specify the "key
equivalent" (in this bug's case the delete/backspace key) directly,
using [NSMenuItem setKeyEquivalent:].  As it happens, OS X supports
using defines for a small number of special keys.  For example, the
delete/backspace key can be specified (to [NSMenuItem
setKeyEquivalent:]) using NSBackspaceCharacter.  But there are lots of
special keys in Firefox's list of "virtual keys" for which there
aren't any special defines.  One of them is VK_HOME.

If there existed Unicode representations of these keys, we might be
able to use them ... even if Apple considered the practice
non-standard.  But I'm not eager to start looking through the entire
Unicode character set, experimentally feeding them to [NSMenuItem
setKeyEquivalent:] one by one.

So I have a question for you all:  Does anyone know of a list that
matches "virtual keys" (like those in nsIDOMKeyEvent.idl) with Unicode
characters that can be used to display them?  I'm particularly
interested in keys (like VK_HOME) that I already know FF uses on the
Mac, and also in keys that are likely to get used (like the F keys --
e.g. F1, F5, F10 and so forth).

Comment 20

[Neil Deakin]

Don't know for sure but a quick search shows some sample code at http://lists.apple.com/archives/cocoa-dev/2002/Oct/msg01750.html
using constants from http://developer.apple.com/mac/library/DOCUMENTATION/Cocoa/Reference/ApplicationKit/Classes/NSEvent_Class/Reference/Reference.html#//apple_ref/c/econst/NSUpArrowFunctionKey

Terminal displays cursor movement shortcuts on the View menu with various arrows.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Thanks again, Neil, for the info.  I tried several keys in this list
with [ChildView setKeyEquivalent:], and they all worked fine.

So here's a patch that fixes this bug ... but it's not as simple as
I'd hoped it would be.

I discovered that telling the OS about the Option+Home keyboard
shortcut (using my alterations to nsMenuItemX) stopped it from
working!  Fixing the problem required me to mess with [ChildView
performKeyEquivalent:] -- which is not for the faint of heart :-)

Both before and after my changes, the OS sends every keyDown event for
a function key first to performKeyQuivalent, then possibly also to
keyDown.  Before my changes, performKeyEquivalent always returned NO
on a function key keyDown event (even a function key event with
modifiers), on the assumption that the OS would always then send the
event to [ChildView keyDown:].  But it turns out this isn't always
true for modified function key events.

Before we told the OS about the Option+Home keyboard shortcut, an
Option+Home keyDown event would always get sent to keyDown after
performKeyEquivalent returned NO on it.  But afterwards the OS stopped
sending it to keyDown -- so the Option+Home shortcut stopped working.

So I fixed the problem by having performKeyboardShortcut return NO
only on non-modified function key events.  With my patch it now always
handles (and returns YES on) modified function key events.

Here's a tryserver build made with my patch:
https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla418091/bugzilla418091-macosx.dmg

Please test it as throughly as you can!  Pay particular attention to
function keys -- both alone and in combination with modifiers.

I'm encouraged that PageDown/PageUp and Ctrl+PageDown/Ctrl+PageUp
still work as expected.  But I need to find out all the other function
keys that FF uses on the Mac, and test them too.

By the way (in case you hadn't noticed), Apple's notion of "function
keys" covers a lot more than just the F1 through F12 keys at the top
of the keyboard.  You get some idea of this from the Apple doc Neil
referred to in comment #20.

Apple doesn't seem to count Esc and Tab as function keys, though.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Steven, that try server build looks pretty good! Those shortcuts are visible now and I wasn't able to see any regression with exiting key combination with modifiers. I have tested the following:

* (shift) F3 [search]
* F5 [reload]
* F7 [caret mode]
* F11 [fullscreen]

Comment 23

[Steven Michaud [:smichaud] (Retired)]

Thanks, Henrik!

To your list I'd add the following (some of which I already mentioned
above).  All of them work fine with my patch.

* PageUp/PageDown -- scroll up or down one page
* Control+PageUp/PageDown -- go back or forward one tab

* Shift+left/right -- extend selection one character left or right
* Shift+up/down -- extend selection one line up or down

* Control+Shift+left/right -- extend selection one word left or right

* Option+Delete(Backspace) -- delete word left
* Option+Del -- delete word right

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Josh, any chance to get this patch reviewed?

Comment 25

[Josh Aas]

Comment on attachment 423024 [details] [diff] [review]
Fix

Doesn't apply any more and I think a lot has changed since this was made. Sorry for the bad review time, I'll get to a new patch quickly.

Comment 26

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Fix (unbitrotted)

I unbitrotted the menu item part. I couldn't find any equivalent child view code and it _seems_ to be working, so maybe that's not needed anymore? I know devtools was hoping this would get done since some of their menuitems aren't showing shortcuts (eg, shift+F4)

Comment 27

[Josh Aas]

(In reply to Paul O'Shannessy [:zpao] from comment #26)

> I unbitrotted the menu item part. I couldn't find any equivalent child view
> code and it _seems_ to be working, so maybe that's not needed anymore?

I wouldn't be surprised if we don't need it any more. I rewrote the way we handle key events in bug 584965, the new behavior is much more sane.

Comment 28

[Josh Aas]

Comment on attachment 597144 [details] [diff] [review]
Fix (unbitrotted)

Review of attachment 597144 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for updating this.

::: widget/cocoa/nsMenuItemX.mm
@@ +232,5 @@
> +  PRUint32 keyCodeNameLength = keyCodeName.Length();
> +  const char* keyCodeNameStr = keyCodeName.get();
> +  for (PRUint16 i = 0; i < (sizeof(gMacKeyCodes) / sizeof(gMacKeyCodes[0])); ++i) {
> +    if (keyCodeNameLength == gMacKeyCodes[i].strlength &&
> +        !nsCRT::strcmp(gMacKeyCodes[i].str, keyCodeNameStr))

I'd prefer to not use boolean operators on non-boolean values, for clarity (it leads some people to think the return value is boolean). Can you please compare strcmp's result to zero (== 0) instead?

Also, please put braces around all if blocks.

Comment 29

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c41c28052c6

Comment 30

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Fix (checked in)

Patch as checked in. The changes I made were all trivial so of course author=smichaud.

Comment 31

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/7c41c28052c6

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Looks great with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:13.0a1) Gecko/20120214 Firefox/13.0a1 ID:20120214031227