1078825 - ShortcutUtils (e.g. Australis menu panel tooltips, devtools) ignore ui.key.accelKey pref

Status: NEW

(Toolkit :: General, defect)

Description

[Adam Moore]

Attachment: Adds a ui.key.accelKey lookup to ShortcutUtils.prettifyShortcut()

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140923175406

Steps to reproduce:

I'm running Firefox on Linux, accessing the machine remotely from an OSX machine.  The ui.key.accelKey pref is set to 224 in order to map the Mac's command key to handle keyboard shortcuts instead of CTRL.  The keyboard shortcuts work as expected.


Actual results:

The tooltips in the 'old' menu have the correct value (VK_META from platformKeys.properties) for the modifier keys.  However, all menu panel icons that have keyboard shortcuts have CTRL instead of VK_META in the tooltips.


Expected results:

Modifiers in tooltips should be VK_META based on my config.  The platform modifier for these tooltips is hard-coded - the code forks on Services.appinfo.OS == "Darwin", and does not look at the ui.key.accelKey pref.  I tried working around this behavior in an extension, but it can't be done - so I'm attaching a quick diff that should make it clear how to fix it.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8500720 [details] [diff] [review]
Adds a ui.key.accelKey lookup to ShortcutUtils.prettifyShortcut()

I'm not familiar with this file. Enn, how about you?

# FYI: C++ code's logic is now here:
# http://mxr.mozilla.org/mozilla-central/source/widget/shared/WidgetEventImpl.cpp#254


nit: the indent is wrong. Use 2 spaces.

Comment 2

[:Gijs (he/him)]

I don't think the patch is right; if I set the pref to indicate 'alt' or 'ctrl' on OS X, presumably the shortcut description should be updated there as well. The pref setting for 'accel' should also not be used to determine what key description to use for the 'access' modifier key (for which it seems there are other prefs instead).

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to :Gijs Kruitbosch from comment #2)
> I don't think the patch is right; if I set the pref to indicate 'alt' or
> 'ctrl' on OS X, presumably the shortcut description should be updated there
> as well.

If you meant that shortcut key labels in XUL menu items should be changed at changing the pref directly, it's not supported yet. For doing that we need to store all nsMenuFrames only for updating the labels.

Comment 4

[:Gijs (he/him)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3)
> (In reply to :Gijs Kruitbosch from comment #2)
> > I don't think the patch is right; if I set the pref to indicate 'alt' or
> > 'ctrl' on OS X, presumably the shortcut description should be updated there
> > as well.
> 
> If you meant that shortcut key labels in XUL menu items should be changed at
> changing the pref directly, it's not supported yet. For doing that we need
> to store all nsMenuFrames only for updating the labels.

No, I mean that the patch only fixes the specific case the reporter reported: it sets a flag that basically means "this either is or is pretending to be OS X", but the problem of respecting the pref is broader than that.

Comment 5

[Adam Moore]

Attachment: Honors accelKey and menuAccessKey prefs

Both points are correct.  Attached a less selfish attempt.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8501266 [details] [diff] [review]
Honors accelKey and menuAccessKey prefs

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

Thanks for the updated patch! This looks much more correct. However, I still have some nits. First off, do you want to generate it so that it has a header at the top, roughly of the form:

Bug 1078825 - ShortcutUtils shouldn't ignore ui.key.accelKey pref, r?enndeakin

or such (see https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F ) Further nits below, but if you can write a next version, it probably makes sense to request review from Neil who knows the XUL base for this better than I do.

::: toolkit/modules/ShortcutUtils.jsm
@@ +61,4 @@
>      let haveCloverLeaf = false;
>  
>      if (elemMod.match("accel")) {
> +      let accelKey = Services.prefs.getIntPref("ui.key.accelKey");

Sadly, unlike its C++ cousin which you looked at and which takes a default value to use if the pref is not set, this doesn't take a default value and will throw if the pref is not set, so we end up with the ugly:

let accelKey = 0;
try {
  accelKey = ...;
} catch (e) {}

@@ +68,4 @@
>          // XXX bug 779642 Use "Cmd-" literal vs. cloverleaf meta-key until
>          // Orion adds variable height lines.
> +        let prefString = getModifierStringFromKeyCode(accelKey);
> +        if (prefString == "\u2318") {

Nit: I'd prefer:

if (accelKey == Ci.nsIDOMKeyEvent.DOM_VK_META)

so that this works independently of how it was localized (I recognize that that is kind of odd considering the objection to using the cloverleaf is the actual character / font rendering, but even so - it seems too fragile to rely on the locale here and/or the codepoint not changing)

@@ +79,3 @@
>          }
> +      } else if (Services.appinfo.OS == "Darwin") {
> +        haveCloverLeaf = true;

This is slightly wrong because now, if the pref is not set, we always set haveCloverLeaf, even if aNoCloverLeaf is passed.

What I'd do instead is fix up accelKey to be the default before running the other code, ie:

if (!accelKey) {
  accelKey = Ci.nsIDOMKeyEvent[Services.appinfo.OS == "Darwin" ? "DOM_VK_META" : "DOM_VK_CONTROL"];
}

Then getModifierStringFromKeyCode is guaranteed to return something for "prefString" (which I guess should then change names...).

@@ +84,5 @@
>            PlatformKeys.GetStringFromName("MODIFIER_SEPARATOR");
>        }
>      }
>      if (elemMod.match("access")) {
> +      let menuAccessKey = Services.prefs.getIntPref("ui.key.menuAccessKey");

Nit: same here as for the other pref read.

@@ +87,5 @@
>      if (elemMod.match("access")) {
> +      let menuAccessKey = Services.prefs.getIntPref("ui.key.menuAccessKey");
> +      // Looks like this should print nothing if the pref is missing or 0, per
> +      // XULMenuitemAccessible::AccessKey.
> +      // http://mxr.mozilla.org/mozilla-central/source/accessible/xul/XULMenuAccessible.cpp#154

Nit: both this and the previous MXR links will bitrot. If you click on the "hg blame" link at the top, you can get non-bitrotting links.

Comment 7

[Adam Moore]

Attachment: Honors accelKey and menuAccessKey prefs, updated based on feedback

I think this takes care of the nits.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8501421 [details] [diff] [review]
Honors accelKey and menuAccessKey prefs, updated based on feedback

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

Thanks! Yes, this looks perfect to me. Neil, can you doublecheck this mirrors the logic correctly (AFAICT it does, but I don't know that code very well).

Comment 9

[Neil Deakin]

Comment on attachment 8501421 [details] [diff] [review]
Honors accelKey and menuAccessKey prefs, updated based on feedback

>+      // XXX bug 779642 Use "Cmd-" literal vs. cloverleaf meta-key until
>+      // Orion adds variable height lines.
>+      if (accelKey == Ci.nsIDOMKeyEvent.DOM_VK_META) {
>         if (aNoCloverLeaf) {
>           elemString += "Cmd-";
>         } else {
>           haveCloverLeaf = true;
>         }

This needs to be Mac specific. (On other platforms, meta is displayed as 'Meta')

Also, shouldn't 'Cmd-' be localized?

Comment 10

[Adam Moore]

> This needs to be Mac specific. (On other platforms, meta is displayed as
> 'Meta')

Meta is displayed as whatever is defined in platformKeys.properties, which I override in order to show mac-like shortcuts on Linux.

> Also, shouldn't 'Cmd-' be localized?

This bit of code is just carried over from what is there now.  Per the comment, it looks like a hack that should go away eventually.

Comment 11

[Neil Deakin]

(In reply to Adam Moore from comment #10)
> > This needs to be Mac specific. (On other platforms, meta is displayed as
> > 'Meta')
> 
> Meta is displayed as whatever is defined in platformKeys.properties, which I
> override in order to show mac-like shortcuts on Linux.
> 

Why? Linux doesn't have command/clover keys. The right name is 'Meta'.

Comment 12

[Adam Moore]

That's the reason I came across the issue, and most other people won't - I'm using a Mac to control a Firefox browser on a remote Linux machine.

Comment 13

[Neil Deakin]

If someone is using a keyboard with a Meta key on it though, although rare, with your patch if will display Cmd+. It should say "Meta'. On Linux, there is a Meta key even though there isn't usually a physical Meta key, but there is never a Command/clover key. It doesn't make sense to display it that way, even though one might happen to be using a Mac keyboard. In your case, the command key is mapped to what Linux thinks is the Meta key.

Comment 14

[Adam Moore]

The hard-coded Cmd- is code that should not be there at all, but I left it in as is because that was deemed to be sufficient for bug 779642, which is to say the scratchpad isn't able to display properly because of line height problems the clover.  The previous version of my patch changed the hack so that Cmd- was used only when the actual value was the clover.  I changed this per the previous review.

What really needs to be done is for someone to fix the scratchpad and remove this hack.

Comment 15

[Adam Moore]

I got a note about the patch being accepted, but I'm not sure it was.  Will this be merged?  Is there something else I need to do?  If necessary, I'll update the patch to close this out.

Comment 16

[Justin Dolske [:Dolske]]

No, it's not accepted yet. It looked good to Gijs, but not to Neil, so it's not ready for integration.

Skimming the last few comments, I think I'd agree. Linux UI should never say "Cmd-x", always "Meta-x". As I understand Neil's comments, he's saying that the old code clearly only could use that hardcoded "Cmd-" prefix on OS X because of the explicit platform check, and now it doesn't.

Comment 17

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #16)
> No, it's not accepted yet. It looked good to Gijs, but not to Neil, so it's
> not ready for integration.
> 
> Skimming the last few comments, I think I'd agree. Linux UI should never say
> "Cmd-x", always "Meta-x". As I understand Neil's comments, he's saying that
> the old code clearly only could use that hardcoded "Cmd-" prefix on OS X
> because of the explicit platform check, and now it doesn't.

Yes.

Adam, could you update the patch to use the getModifierStringFromKeyCode function for vk_meta (which will return the cloverleaf on mac, and "Meta" elsewhere), and only use "Cmd-" if we're on mac and aNoCloverleaf was passed, and re-request review from Neil?

I think we can deal with fixing/localizing the "Cmd-" bit of scratchpad in a followup, considering it's not a regression.

Comment 18

[Adam Moore]

In my configuration, getModifierStringFromKeyCode will return the clover, but Firefox is running on Linux.  So if I make this change, I'll also have to check to see if the accelKey is actually a clover to cover my use case.  This is probably the right compromise, as it keeps the brittle part (checking for the clover) isolated to my somewhat unusual configuration.

Comment 19

[Adam Moore]

Attachment: ShortcutUtils honors ui.key.accelKey pref

ShortcutUtils honors ui.key.accelKey pref, the revised patch only maps ⌘ to Cmd- on a Mac, or if the accelerator key platform string is a ⌘, r?enndeakin

Comment 20

[Adam Moore]

I had a look at Scratchpad on both Mac and Linux with the clover hack removed, and yeah, it looks a lot better with the hack in place.  In fact, I doubt anyone would want to spend cycles to address it.  So, Cmd- should definately be localized.  What's the process for that?  Would I simply need to add a new entry to platformKeys.properties?

Comment 21

[:Gijs (he/him)]

(In reply to Adam Moore from comment #20)
> I had a look at Scratchpad on both Mac and Linux with the clover hack
> removed, and yeah, it looks a lot better with the hack in place.  In fact, I
> doubt anyone would want to spend cycles to address it.  So, Cmd- should
> definately be localized.  What's the process for that?  Would I simply need
> to add a new entry to platformKeys.properties?

Yup!

Comment 22

[Neil Deakin]

Comment on attachment 8519497 [details] [diff] [review]
ShortcutUtils honors ui.key.accelKey pref

>+
>+      // XXX bug 779642 Use "Cmd-" literal vs. cloverleaf meta-key until
>+      // Orion adds variable height lines.
>+      if (darwin || modString == "\u2318") {

Shouldn't this be && and not || ?

Comment 23

[Adam Moore]

I meant it to be || so that my configuration (Linux with a Mac keyboard) gets the same treatment as a mac.  Technically the darwin check could be omitted, but it was pointed out earlier that the clover test is potentially brittle.  The way it is written limits the exposure to the brittle bit.

Comment 24

[Neil Deakin]

With this patch, if someone has set the preference on Mac to use ctrl as the accelkey, it will print the clover anyway.

Comment 25

[Adam Moore]

Please let me know precisely what you want.  I'm attempting to fix something that is broken today, has been broken since the introduction of Australis.  You should want it fixed as well.  A blunt rejection isn't productive.  Yes, this patch exposes an edge case, and this was done this way per the earlier advice.  I can change it back to simply look for this cloverleaf if that is what will get it accepted, but I do need to get some feedback.  If you don't care that it gets fixed, I'll just keep my local patch and know that I'm on my own regarding this particular cross platform complexity.