677252 - Reimplement keycode computation in cocoa widget

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

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

Attachment: 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)]

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] (he/him)(JST, +0900)]

Attachment: Patch part.1 Reimplement keycode computation in cocoa widget

Comment 3

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

Attachment: Patch part.2 Add Eisu keycode for Japanese keyboard layout of Mac

Comment 4

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

Attachment: Patch part.1 Reimplement keycode computation in cocoa widget

fix a mistake, sorry for the spam.

Comment 5

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

Attachment: Patch part.2 Add Eisu keycode for Japanese keyboard layout of Mac

Comment 6

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

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)]

> 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][[e-mail]](vacation until July 14)]

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] (he/him)(JST, +0900)]

(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] (he/him)(JST, +0900)]

Attachment: Patch part.1 Reimplement keycode computation in cocoa widget

Updated for latest trunk (and removed the unnecessary change).

Comment 11

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

Attachment: Patch part.2 Add Eisu keycode for Japanese keyboard layout of Mac

Updated for latest trunk.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

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)]

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] (he/him)(JST, +0900)]

(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/[e-mail]-5dfc229cc591/

Comment 15

[Steven Michaud [:smichaud] (Retired)]

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)]

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] (he/him)(JST, +0900)]

(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] (he/him)(JST, +0900)]

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] (he/him)(JST, +0900)]

(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] (he/him)(JST, +0900)]

Attachment: part.1 Reimplement keycode computation in cocoa widget

updated for latest trunk.

Comment 21

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

Attachment: part.2 Add Eisu keycode for Japanese keyboard layout of Mac

Comment 22

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

Attachment: part.1 Reimplement keycode computation in cocoa widget

updating for latest trunk.

Comment 23

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

Attachment: part.1 Reimplement keycode computation in cocoa widget

Updated for latest trunk.

Comment 24

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/29e512736f30
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c5211cd5a39

Comment 25

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/mozilla-central/rev/29e512736f30
http://hg.mozilla.org/mozilla-central/rev/5c5211cd5a39

Comment 27

[Stanislav]

I have an error in determining the keyCode for tilda (~) in Cyrillic keyboard layout, keyCode = 0.. Firefox 22

Comment 28

[Greg K.]

(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.