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

VERIFIED FIXED in mozilla13

Status

()

Core
Widget: Cocoa
--
trivial
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: computerjoe, Assigned: smichaud)

Tracking

({polish})

Trunk
mozilla13
All
Mac OS X
polish
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Reporter)

Updated

10 years ago
Hardware: PC → Macintosh

Updated

10 years ago
Component: Keyboard Navigation → General
Hardware: Macintosh → PC
Version: unspecified → 2.0 Branch

Updated

10 years ago
QA Contact: keyboard.navigation → general
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.
(Reporter)

Comment 2

9 years ago
(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.
(Reporter)

Updated

9 years ago
Component: General → Private Browsing
Version: 2.0 Branch → 3.0 Branch
Component: Private Browsing → Keyboard Navigation
QA Contact: general → keyboard.navigation

Comment 3

8 years ago
There *is* a keyboard shortcut (Cmd+Shift+Delete), it works, but isn't shown in the menu.
Severity: normal → trivial
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: polish
Summary: No keyboard shortcut for clear private data; custom ones don't work → (Mac OS X-only) keyboard shortcut not shown for Clear Recent History
Version: 3.0 Branch → Trunk
Component: Keyboard Navigation → Menus
QA Contact: keyboard.navigation → menus
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?
(Assignee)

Comment 5

8 years ago
> 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.
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.
Component: Menus → XP Toolkit/Widgets: Menus
Product: Firefox → Core
QA Contact: menus → xptoolkit.menus
In our Cocoa menu implementation, I meant. But that's only guessing.
Component: XP Toolkit/Widgets: Menus → Widget: Cocoa
QA Contact: xptoolkit.menus → cocoa
Hardware: x86 → All
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.
(Assignee)

Comment 9

8 years ago
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"/>
Oops, should have looked for the 'key_sanitize" key.
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"/>
Looks like we should dupe bug 404422 then because it's the identical situation there.
Yes:

  <key id="goHome" keycode="VK_HOME" command="Browser:Home" modifiers="alt"/>
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.
(Assignee)

Updated

8 years ago
Duplicate of this bug: 404422
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"'?
(Oops, didn't mean to say it twice!)
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.
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).
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.
Created attachment 423024 [details] [diff] [review]
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/smichaud@pobox.com-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.
Assignee: nobody → smichaud
(Assignee)

Updated

8 years ago
Attachment #423024 - Flags: review?(joshmoz)
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]
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
Josh, any chance to get this patch reviewed?

Comment 25

6 years ago
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.
Attachment #423024 - Flags: review?(joshmoz)
Created attachment 597144 [details] [diff] [review]
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)
Attachment #597144 - Flags: review?(joshmoz)

Comment 27

6 years ago
(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

6 years ago
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.
Attachment #597144 - Flags: review?(joshmoz) → review+

Updated

6 years ago
Attachment #423024 - Attachment is obsolete: true

Updated

6 years ago
Attachment #597144 - Attachment description: Fix (unbitrotted?) → Fix (unbitrotted)
Summary: (Mac OS X-only) keyboard shortcut not shown for Clear Recent History → [OS X] keyboard shortcuts with symbols not shown in menus
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c41c28052c6
Whiteboard: [inbound]
Created attachment 597534 [details] [diff] [review]
Fix (checked in)

Patch as checked in. The changes I made were all trivial so of course author=smichaud.
Attachment #597144 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/7c41c28052c6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla13
Duplicate of this bug: 294437
Looks great with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:13.0a1) Gecko/20120214 Firefox/13.0a1 ID:20120214031227
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.