The default bug view has changed. See this FAQ.

Reimplement keycode computation in cocoa widget

RESOLVED FIXED in mozilla15

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Depends on: 1 bug, {intl})

Trunk
mozilla15
All
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 9 obsolete attachments)

4.08 KB, patch
Details | Diff | Splinter Review
110.35 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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.
Attachment #551463 - Flags: superreview?(Olli.Pettay)
Attachment #551463 - Flags: review?(smichaud)
(Assignee)

Updated

6 years ago
Summary: Refactor keycode computation in cocoa widget → Reimplement keycode computation in cocoa widget
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).
(Assignee)

Comment 2

6 years ago
Created attachment 551665 [details] [diff] [review]
Patch part.1 Reimplement keycode computation in cocoa widget
Attachment #551463 - Attachment is obsolete: true
Attachment #551463 - Flags: superreview?(Olli.Pettay)
Attachment #551463 - Flags: review?(smichaud)
Attachment #551665 - Flags: review?(smichaud)
Attachment #551665 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 3

6 years ago
Created attachment 551666 [details] [diff] [review]
Patch part.2 Add Eisu keycode for Japanese keyboard layout of Mac
Attachment #551666 - Flags: superreview?(Olli.Pettay)
Attachment #551666 - Flags: review?(smichaud)
(Assignee)

Comment 4

6 years ago
Created attachment 551672 [details] [diff] [review]
Patch part.1 Reimplement keycode computation in cocoa widget

fix a mistake, sorry for the spam.
Attachment #551665 - Attachment is obsolete: true
Attachment #551665 - Flags: review?(smichaud)
Attachment #551665 - Flags: review?(Olli.Pettay)
Attachment #551672 - Flags: review?(smichaud)
Attachment #551672 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 5

6 years ago
Created attachment 551673 [details] [diff] [review]
Patch part.2 Add Eisu keycode for Japanese keyboard layout of Mac
Attachment #551666 - Attachment is obsolete: true
Attachment #551666 - Flags: superreview?(Olli.Pettay)
Attachment #551666 - Flags: review?(smichaud)
Attachment #551673 - Flags: superreview?(Olli.Pettay)
Attachment #551673 - Flags: review?(smichaud)
smichaud, are you planning to review this?

(I'll try to review mainly non-OSX-widget parts today or tomorrow)
> 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.
Attachment #551673 - Flags: superreview?(Olli.Pettay) → superreview+
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.
Attachment #551672 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 9

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

Comment 10

6 years ago
Created attachment 566447 [details] [diff] [review]
Patch part.1 Reimplement keycode computation in cocoa widget

Updated for latest trunk (and removed the unnecessary change).
Attachment #551672 - Attachment is obsolete: true
Attachment #551672 - Flags: review?(smichaud)
Attachment #566447 - Flags: review?(smichaud)
(Assignee)

Comment 11

6 years ago
Created attachment 566448 [details] [diff] [review]
Patch part.2 Add Eisu keycode for Japanese keyboard layout of Mac

Updated for latest trunk.
Attachment #551673 - Attachment is obsolete: true
Attachment #551673 - Flags: review?(smichaud)
Attachment #566448 - Flags: review?(smichaud)
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 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 :-)
(Assignee)

Comment 14

6 years ago
(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 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" :-)
Attachment #566447 - Flags: review?(smichaud) → review+
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.
Attachment #566448 - Flags: review?(smichaud) → review+
(Assignee)

Comment 17

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

Comment 18

6 years ago
FYI:

I'll work on GTK2 part next week, all of the related patches are going to be landed same time.
(Assignee)

Comment 19

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

Updated

6 years ago
Depends on: 694501
(Assignee)

Comment 20

5 years ago
Created attachment 606100 [details] [diff] [review]
part.1 Reimplement keycode computation in cocoa widget

updated for latest trunk.
Attachment #566447 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
Created attachment 606101 [details] [diff] [review]
part.2 Add Eisu keycode for Japanese keyboard layout of Mac
Attachment #566448 - Attachment is obsolete: true
(Assignee)

Comment 22

5 years ago
Created attachment 610830 [details] [diff] [review]
part.1 Reimplement keycode computation in cocoa widget

updating for latest trunk.
Attachment #606100 - Attachment is obsolete: true
(Assignee)

Comment 23

5 years ago
Created attachment 621851 [details] [diff] [review]
part.1 Reimplement keycode computation in cocoa widget

Updated for latest trunk.
Attachment #610830 - Attachment is obsolete: true
(Assignee)

Comment 24

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/29e512736f30
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c5211cd5a39
Target Milestone: --- → mozilla15
http://hg.mozilla.org/mozilla-central/rev/29e512736f30
http://hg.mozilla.org/mozilla-central/rev/5c5211cd5a39
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Duplicate of this bug: 756096
(Assignee)

Updated

5 years ago
Blocks: 603697
(Assignee)

Updated

4 years ago
Blocks: 412627
(Assignee)

Updated

4 years ago
Blocks: 413875

Comment 27

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

Comment 28

4 years ago
(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.
You need to log in before you can comment on or make changes to this bug.