Bug 1464016 - TextInputHandler should not clear alt/ctrl/meta modifiers of eKeyPress event before sending TextEventDispatcher r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 24 May 2018 20:59:48 +0900
changeset 474572 2415a1d53df35624a109643533f1f3d8d8e0ab15
parent 474571 d67e33e20cbd0603db8e9a71b24756567883b36e
child 474573 c094fc31b95f079c5cec2f3e88cb9fec0bdd7ca5
push id9374
push userjlund@mozilla.com
push dateMon, 18 Jun 2018 21:43:20 +0000
treeherdermozilla-beta@160e085dfb0b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1464016
milestone62.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
widget/TextEvents.h
widget/cocoa/TextInputHandler.h
widget/cocoa/TextInputHandler.mm
widget/cocoa/nsChildView.mm
widget/tests/test_keypress_event_with_alt_on_mac.html
--- a/widget/TextEvents.h
+++ b/widget/TextEvents.h
@@ -229,46 +229,48 @@ public:
   }
 
   // IsInputtingText() and IsInputtingLineBreak() are used to check if
   // it should cause eKeyPress events even on web content.
   // UI Events defines that "keypress" event should be fired "if and only if
   // 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
   {
     // NOTE: On some keyboard layout, some characters are inputted with Control
     //       key or Alt key, but at that time, widget clears the modifier flag
     //       from eKeyPress event because our TextEditor won't handle eKeyPress
     //       events as inputting text (bug 1346832).
     // NOTE: There are some complicated issues of our traditional behavior.
     //       -- On Windows, KeyboardLayout::WillDispatchKeyboardEvent() clears
     //       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,
     //       labeled "option" on keyboard for Mac, is AltGraph key on the other
     //       platforms.  So, even if MODIFIER_ALT is set, we need to dispatch
     //       eKeyPress event even on web content unless mCharCode is 0.
     //       Therefore, we need to ignore MODIFIER_ALT flag here only on macOS.
     return mMessage == eKeyPress &&
--- a/widget/cocoa/TextInputHandler.h
+++ b/widget/cocoa/TextInputHandler.h
@@ -1083,16 +1083,20 @@ public:
   bool HasMarkedText();
   NSRange MarkedRange();
 
   bool IsIMEComposing() { return mIsIMEComposing; }
   bool IsDeadKeyComposing() { return mIsDeadKeyComposing; }
   bool IsIMEOpened();
   bool IsIMEEnabled() { return mIsIMEEnabled; }
   bool IsASCIICapableOnly() { return mIsASCIICapableOnly; }
+  bool IsEditableContent() const
+  {
+    return mIsIMEEnabled || mIsASCIICapableOnly;
+  }
   bool IgnoreIMECommit() { return mIgnoreIMECommit; }
 
   void CommitIMEComposition();
   void CancelIMEComposition();
 
   void EnableIME(bool aEnableIME);
   void SetIMEOpenState(bool aOpen);
   void SetASCIICapableOnly(bool aASCIICapableOnly);
--- a/widget/cocoa/TextInputHandler.mm
+++ b/widget/cocoa/TextInputHandler.mm
@@ -170,27 +170,24 @@ GetKeyNameForNativeKeyCode(unsigned shor
     case kVK_ANSI_Period:         return "Period";
     case kVK_ANSI_Grave:          return "Grave";
 
     default:                      return "undefined";
   }
 }
 
 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);
     } else if (ch <= 0x7E) {
       escapedStr += ch;
     } else {
       nsPrintfCString utf8str("(U+%04X)", ch);
       escapedStr += ch;
@@ -199,16 +196,24 @@ GetCharacters(const NSString* aString)
   }
 
   // the result will be freed automatically by cocoa.
   NSString* result = nsCocoaUtils::ToNSString(escapedStr);
   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)
 {
   const NSString* str = reinterpret_cast<const NSString*>(aString);
   return GetCharacters(str);
 }
 
 static const char*
 GetNativeKeyEventType(NSEvent* aNativeEvent)
@@ -1197,20 +1202,23 @@ TISInputSourceWrapper::WillDispatchKeybo
 
   if (MOZ_LOG_TEST(gLog, LogLevel::Info)) {
     nsAutoString chars;
     nsCocoaUtils::GetStringForNSString([aNativeKeyEvent characters], chars);
     NS_ConvertUTF16toUTF8 utf8Chars(chars);
     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())));
   }
 
   nsAutoString insertStringForCharCode;
   ComputeInsertStringForCharCode(aNativeKeyEvent, aKeyEvent, aInsertString,
                                  insertStringForCharCode);
@@ -1223,16 +1231,24 @@ TISInputSourceWrapper::WillDispatchKeybo
     insertStringForCharCode.IsEmpty() ? 0 : insertStringForCharCode[0];
   aKeyEvent.SetCharCode(charCode);
 
   MOZ_LOG(gLog, LogLevel::Info,
     ("%p 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();
 
   UInt32 key = [aNativeKeyEvent keyCode];
 
   // Caps lock and num lock modifier state:
   UInt32 lockState = 0;
   if ([aNativeKeyEvent modifierFlags] & NSAlphaShiftKeyMask) {
@@ -2434,16 +2450,21 @@ TextInputHandler::InsertText(NSAttribute
   nsresult rv = mDispatcher->BeginNativeInputTransaction();
   if (NS_WARN_IF(NS_FAILED(rv))) {
     MOZ_LOG(gLog, LogLevel::Error,
       ("%p IMEInputHandler::InsertText, "
        "FAILED, due to BeginNativeInputTransaction() failure", this));
     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
   //     string?  If it wants to delete the specified range, should we
   //     dispatch an eContentCommandDelete event instead?  Because this
   //     must not be caused by a key operation, a part of IME's processing.
 
   // Don't set other modifiers from the current event, because here in
@@ -2455,22 +2476,16 @@ TextInputHandler::InsertText(NSAttribute
   } else {
     nsCocoaUtils::InitInputEvent(keypressEvent, static_cast<NSEvent*>(nullptr));
     keypressEvent.mKeyNameIndex = KEY_NAME_INDEX_USE_STRING;
     keypressEvent.mKeyValue = str;
     // FYI: TextEventDispatcher will set mKeyCode to 0 for printable key's
     //      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.
   nsEventStatus status = nsEventStatus_eIgnore;
   bool keyPressDispatched =
     mDispatcher->MaybeDispatchKeypressEvents(keypressEvent, status,
                                              currentKeyEvent);
   bool keyPressHandled = (status == nsEventStatus_eConsumeNoDefault);
@@ -3249,20 +3264,33 @@ IMEInputHandler::WillDispatchKeyboardEve
     //     EventStateManager::PreHandleEvent() will reset the flags if
     //     the event target isn't in remote process.
     aKeyboardEvent.MarkAsWaitingReplyFromRemoteProcess();
   }
   if (KeyboardLayoutOverrideRef().mOverrideEnabled) {
     TISInputSourceWrapper tis;
     tis.InitByLayoutID(KeyboardLayoutOverrideRef().mKeyboardLayout, true);
     tis.WillDispatchKeyboardEvent(nativeEvent, insertString, aKeyboardEvent);
-    return;
-  }
-  TISInputSourceWrapper::CurrentInputSource().
-    WillDispatchKeyboardEvent(nativeEvent, insertString, aKeyboardEvent);
+  } 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);
+  }
 }
 
 void
 IMEInputHandler::NotifyIMEOfFocusChangeInGecko()
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
 
   MOZ_LOG(gLog, LogLevel::Info,
--- a/widget/cocoa/nsChildView.mm
+++ b/widget/cocoa/nsChildView.mm
@@ -1803,16 +1803,20 @@ nsChildView::SetInputContext(const Input
   if (mTextInputHandler->IsFocused()) {
     if (aContext.IsPasswordEditor()) {
       TextInputHandler::EnableSecureEventInput();
     } else {
       TextInputHandler::EnsureSecureEventInputDisabled();
     }
   }
 
+  // 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:
     case IMEState::PLUGIN:
       mTextInputHandler->SetASCIICapableOnly(false);
       mTextInputHandler->EnableIME(true);
       if (mInputContext.mIMEState.mOpen != IMEState::DONT_CHANGE_OPEN_STATE) {
         mTextInputHandler->SetIMEOpenState(
--- a/widget/tests/test_keypress_event_with_alt_on_mac.html
+++ b/widget/tests/test_keypress_event_with_alt_on_mac.html
@@ -85,25 +85,18 @@ async function runTests()
     ok(keypress.shiftKey,
        kDescription + "shiftKey of 'a' key press with option key and shift key should be true");
 
     keypress = await testNativeKey(KEYBOARD_LAYOUT_EN_US, MAC_VK_ANSI_A, {ctrlKey: true}, "\u0001", "a");
     ok(!keypress,
        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.
 
     // Some keys of Arabic - PC keyboard layout do not input any character with option key.
     // In such case, we shouldn't dispatch keypress event.
     keypress = await testNativeKey(KEYBOARD_LAYOUT_ARABIC_PC, MAC_VK_ANSI_7, {altKey: true}, "", "\u0667");
     ok(!keypress,
        kDescription + "'7' key press with option key should not cause firing keypress event");