Last Comment Bug 735648 - Cmd + Opt + something shortcut keys don't work in "Dvorak - Qwerty ⌘" keyboard layout
: Cmd + Opt + something shortcut keys don't work in "Dvorak - Qwerty ⌘" keyboar...
Status: RESOLVED FIXED
[key hell]
:
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-14 06:23 PDT by Jason Garber
Modified: 2012-03-30 12:56 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Dvorak-Qwerty Firefox bug.m4v (1.01 MB, application/octet-stream)
2012-03-14 06:23 PDT, Jason Garber
no flags Details
Patch (3.32 KB, patch)
2012-03-15 06:45 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
smichaud: review+
karlt: feedback-
Details | Diff | Review
Patch (8.06 KB, patch)
2012-03-28 21:53 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Patch (6.80 KB, patch)
2012-03-29 16:37 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
karlt: review+
Details | Diff | Review

Description Jason Garber 2012-03-14 06:23:16 PDT
Created attachment 605723 [details]
Dvorak-Qwerty Firefox bug.m4v

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:11.0) Gecko/20100101 Firefox/11.0
Build ID: 20120312181643

Steps to reproduce:

Mac OS X has a keyboard layout called "Dvorak - Qwerty ⌘" where the keys are ordinarily in the Dvorak layout, but when the ⌘ (Command) key is held down, it temporarily switches back to Qwerty. Thus, you can type text in Dvorak, but all the keyboard shortcuts in your muscle memory still work.

While using he Dvorak-Qwerty ⌘ layout, I attempted to trigger the Web Developer: Inspect menu item with shortcut key Opt-Cmd-I.


Actual results:

The tools menu highlights briefly (but does not open), but the Inspect mode is not triggered.


Expected results:

The inspect mode should have started. Note that if I first click on the Tools menu, the keyboard shortcut works as expected.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-15 06:45:04 PDT
Created attachment 606187 [details] [diff] [review]
Patch

tryserver builds:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/masayuki@d-toybox.com-9579ab1bf9a6/

This patch depends on the patches of bug 479942 and others.
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-15 06:51:07 PDT
Comment on attachment 606187 [details] [diff] [review]
Patch

I think that this is a mistake.

|!isDvorakQWERTY| means we don't need to append altCharCodes for Dvorak-Qwerty because charCode and cmdedChar (or cmdedShiftChar) must be same. But there is the case Alt key is pressed (Mac's Alt key is similar to AltGr of other platforms).

So, if cmdedChar or cmdedShiftChar doesn't match to the charCode, we should append them.
Comment 3 Steven Michaud [:smichaud] (Retired) 2012-03-28 08:52:39 PDT
Comment on attachment 606187 [details] [diff] [review]
Patch

I haven't tested this.  But it looks like it shouldn't do harm.
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-28 19:14:24 PDT
karlt:

Would you check the patch for feedback? I'd like to land this ASAP due to the risk.
Comment 5 Karl Tomlinson (ni?:karlt) 2012-03-28 19:47:31 PDT
Looks like this code was added in http://hg.mozilla.org/mozilla-central/rev/9515fda788fd (bug 432388).  Still making sense of it.
Comment 6 Karl Tomlinson (ni?:karlt) 2012-03-28 20:17:09 PDT
Comment on attachment 606187 [details] [diff] [review]
Patch

It doesn't feel right to me to add the Qwerty alternative char codes when
!isMeta, because we have already included the Dvorak char codes.

For other isCmdSwitchLayout layouts, don't we still want to include
cmdedChar when preferredCharCode is cmdedShiftChar?

>+  // cmdedChar or comdedShiftChar, we don't need to do it.

Please fix up comded here.
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-28 21:53:07 PDT
Created attachment 610429 [details] [diff] [review]
Patch

Did you mean this?

Comparing charCode makes it more complicated. Therefore, I removed it. It shouldn't be an issue for performance.
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-28 21:55:02 PDT
Ah, note that the new patch includes the change for bug 677252 for new tests because the bug is still blocked by bug 447757. But this bug should go first.
Comment 9 Karl Tomlinson (ni?:karlt) 2012-03-29 16:20:53 PDT
Comment on attachment 610429 [details] [diff] [review]
Patch

>+  bool isCmdPressedOnDvorakQWERTY = aKeyEvent.isMeta && isDvorakQWERTY;
>+

>-  if ((unshiftedChar || shiftedChar) &&
>-      (!aKeyEvent.isMeta || !isDvorakQWERTY)) {
>+  if ((unshiftedChar || shiftedChar) && !isCmdPressedOnDvorakQWERTY) {

>-  // Latin char for the key. But don't append at Dvorak-QWERTY.
>-  if ((cmdedChar || cmdedShiftChar) && isCmdSwitchLayout && !isDvorakQWERTY) {
>+  if ((cmdedChar || cmdedShiftChar) && isCmdSwitchLayout &&
>+      (!isDvorakQWERTY || isCmdPressedOnDvorakQWERTY)) {

Yes, this logic looks good to me, but I don't think the boolean isCmdPressedOnDvorakQWERTY is helping here.
I would just leave the first test as is and use (!isDvorakQWERTY || aKeyEvent.isMeta) in the second, or perhaps swap the order of the variables in one of the tests to be consistent.
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-29 16:37:34 PDT
Created attachment 610749 [details] [diff] [review]
Patch

Thanks.
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-29 20:39:37 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/31f1d027a49f
Comment 12 Ed Morley [:emorley] 2012-03-30 12:56:24 PDT
https://hg.mozilla.org/mozilla-central/rev/31f1d027a49f

Note You need to log in before you can comment on or make changes to this bug.