Bug 1310565 TextInputHandler shouldn't dispatch a composition events when a key press causes 2 or more characters r?m_kato draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Tue, 18 Oct 2016 15:26:54 +0900
changeset 426432 774c0b5c1a05bb83a113239e8610380214d5847c
parent 426151 56b3f2c6f53e72698fea6c25130efceef2a26548
child 534148 48b37538f99a3fd8c411cae9a9b72d892713cddc
push id32676
push usermasayuki@d-toybox.com
push dateTue, 18 Oct 2016 09:40:18 +0000
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"},