775414 - InitKeyEvent() should decide input string instead of InitKeyPressEvent()

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

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

Attachment: Patch

In bug 758420, we used a hecky way, that is, setting charCode before calling InitKeyEvent() if the caller (InsertText()) wants to override NSEvent's characters value with specified value.

When I fix bug 680830, the inputting characters are also set to char attribute of D3E even if the key event is NS_KEY_DOWN or NS_KEY_UP.

Therefore, it makes the patch simpler if I move the code deciding inputting character from InitKeyPressEvent() to InitKeyEvent().

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 643731 [details] [diff] [review]
Patch

This basically looks fine to me.

But it does change the logic in the following case:

if (aKeyEvent.IsMeta() && !(aKeyEvent.IsControl() || aKeyEvent.IsAlt()))

The changes look reasonable, but I can't tell whether they're correct or not.

Comment 2

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

(In reply to Steven Michaud from comment #1)
> But it does change the logic in the following case:
> 
> if (aKeyEvent.IsMeta() && !(aKeyEvent.IsControl() || aKeyEvent.IsAlt()))
> 
> The changes look reasonable, but I can't tell whether they're correct or not.

Oh, really? I didn't expect to change anything. I just ported some parts from InitKeyPressEvent(). Would you explain what's changed?

Comment 3

[Steven Michaud [:smichaud] (Retired)]

This code in TISInputSourceWrapper::InitKeyPressEvent():

-  if (computeCharCode &&
-      aKeyEvent.IsMeta() && !(aKeyEvent.IsControl() || aKeyEvent.IsAlt())) {
-    // The character to use for charCode.
-    PRUint32 preferredCharCode = 0;
-    preferredCharCode = aKeyEvent.IsShift() ? cmdedShiftChar : cmdedChar;
-
-    if (preferredCharCode) {
-      aKeyEvent.charCode = preferredCharCode;
-      PR_LOG(gLog, PR_LOG_ALWAYS,
-        ("%p TISInputSourceWrapper::InitKeyPressEvent, "
-         "aKeyEvent.charCode=U+%X",
-         this, aKeyEvent.charCode));
-    }
-  }

was replaced by this code in TISInputSourceWrapper::InitKeyEvent():

+    else if (aKeyEvent.IsMeta() &&
+             !(aKeyEvent.IsControl() || aKeyEvent.IsAlt())) {
+      UInt32 numLockState =
+        aKeyEvent.IsNumLocked() ? kEventKeyModifierNumLockMask : 0;
+      UInt32 capsLockState = aKeyEvent.IsCapsLocked() ? alphaLock : 0;
+      UInt32 shiftState = aKeyEvent.IsShift() ? shiftKey : 0;
+      PRUint32 uncmdedChar =
+        TranslateToChar(nativeKeyCode, numLockState, kbType);
+      PRUint32 cmdedChar =
+        TranslateToChar(nativeKeyCode, cmdKey | numLockState, kbType);
+      // If we can make a good guess at the characters that the user would
+      // expect this key combination to produce (with and without Shift) then
+      // use those characters.  This also corrects for CapsLock.
+      PRUint32 ch = 0;
+      if (uncmdedChar == cmdedChar) {
+        // The characters produced with Command seem similar to those without
+        // Command.
+        ch = TranslateToChar(nativeKeyCode,
+                             shiftState | capsLockState | numLockState, kbType);
+      } else {
+        TISInputSourceWrapper USLayout("com.apple.keylayout.US");
+        PRUint32 uncmdedUSChar =
+          USLayout.TranslateToChar(nativeKeyCode, numLockState, kbType);
+        // If it looks like characters from US keyboard layout when Command key
+        // is pressed, we should compute a character in the layout.
+        if (uncmdedUSChar == cmdedChar) {
+          ch = USLayout.TranslateToChar(nativeKeyCode,
+                          shiftState | capsLockState | numLockState, kbType);
+        }
+      }
+

They look different to me :-)

But maybe I just don't fully understand the context of your changes.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

I looked again and found this block of code in TISInputSourceWrapper::InitKeyPressEvent():

http://hg.mozilla.org/mozilla-central/annotate/eea94a9b40a1/widget/cocoa/TextInputHandler.mm#l920

I'm no longer sure the logic has actually changed.  But neither can I say it *hasn't* changed.

Comment 5

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

Ah, I see. I ported the each variable's initialization too. Ideally, these parts should be shared by making another method. However, I couldn't find reasonable way for that. But I think that it's very risky to change the key hell part, so, I believe we will never touch the part again. Therefore, I just picked up the part for InitKeyEvent().

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/440ac3414c64

Thank you.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/440ac3414c64