Bug 1310565 TextInputHandler shouldn't dispatch a composition events when a key press causes 2 or more characters r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Tue, 18 Oct 2016 15:26:54 +0900
changeset 318588 106fc7412c9f743be5e0a4b2f87a5a196f6b034e
parent 318587 1d9e3c771a6cdfc2bb831a4bc1049de0457b3798
child 318589 a4006ffc814bdddd6763d1aa7a79547232e6557f
push id20725
push userphilringnalda@gmail.com
push dateThu, 20 Oct 2016 01:36:01 +0000
treeherderfx-team@998ad5a74da8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1310565
milestone52.0a1
Bug 1310565 TextInputHandler shouldn't dispatch a composition events when a key press causes 2 or more characters r=m_kato 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. So, for consistency between platforms, TextInputHandler should dispatch eKeyPress events in such case. MozReview-Commit-ID: EMvaL7sklKf
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
@@ -1901,16 +1901,31 @@ function* runKeyEventTests()
                    modifiers:{metaKey:1}, chars:"d", unmodifiedChars:"e"},
                   "d", "KeyD", 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"},
                   "^", "KeyI", 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"},
                   "\u02C6", "KeyI", 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"},
+                  ["\u0644\u0623", "\u0644", "\u0623", "\u0644\u0623"], "KeyG", 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"},
+                  ["\u0644\u0625", "\u0644", "\u0625", "\u0644\u0625"], "KeyT", 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"},
+                  ["\u0644\u0622", "\u0644", "\u0622", "\u0644\u0622"], "KeyB", 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"},
+                  ["\u0644\u0627", "\u0644", "\u0627", "\u0644\u0627"], "KeyB", nsIDOMKeyEvent.DOM_VK_B, "\u0644\u0627", SHOULD_DELIVER_ALL, KeyboardEvent.DOM_KEY_LOCATION_STANDARD);
+
     cleanup();
   }
 
   function testKeysOnWindows()
   {
     // 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
@@ -4192,21 +4207,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"},