Bug 1310565 TextInputHandler shouldn't dispatch a composition events when a key press causes 2 or more characters r=m_kato, a=gchang
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 24 Nov 2016 14:08:15 +0900
changeset 358945 2d052cc5f70016e56e41cdc741573e5c967e9aed
parent 358944 482eefb7b7a75d450af08c4537ff17515fd8871c
child 358946 de40de49d38983e9bc745f305f509acb92876e12
push id1324
push usermtabara@mozilla.com
push dateMon, 16 Jan 2017 13:07:44 +0000
treeherdermozilla-release@a01c49833940 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato, gchang
bugs1310565
milestone51.0
Bug 1310565 TextInputHandler shouldn't dispatch a composition events when a key press causes 2 or more characters r=m_kato, a=gchang TextInputHandler::InsertText() dispatches a set of composition events when a key press causes 2 or more characters (Note that InsertText() is typically called only when IME is available because it's called via [NSResponder interpretKeyEvents]). However, this is different from the behavior of Windows. On Windows, NativeKey dispatches two ore more eKeyPress events in this case.
widget/cocoa/TextInputHandler.mm
widget/tests/test_keycodes.xul
--- a/widget/cocoa/TextInputHandler.mm
+++ b/widget/cocoa/TextInputHandler.mm
@@ -2183,26 +2183,20 @@ TextInputHandler::InsertText(NSAttribute
     WidgetContentCommandEvent deleteCommandEvent(true, eContentCommandDelete,
                                                  mWidget);
     DispatchEvent(deleteCommandEvent);
     NS_ENSURE_TRUE_VOID(deleteCommandEvent.mSucceeded);
     // Be aware! The widget might be destroyed here.
     return;
   }
 
-  if (str.Length() != 1 || IsIMEComposing()) {
+  // If this is not caused by pressing a key or there is a composition, let's
+  // insert the text as committing a composition.
+  if (!currentKeyEvent || IsIMEComposing()) {
     InsertTextAsCommittingComposition(aAttrString, aReplacementRange);
-    // For now, consume keypress events when we dispatch the string with a
-    // composition for preventing to dispatch keypress events later.
-    // TODO: When there is a currentKeyEvent, we should dispatch keypress
-    //       events even if the length of the string is over 1.
-    if (currentKeyEvent) {
-      currentKeyEvent->mKeyPressHandled = true;
-      currentKeyEvent->mKeyPressDispatched = true;
-    }
     return;
   }
 
   // Don't let the same event be fired twice when hitting
   // enter/return! (Bug 420502)
   if (currentKeyEvent && !currentKeyEvent->CanDispatchKeyPressEvent()) {
     return;
   }
--- a/widget/tests/test_keycodes.xul
+++ b/widget/tests/test_keycodes.xul
@@ -1878,16 +1878,30 @@ function* runKeyEventTests()
                    modifiers:{metaKey:1}, chars:"d", unmodifiedChars:"e"},
                   nsIDOMKeyEvent.DOM_VK_D, "d", SHOULD_DELIVER_KEYDOWN_KEYPRESS, KeyboardEvent.DOM_KEY_LOCATION_STANDARD);
     yield testKey({layout:KEYBOARD_LAYOUT_DVORAK_QWERTY, keyCode:MAC_VK_ANSI_I,
                    modifiers:{metaKey:1, altKey:1}, chars:"^", unmodifiedChars:"c"},
                   nsIDOMKeyEvent.DOM_VK_I, "^", SHOULD_DELIVER_KEYDOWN_KEYPRESS, KeyboardEvent.DOM_KEY_LOCATION_STANDARD);
     yield testKey({layout:KEYBOARD_LAYOUT_DVORAK_QWERTY, keyCode:MAC_VK_ANSI_I,
                    modifiers:{metaKey:1, altKey:1, shiftKey:1}, chars:"\u02C6", unmodifiedChars:"C"},
                   nsIDOMKeyEvent.DOM_VK_I, "\u02C6", SHOULD_DELIVER_KEYDOWN_KEYPRESS, KeyboardEvent.DOM_KEY_LOCATION_STANDARD);
+
+    // Arabic - PC keyboard layout inputs 2 or more characters with some key.
+    yield testKey({layout:KEYBOARD_LAYOUT_ARABIC_PC, keyCode:MAC_VK_ANSI_G,
+                   modifiers:{shiftKey:1}, chars:"\u0644\u0623", unmodifiedChars:"\u0644\u0623"},
+                  nsIDOMKeyEvent.DOM_VK_G, "\u0644\u0623", SHOULD_DELIVER_ALL, KeyboardEvent.DOM_KEY_LOCATION_STANDARD);
+    yield testKey({layout:KEYBOARD_LAYOUT_ARABIC_PC, keyCode:MAC_VK_ANSI_T,
+                   modifiers:{shiftKey:1}, chars:"\u0644\u0625", unmodifiedChars:"\u0644\u0625"},
+                  nsIDOMKeyEvent.DOM_VK_T, "\u0644\u0625", SHOULD_DELIVER_ALL, KeyboardEvent.DOM_KEY_LOCATION_STANDARD);
+    yield testKey({layout:KEYBOARD_LAYOUT_ARABIC_PC, keyCode:MAC_VK_ANSI_B,
+                   modifiers:{shiftKey:1}, chars:"\u0644\u0622", unmodifiedChars:"\u0644\u0622"},
+                  nsIDOMKeyEvent.DOM_VK_B, "\u0644\u0622", SHOULD_DELIVER_ALL, KeyboardEvent.DOM_KEY_LOCATION_STANDARD);
+    yield testKey({layout:KEYBOARD_LAYOUT_ARABIC_PC, keyCode:MAC_VK_ANSI_B,
+                   modifiers:{}, chars:"\u0644\u0627", unmodifiedChars:"\u0644\u0627"},
+                  nsIDOMKeyEvent.DOM_VK_B, "\u0644\u0627", SHOULD_DELIVER_ALL, KeyboardEvent.DOM_KEY_LOCATION_STANDARD);
   } else if (IS_WIN) {
     // On Windows, you can use Spy++ or Winspector (free) to watch window messages.
     // The keyCode is given by the wParam of the last WM_KEYDOWN message. The
     // chars string is given by the wParam of the WM_CHAR message. unmodifiedChars
     // is not needed on Windows.
 
     // Plain text input
     yield testKey({layout:KEYBOARD_LAYOUT_EN_US, keyCode:WIN_VK_A,
@@ -3464,21 +3478,47 @@ function* runReservedKeyTests()
 function* runTextInputTests()
 {
   var textbox = document.getElementById("textbox");
 
   function testKey(aEvent, aExpectText) {
     textbox.value = "";
     textbox.focus();
 
-    return synthesizeKey(aEvent, "textbox", function() {
+    var name = eventToString(aEvent);
 
-      var name = eventToString(aEvent);
+    // Check if the text comes with keypress events rather than composition events.
+    var keypress = 0;
+    function onKeypress(aEvent) {
+      keypress++;
+      if (keypress == 1 && aExpectText == "") {
+        if (!aEvent.ctrlKey && !aEvent.altKey) {
+          is(aEvent.charCode, 0, name + ", the charCode value should be 0 when it shouldn't cause inputting text");
+        }
+        return;
+      }
+      if (keypress > aExpectText.length) {
+        ok(false, name + " causes too many keypress events");
+        return;
+      }
+      is(aEvent.key, aExpectText[keypress - 1],
+         name + ", " + keypress + "th keypress event's key value should be '" + aExpectText[keypress - 1] + "'");
+      is(aEvent.charCode, aExpectText.charCodeAt(keypress - 1),
+         name + ", " + keypress + "th keypress event's charCode value should be 0x" + parseInt(aExpectText.charCodeAt(keypress - 1), 16));
+    }
+    textbox.addEventListener("keypress", onKeypress, true);
 
-      is(textbox.value, aExpectText, name + " does not input correct text.");
+    return synthesizeKey(aEvent, "textbox", function() {
+      textbox.removeEventListener("keypress", onKeypress, true);
+      if (aExpectText == "") {
+        is(keypress, 1, name + " should cause one keypress event because it doesn't cause inputting text");
+      } else {
+        is(keypress, aExpectText.length, name + " should cause " + aExpectText.length + " keypress events");
+        is(textbox.value, aExpectText, name + " does not input correct text.");
+      }
 
       continueTest();
     });
   }
 
   if (IS_MAC) {
     yield testKey({layout:KEYBOARD_LAYOUT_ARABIC_PC, keyCode:MAC_VK_ANSI_G,
                    modifiers:{shiftKey:1}, chars:"\u0644\u0623", unmodifiedChars:"\u0644\u0623"},