Last Comment Bug 677252 - Reimplement keycode computation in cocoa widget
: Reimplement keycode computation in cocoa widget
Status: RESOLVED FIXED
: intl
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: All Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
: 756096 (view as bug list)
Depends on: 694501 630810
Blocks: 412627 413875 448434 479942 545721 603697 631165 649348
  Show dependency treegraph
 
Reported: 2011-08-08 08:21 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2013-10-11 03:24 PDT (History)
9 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0 (115.65 KB, patch)
2011-08-08 08:21 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Patch part.1 Reimplement keycode computation in cocoa widget (113.71 KB, patch)
2011-08-08 20:01 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Patch part.2 Add Eisu keycode for Japanese keyboard layout of Mac (3.52 KB, patch)
2011-08-08 20:02 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Patch part.1 Reimplement keycode computation in cocoa widget (113.90 KB, patch)
2011-08-08 21:08 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
Details | Diff | Review
Patch part.2 Add Eisu keycode for Japanese keyboard layout of Mac (3.95 KB, patch)
2011-08-08 21:09 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: superreview+
Details | Diff | Review
Patch part.1 Reimplement keycode computation in cocoa widget (114.05 KB, patch)
2011-10-11 23:31 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
smichaud: review+
Details | Diff | Review
Patch part.2 Add Eisu keycode for Japanese keyboard layout of Mac (4.10 KB, patch)
2011-10-11 23:32 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
smichaud: review+
Details | Diff | Review
part.1 Reimplement keycode computation in cocoa widget (114.02 KB, patch)
2012-03-14 20:29 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
part.2 Add Eisu keycode for Japanese keyboard layout of Mac (4.08 KB, patch)
2012-03-14 20:29 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
part.1 Reimplement keycode computation in cocoa widget (110.98 KB, patch)
2012-03-30 00:46 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
part.1 Reimplement keycode computation in cocoa widget (110.35 KB, patch)
2012-05-07 20:24 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-08-08 08:21:54 PDT
Created attachment 551463 [details] [diff] [review]
Patch v1.0

This is a bug for refactoring keyCode computation on Mac.

The rules were defined in bug 631165 and bug 630810. And the patch would be landed with bug 630810 (for Win) and bug 447757 (for GTK).

The new rules are:

1. If a key produces an ASCII character without modifier keys, we should use it.
2. Otherwise, if a key produces an ASCII character with Shift key, we should use it.
3. If unshifted character is an ASCII character but shifted character is an ASCII numeric character, we should use it (for AZERTY).
4. If neither unshifted character nor shifted character is not an ASCII character, we should use latest ASCII capable keyboard layout character if the key produces an alphabet or a numeric. (For other ASCII character may be inputtable by another key, so, for avoiding the keycode conflict, we shouldn't use them.)
5. If Command key switches keyboard layout, e.g., DVORAK-QWERTY Command and ß key of German keyboard layout, we should use the switched layout.
6. Otherwise, we set 0 for keyCode.

And also this patch adds new keyCode for 英数 (Ei-Su, means English and Number) key on Japanese keyboard layout. It switches the selected TIS to the turned off Japanese IME (i.e., typically, this is used for turning off Japanese IME), but same caption doesn't exist on other platform's keyboard. I looked for other keyboard layout of CKT, but I couldn't find them. Therefore, I'm not sure whether the other East Asian keyboard layouts have such special keys.

Note that if you want to apply this patch, you need to apply 2 patches of bug 630810, first.

I'd like to land those keycode refactoring patches in early of mozilla9 or mozilla10 cycle.
Comment 1 Mounir Lamouri (:mounir) 2011-08-08 15:48:43 PDT
FWIW, I think it would be better to separate the code refactoring and the addition of the new keyCode (英数) with two separate patches (ideally in two different bugs).
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-08-08 20:01:10 PDT
Created attachment 551665 [details] [diff] [review]
Patch part.1 Reimplement keycode computation in cocoa widget
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-08-08 20:02:10 PDT
Created attachment 551666 [details] [diff] [review]
Patch part.2 Add Eisu keycode for Japanese keyboard layout of Mac
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-08-08 21:08:24 PDT
Created attachment 551672 [details] [diff] [review]
Patch part.1 Reimplement keycode computation in cocoa widget

fix a mistake, sorry for the spam.
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-08-08 21:09:26 PDT
Created attachment 551673 [details] [diff] [review]
Patch part.2 Add Eisu keycode for Japanese keyboard layout of Mac
Comment 6 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-10-10 12:02:50 PDT
smichaud, are you planning to review this?

(I'll try to review mainly non-OSX-widget parts today or tomorrow)
Comment 7 Steven Michaud [:smichaud] (Retired) 2011-10-10 12:08:48 PDT
> smichaud, are you planning to review this?

Yes.

Sorry for the delay, but other (more urgent) things keep coming up.

I should be able to get to it this week.
Comment 8 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-10-10 13:34:29 PDT
Comment on attachment 551672 [details] [diff] [review]
Patch part.1 Reimplement keycode computation in cocoa widget

>diff --git a/widget/src/cocoa/TextInputHandler.h b/widget/src/cocoa/TextInputHandler.h
>--- a/widget/src/cocoa/TextInputHandler.h
>+++ b/widget/src/cocoa/TextInputHandler.h
>@@ -56,26 +56,29 @@ class nsChildView;
> struct nsTextRange;
> 
> namespace mozilla {
> namespace widget {
> 
> // Key code constants
> enum
> {
>+  kSpaceKeyCode       = 0x31,
>   kEscapeKeyCode      = 0x35,
>+
>   kRCommandKeyCode    = 0x36, // right command key
Why the newline between 0x35 and 0x36? I no reason, please remove it.
I'd guess the space should be between 0x31 and 0x35



> TISInputSourceWrapper::InitByLayoutID(SInt32 aLayoutID,
>                                       PRBool aOverrideKeyboard)
> {
>   // NOTE: Doument new layout IDs in TextInputHandler.h when you add ones.
>   switch (aLayoutID) {
>     case 0:
>       InitByInputSourceID("com.apple.keylayout.US");
>       break;
>-    case -18944:
>+    case 1:
>       InitByInputSourceID("com.apple.keylayout.Greek");
>       break;
>-    case 3:
>+    case 2:
>       InitByInputSourceID("com.apple.keylayout.German");
>       break;
>-    case 224:
>+    case 3:
>       InitByInputSourceID("com.apple.keylayout.Swedish-Pro");
>       break;
>+    case 4:
>+      InitByInputSourceID("com.apple.keylayout.DVORAK-QWERTYCMD");
>+      break;
>+    case 5:
>+      InitByInputSourceID("com.apple.keylayout.Thai");
>+      break;
>     default:
>       Clear();
>       break;
>   }
Could you explain why this change is needed? I mean, why change from the backwards compatible values to something new?


>+  // If Cmd key is pressed, that causes switching keyboard layout temporarily.
>+  // E.g., Dvorak-QWERTY.  Therefore, if Cmd key is pressed, we should honor it.
>+  UInt32 modifiers = aCmdIsPressed ? cmdKey : 0;
>+
>+  PRUint32 charCode = TranslateToChar(aNativeKeyCode, modifiers, aKbType);
>+
>+  // Special case for Mac.  Mac inputs Yen sign (U+00A5) directly instead of
>+  // Back slash (U+005C).  We should return NS_VK_BACK_SLASH for compatibility
>+  // with other platforms.
>+  // XXX How about Won sign (U+20A9) which has same problem as Yen sign?
Please file bugs about XXX comments and add bug number to the code


This is all highly OSX-dependent, so Steven and/or Josh need to look at it carefully.
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-10-10 22:24:19 PDT
(In reply to Olli Pettay [:smaug] from comment #8)
> > TISInputSourceWrapper::InitByLayoutID(SInt32 aLayoutID,
> >                                       PRBool aOverrideKeyboard)
> > {
> >   // NOTE: Doument new layout IDs in TextInputHandler.h when you add ones.
> >   switch (aLayoutID) {
> >     case 0:
> >       InitByInputSourceID("com.apple.keylayout.US");
> >       break;
> >-    case -18944:
> >+    case 1:
> >       InitByInputSourceID("com.apple.keylayout.Greek");
> >       break;
> >-    case 3:
> >+    case 2:
> >       InitByInputSourceID("com.apple.keylayout.German");
> >       break;
> >-    case 224:
> >+    case 3:
> >       InitByInputSourceID("com.apple.keylayout.Swedish-Pro");
> >       break;
> >+    case 4:
> >+      InitByInputSourceID("com.apple.keylayout.DVORAK-QWERTYCMD");
> >+      break;
> >+    case 5:
> >+      InitByInputSourceID("com.apple.keylayout.Thai");
> >+      break;
> >     default:
> >       Clear();
> >       break;
> >   }
> Could you explain why this change is needed? I mean, why change from the
> backwards compatible values to something new?

I think that the change should have been done when we drop legacy keyboard handling code. The random numbers shouldn't use now. That may make confuse some developers who need to add new keyboard layout code for testing.

The API shouldn't be used by chrome and addons. So, I think that this reimplementing change is a good timing to change the code.

# It might be better nsIDOMWindowUtils has consts of keyboard layout IDs.
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-10-11 23:31:35 PDT
Created attachment 566447 [details] [diff] [review]
Patch part.1 Reimplement keycode computation in cocoa widget

Updated for latest trunk (and removed the unnecessary change).
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-10-11 23:32:29 PDT
Created attachment 566448 [details] [diff] [review]
Patch part.2 Add Eisu keycode for Japanese keyboard layout of Mac

Updated for latest trunk.
Comment 12 Steven Michaud [:smichaud] (Retired) 2011-10-12 13:15:52 PDT
Comment on attachment 566447 [details] [diff] [review]
Patch part.1 Reimplement keycode computation in cocoa widget

I've gotten about half-way through this, and what I've seen so far looks fine.  But I'd really like to be able to build it, and it won't apply to current trunk code.

Does it depend on other patches that haven't yet landed?

Is WidgetUtils::ComputeKeyCodeFromChar() in one of those unlanded patches?
Comment 13 Steven Michaud [:smichaud] (Retired) 2011-10-12 13:31:29 PDT
Comment on attachment 566447 [details] [diff] [review]
Patch part.1 Reimplement keycode computation in cocoa widget

Also, most of your additions to test_keycodes.xul go beyond my knowledge of key codes and different kinds of keyboards.  So I'd really like to be able to build this patch and run that test :-)
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-10-12 17:46:44 PDT
(In reply to Steven Michaud from comment #12)
> Does it depend on other patches that haven't yet landed?

Ah, yes. You need to apply bug 630810's patches first. They implemented the utils and new key codes. And you can use following try builds:

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/masayuki@d-toybox.com-5dfc229cc591/
Comment 15 Steven Michaud [:smichaud] (Retired) 2011-10-13 13:33:56 PDT
Comment on attachment 566447 [details] [diff] [review]
Patch part.1 Reimplement keycode computation in cocoa widget

This looks fine to me.  And I've run your patched test_keycodes.xul
several times -- I didn't see any failures, either when run alone or
with the other widget tests.

I do have one question and a nit.

I noticed that, unlike the
TextInputHandlerBase::ComputeGeckoKeyCodeFromChar() method it
replaced, WidgetUtils::ComputeKeyCodeFromChar() returns '0' for "\n".
Did you intend this?  Is it correct?

"Devorak Qwerty" should be "Dvorak Qwerty" :-)
Comment 16 Steven Michaud [:smichaud] (Retired) 2011-10-13 13:35:04 PDT
Comment on attachment 566448 [details] [diff] [review]
Patch part.2 Add Eisu keycode for Japanese keyboard layout of Mac

I don't have a Japanese Mac keyboard to test this with, but it looks fine.
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-10-13 19:19:05 PDT
(In reply to Steven Michaud from comment #15)
> I noticed that, unlike the
> TextInputHandlerBase::ComputeGeckoKeyCodeFromChar() method it
> replaced, WidgetUtils::ComputeKeyCodeFromChar() returns '0' for "\n".
> Did you intend this?  Is it correct?

Yes, the Enter or Return should be resolved by the keycode. The method is needed only for a printable character inputting key actually.

> "Devorak Qwerty" should be "Dvorak Qwerty" :-)

Oops, thanks!
Comment 18 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-10-13 19:20:27 PDT
FYI:

I'll work on GTK2 part next week, all of the related patches are going to be landed same time.
Comment 19 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-10-13 19:36:40 PDT
(In reply to Olli Pettay [:smaug] from comment #8)
> >+  UInt32 modifiers = aCmdIsPressed ? cmdKey : 0;
> >+
> >+  PRUint32 charCode = TranslateToChar(aNativeKeyCode, modifiers, aKbType);
> >+
> >+  // Special case for Mac.  Mac inputs Yen sign (U+00A5) directly instead of
> >+  // Back slash (U+005C).  We should return NS_VK_BACK_SLASH for compatibility
> >+  // with other platforms.
> >+  // XXX How about Won sign (U+20A9) which has same problem as Yen sign?
> Please file bugs about XXX comments and add bug number to the code

Hmm, I checked all Korean keyboard layouts which are installed in MacOS X in default settings. Only 2-Set Korean can input the Won sign, but it's shifted character. So, it's not related to this issue.

But according to Wikipedia, there are some keyboard layouts (maybe for Win/Linux):
http://en.wikipedia.org/wiki/Keyboard_layout#Hangul_.28for_Korean.29

I guess someone can install the layout manually. So, I think that we still need to do it in another bug. I'll file the bug.
Comment 20 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-14 20:29:30 PDT
Created attachment 606100 [details] [diff] [review]
part.1 Reimplement keycode computation in cocoa widget

updated for latest trunk.
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-14 20:29:55 PDT
Created attachment 606101 [details] [diff] [review]
part.2 Add Eisu keycode for Japanese keyboard layout of Mac
Comment 22 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-30 00:46:10 PDT
Created attachment 610830 [details] [diff] [review]
part.1 Reimplement keycode computation in cocoa widget

updating for latest trunk.
Comment 23 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-05-07 20:24:54 PDT
Created attachment 621851 [details] [diff] [review]
part.1 Reimplement keycode computation in cocoa widget

Updated for latest trunk.
Comment 26 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-05-17 15:52:08 PDT
*** Bug 756096 has been marked as a duplicate of this bug. ***
Comment 27 Stanislav 2013-07-01 00:20:10 PDT
I have an error in determining the keyCode for tilda (~) in Cyrillic keyboard layout, keyCode = 0.. Firefox 22
Comment 28 Greg K. 2013-10-11 03:24:30 PDT
(In reply to Stanislav from comment #27)
> I have an error in determining the keyCode for tilda (~) in Cyrillic
> keyboard layout, keyCode = 0.. Firefox 22

Stanislav, please file a new bug for that.

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