Bug 1464016 - TextInputHandler should not clear alt/ctrl/meta modifiers of eKeyPress event before sending TextEventDispatcher r=m_kato

If a key combination causes text input, we need to dispatch keypress
events without alt/ctrl/meta modifiers since TextEditor won't handle
keyepress events whose altKey, ctrlKey or metaKey is true as inputting
text.

Currently, TextEventDispatcher sets mCharCode of eKeyPress event from
mKeyValue.  Then, when altKey, ctrlKey or metaKey is true, it'll call
WillDispatchKeyboardEvent() and then, TextInputHandler needs to reset
the charCode value from native event information.

However, the problem is, TextInputHandler::InsertText() is called
with control character when control key is pressed and InsertText()
clears the modifier information before sending eKeyPress event to
TextEvenDispatcher so that TextEventDispatcher won't call
WillDispatchKeyboardEvent() even though control key is actually
pressed.  Therefore, TextInputHandler cannot adjust charCode value
and modifier flags in some cases such as control + option + 'a'.

This patch makes InsertText() stop clearing the modifiers and
makes WillDispatchKeyboardEvent() do it instead.  This procedure
is expected by TextEventDispatcher.

MozReview-Commit-ID: Ig6qgRBeQDh

--HG--
extra : rebase_source : 446e8af0e921946f3409d26ede70446248317673

widget/TextEvents.h

@@ -234,7 +234,7 @@ public:
   // that key normally produces a character value".
   // <https://www.w3.org/TR/uievents/#event-type-keypress>
   // Additionally, for backward compatiblity with all existing browsers,
-  // there is an spec issue for Enter key press.
+  // there is a spec issue for Enter key press.
   // <https://github.com/w3c/uievents/issues/183>
   bool IsInputtingText() const
   {
@@ -247,23 +247,25 @@ public:
     //       MODIFIER_ALT and MODIFIER_CONTROL of eKeyPress event if it
     //       should be treated as inputting a character because AltGr is
     //       represented with both Alt key and Ctrl key are pressed, and
-    //       some keyboard layout may produces a character with Ctrl key.
+    //       some keyboard layouts may produces a character with Ctrl key.
     //       -- On Linux, KeymapWrapper doesn't have this hack since perhaps,
     //       we don't have any bug reports that user cannot input proper
     //       character with Alt and/or Ctrl key.
-    //       -- On macOS, TextInputHandler::InsertText() clears MODIFIER_ALT
-    //       and MDOFIEIR_CONTROL of eKeyPress event.  However, this method
-    //       is called only when an editor has focus (even if IME is disabled
-    //       in password field or by |ime-mode: disabled;|) because it's
-    //       called while TextInputHandler::HandleKeyDownEvent() calls
-    //       interpretKeyEvents: to notify text input processor of Cocoa
-    //       (including IME).  In other words, when we need to disable IME
-    //       completey when no editor has focus, we cannot call
-    //       interpretKeyEvents:.  So, TextInputHandler::InsertText() won't
-    //       be called when no editor has focus so that neither MODIFIER_ALT
-    //       nor MODIFIER_CONTROL is cleared.  So, fortunately, altKey and
-    //       ctrlKey values of "keypress" events are same as the other browsers
-    //       only when no editor has focus.
+    //       -- On macOS, IMEInputHandler::WillDispatchKeyboardEvent() clears
+    //       MODIFIER_ALT and MDOFIEIR_CONTROL of eKeyPress event only when
+    //       TextInputHandler::InsertText() has been called for the event.
+    //       I.e., they are cleared only when an editor has focus (even if IME
+    //       is disabled in password field or by |ime-mode: disabled;|) because
+    //       TextInputHandler::InsertText() is called while
+    //       TextInputHandler::HandleKeyDownEvent() calls interpretKeyEvents:
+    //       to notify text input processor of Cocoa (including IME).  In other
+    //       words, when we need to disable IME completey when no editor has
+    //       focus, we cannot call interpretKeyEvents:.  So,
+    //       TextInputHandler::InsertText() won't be called when no editor has
+    //       focus so that neither MODIFIER_ALT nor MODIFIER_CONTROL is
+    //       cleared.  So, fortunately, altKey and ctrlKey values of "keypress"
+    //       events are same as the other browsers only when no editor has
+    //       focus.
     // NOTE: As mentioned above, for compatibility with the other browsers on
     //       macOS, we should keep MODIFIER_ALT and MODIFIER_CONTROL flags of
     //       eKeyPress events when no editor has focus.  However, Alt key,

widget/cocoa/TextInputHandler.h

@@ -1088,6 +1088,10 @@ public:
   bool IsIMEOpened();
   bool IsIMEEnabled() { return mIsIMEEnabled; }
   bool IsASCIICapableOnly() { return mIsASCIICapableOnly; }
+  bool IsEditableContent() const
+  {
+    return mIsIMEEnabled || mIsASCIICapableOnly;
+  }
   bool IgnoreIMECommit() { return mIgnoreIMECommit; }
 
   void CommitIMEComposition();

widget/cocoa/TextInputHandler.mm

@@ -175,17 +175,14 @@ GetKeyNameForNativeKeyCode(unsigned short aNativeKeyCode)
 }
 
 static const char*
-GetCharacters(const NSString* aString)
+GetCharacters(const nsAString& aString)
 {
-  nsAutoString str;
-  nsCocoaUtils::GetStringForNSString(aString, str);
-  if (str.IsEmpty()) {
+  if (aString.IsEmpty()) {
     return "";
   }
-
   nsAutoString escapedStr;
-  for (uint32_t i = 0; i < str.Length(); i++) {
-    char16_t ch = str[i];
+  for (uint32_t i = 0; i < aString.Length(); i++) {
+    char16_t ch = aString.CharAt(i);
     if (ch < 0x20) {
       nsPrintfCString utf8str("(U+%04X)", ch);
       escapedStr += NS_ConvertUTF8toUTF16(utf8str);
@@ -203,6 +200,14 @@ GetCharacters(const NSString* aString)
   return [result UTF8String];
 }
 
+static const char*
+GetCharacters(const NSString* aString)
+{
+  nsAutoString str;
+  nsCocoaUtils::GetStringForNSString(aString, str);
+  return GetCharacters(str);
+}
+
 static const char*
 GetCharacters(const CFStringRef aString)
 {
@@ -1202,10 +1207,13 @@ TISInputSourceWrapper::WillDispatchKeyboardEvent(
     char16_t uniChar = static_cast<char16_t>(aKeyEvent.mCharCode);
     MOZ_LOG(gLog, LogLevel::Info,
       ("%p TISInputSourceWrapper::WillDispatchKeyboardEvent, "
-       "aNativeKeyEvent=%p, [aNativeKeyEvent characters]=\"%s\", "
+       "aNativeKeyEvent=%p, aInsertString=%p (\"%s\"), "
+       "[aNativeKeyEvent characters]=\"%s\", "
        "aKeyEvent={ mMessage=%s, mCharCode=0x%X(%s) }, kbType=0x%X, "
        "IsOpenedIMEMode()=%s",
-       this, aNativeKeyEvent, utf8Chars.get(),
+       this, aNativeKeyEvent, aInsertString,
+       aInsertString ? GetCharacters(*aInsertString) : "",
+       GetCharacters([aNativeKeyEvent characters]),
        GetGeckoKeyEventType(aKeyEvent), aKeyEvent.mCharCode,
        uniChar ? NS_ConvertUTF16toUTF8(&uniChar, 1).get() : "",
        static_cast<unsigned int>(kbType), TrueOrFalse(IsOpenedIMEMode())));
@@ -1228,6 +1236,14 @@ TISInputSourceWrapper::WillDispatchKeyboardEvent(
      "aKeyEvent.mKeyCode=0x%X, aKeyEvent.mCharCode=0x%X",
      this, aKeyEvent.mKeyCode, aKeyEvent.mCharCode));
 
+  // If aInsertString is not nullptr (it means InsertText() is called)
+  // and it acutally inputs a character, we don't need to append alternative
+  // charCode values since such keyboard event shouldn't be handled as
+  // a shortcut key.
+  if (aInsertString && charCode) {
+    return;
+  }
+
   TISInputSourceWrapper USLayout("com.apple.keylayout.US");
   bool isRomanKeyboardLayout = IsASCIICapable();
 
@@ -2439,6 +2455,11 @@ TextInputHandler::InsertText(NSAttributedString* aAttrString,
     return;
   }
 
+  MOZ_LOG(gLog, LogLevel::Info,
+    ("%p IMEInputHandler::InsertText, "
+     "maybe dispatches eKeyPress event without control, alt and meta modifiers",
+     this));
+
   // Dispatch keypress event with char instead of compositionchange event
   WidgetKeyboardEvent keypressEvent(true, eKeyPress, widget);
   // XXX Why do we need to dispatch keypress event for not inputting any
@@ -2460,12 +2481,6 @@ TextInputHandler::InsertText(NSAttributedString* aAttrString,
     //      keypress events even if they don't cause inputting non-empty string.
   }
 
-  // Remove basic modifiers from keypress event because if they are included,
-  // nsPlaintextEditor ignores the event.
-  keypressEvent.mModifiers &= ~(MODIFIER_CONTROL |
-                                MODIFIER_ALT |
-                                MODIFIER_META);
-
   // TODO:
   // If mCurrentKeyEvent.mKeyEvent is null, the text should be inputted as
   // composition events.
@@ -3254,10 +3269,23 @@ IMEInputHandler::WillDispatchKeyboardEvent(
     TISInputSourceWrapper tis;
     tis.InitByLayoutID(KeyboardLayoutOverrideRef().mKeyboardLayout, true);
     tis.WillDispatchKeyboardEvent(nativeEvent, insertString, aKeyboardEvent);
-    return;
+  } else {
+    TISInputSourceWrapper::CurrentInputSource().
+      WillDispatchKeyboardEvent(nativeEvent, insertString, aKeyboardEvent);
+  }
+
+  // Remove basic modifiers from keypress event because if they are included
+  // but this causes inputting text, since TextEditor won't handle eKeyPress
+  // events whose ctrlKey, altKey or metaKey is true as text input.
+  // Note that this hack should be used only when an editor has focus because
+  // this is a hack for TextEditor and modifier key information may be
+  // important for current web app.
+  if (IsEditableContent() && insertString &&
+      aKeyboardEvent.mMessage == eKeyPress && aKeyboardEvent.mCharCode) {
+    aKeyboardEvent.mModifiers &= ~(MODIFIER_CONTROL |
+                                   MODIFIER_ALT |
+                                   MODIFIER_META);
   }
-  TISInputSourceWrapper::CurrentInputSource().
-    WillDispatchKeyboardEvent(nativeEvent, insertString, aKeyboardEvent);
 }
 
 void

widget/cocoa/nsChildView.mm

@@ -1808,6 +1808,10 @@ nsChildView::SetInputContext(const InputContext& aContext,
     }
   }
 
+  // IMEInputHandler::IsEditableContent() returns false when both
+  // IsASCIICableOnly() and IsIMEEnabled() return false.  So, be careful
+  // when you change the following code.  You might need to change
+  // IMEInputHandler::IsEditableContent() too.
   mInputContext = aContext;
   switch (aContext.mIMEState.mEnabled) {
     case IMEState::ENABLED:

widget/tests/test_keypress_event_with_alt_on_mac.html

@@ -90,15 +90,8 @@ async function runTests()
        kDescription + "'a' key press with control key should not cause firing keypress event");
 
     keypress = await testNativeKey(KEYBOARD_LAYOUT_EN_US, MAC_VK_ANSI_A, {altKey: true, ctrlKey: true}, "\u0001", "a");
-    if (kTest.isEditable) {
-      // XXX Currently, control + option + a inputs \u00E5 if IME is available,
-      //     but this must be a bug of our widget for Cocoa.
-      todo(!keypress,
-           kDescription + "'a' key press with option key and control key should not cause firing keypress event");
-    } else {
-      ok(!keypress,
-         kDescription + "'a' key press with option key and control key should not cause firing keypress event");
-    }
+    ok(!keypress,
+       kDescription + "'a' key press with option key and control key should not cause firing keypress event");
 
     // XXX Cannot test with command key for now since keyup event won't be fired due to macOS's limitation.