Bug 1438133 - Ctrl + Enter should cause keypress event even though the key combination doesn't input any character r=smaug
authorMasayuki Nakano <masayuki@d-toybox.com>
Wed, 14 Feb 2018 21:21:18 +0900
changeset 404345 cb4185c40f3d2445850029e4785d324783375497
parent 404337 34d3b357c86707050c08f7b1e1239d9161d484a5
child 404346 e93c3591c5425dab2bf4919cec272aaea83acba4
child 404359 a762bc86c38e51320a2007ecd2a15df0687544e4
push id33468
push usercsabou@mozilla.com
push dateSun, 18 Feb 2018 21:54:13 +0000
treeherdermozilla-central@e93c3591c542 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1438133
milestone60.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 1438133 - Ctrl + Enter should cause keypress event even though the key combination doesn't input any character r=smaug Currently, we dispatch keypress event when Enter is pressed without modifiers or only with Shift key. However, the other browsers dispatch keypress event for Ctrl + Enter in any platforms even if it doesn't cause any text input. So, we should fire keypress event for Ctrl + Enter even in strict keypress dispatching mode. Note that with other modifiers, it depends on browser and/or platform. So, anyway, web developers shouldn't use keypress event to catch Alt + Enter, Meta + Enter and two or more modifiers + Enter. MozReview-Commit-ID: 3uUMkhL5VfJ
dom/events/test/test_dom_keyboard_event.html
widget/TextEventDispatcher.cpp
widget/TextEvents.h
--- a/dom/events/test/test_dom_keyboard_event.html
+++ b/dom/events/test/test_dom_keyboard_event.html
@@ -3,16 +3,19 @@
 <head>
   <title>Test for DOM KeyboardEvent</title>
   <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
   <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
 </head>
 <body>
 <p id="display"></p>
+<p><input type="text" id="input"></p>
+<p><input type="text" id="input_readonly" readonly></p>
+<p><textarea id="textarea"></textarea></p>
 <div id="content" style="display: none">
   
 </div>
 <pre id="test">
 <script type="application/javascript">
 
 SimpleTest.waitForExplicitFinish();
 SimpleTest.waitForFocus(runTests, window);
@@ -372,19 +375,172 @@ function testSynthesizedKeyLocation()
     ok(events.keyup, description + "keyup event wasn't fired");
   }
 
   window.removeEventListener("keydown", handler, true);
   window.removeEventListener("keypress", handler, true);
   window.removeEventListener("keyup", handler, true);
 }
 
+// We're using TextEventDispatcher to decide if we should keypress event
+// on content in the default event group.  So, we can test if keypress
+// event is NOT fired unexpectedly with synthesizeKey().
+function testEnterKeyPressEvent()
+{
+  let keydownFired, keypressFired, beforeinputFired;
+  function onEvent(aEvent) {
+    switch (aEvent.type) {
+      case "keydown":
+        keydownFired = true;
+        return;
+      case "keypress":
+        keypressFired = true;
+        return;
+      case "beforeinput":
+        beforeinputFired = true;
+        return;
+    }
+  }
+
+  for (let targetId of ["input", "textarea", "input_readonly"]) {
+    let target = document.getElementById(targetId);
+
+    function reset() {
+      keydownFired = keypressFired = beforeinputFired = false;
+      target.value = "";
+    }
+
+    target.addEventListener("keydown", onEvent);
+    target.addEventListener("keypress", onEvent);
+    target.addEventListener("beforeinput", onEvent);
+
+    const kDescription = "<" + targetId.replace("_", " ") + ">: ";
+    let isEditable = kDescription.includes("readonly");
+    let isTextarea = kDescription.includes("textarea");
+
+    target.focus();
+
+    reset();
+    synthesizeKey("KEY_Enter", {});
+    is(keydownFired, true,
+       kDescription + "keydown event should be fired when Enter key is pressed");
+    is(keypressFired, true,
+       kDescription + "keypress event should be fired when Enter key is pressed");
+    if (isEditable) {
+      todo_is(beforeinputFired, true,
+              kDescription + "beforeinput event should be fired when Enter key is pressed");
+    } else {
+      is(beforeinputFired, false,
+         kDescription + "beforeinput event shouldn't be fired when Enter key is pressed");
+    }
+    if (isTextarea) {
+      is(target.value, "\n",
+         kDescription + "Enter key should cause inputting a line break in <textarea>");
+    } else {
+      is(target.value, "",
+         kDescription + "Enter key should not cause inputting a line break");
+    }
+
+    reset();
+    synthesizeKey("KEY_Enter", {shiftKey: true});
+    is(keydownFired, true,
+       kDescription + "keydown event should be fired when Shift + Enter key is pressed");
+    is(keypressFired, true,
+       kDescription + "keypress event should be fired when Shift + Enter key is pressed");
+    if (isEditable) {
+      todo_is(beforeinputFired, true,
+              kDescription + "beforeinput event should be fired when Shift + Enter key is pressed");
+    } else {
+      is(beforeinputFired, false,
+         kDescription + "beforeinput event shouldn't be fired when Shift + Enter key is pressed");
+    }
+    if (isTextarea) {
+      is(target.value, "\n",
+         kDescription + "Shift + Enter key should cause inputting a line break in <textarea>");
+    } else {
+      is(target.value, "",
+         kDescription + "Shift + Enter key should not cause inputting a line break");
+    }
+
+    reset();
+    synthesizeKey("KEY_Enter", {ctrlKey: true});
+    is(keydownFired, true,
+       kDescription + "keydown event should be fired when Ctrl + Enter key is pressed");
+    is(keypressFired, true,
+       kDescription + "keypress event should be fired when Ctrl + Enter key is pressed");
+    is(beforeinputFired, false,
+       kDescription + "beforeinput event shouldn't be fired when Ctrl + Enter key is pressed");
+    is(target.value, "",
+       kDescription + "Ctrl + Enter key should not cause inputting a line break");
+
+    reset();
+    synthesizeKey("KEY_Enter", {altKey: true});
+    is(keydownFired, true,
+       kDescription + "keydown event should be fired when Alt + Enter key is pressed");
+    is(keypressFired, !kStrictKeyPressEvents,
+       kDescription + "keypress event shouldn't be fired when Alt + Enter key is pressed in strict keypress dispatching mode");
+    is(beforeinputFired, false,
+       kDescription + "beforeinput event shouldn't be fired when Alt + Enter key is pressed");
+    is(target.value, "",
+       kDescription + "Alt + Enter key should not cause inputting a line break");
+
+    reset();
+    synthesizeKey("KEY_Enter", {metaKey: true});
+    is(keydownFired, true,
+       kDescription + "keydown event should be fired when Meta + Enter key is pressed");
+    is(keypressFired, !kStrictKeyPressEvents,
+       kDescription + "keypress event shouldn't be fired when Meta + Enter key is pressed in strict keypress dispatching mode");
+    is(beforeinputFired, false,
+       kDescription + "beforeinput event shouldn't be fired when Meta + Enter key is pressed");
+    is(target.value, "",
+       kDescription + "Meta + Enter key should not cause inputting a line break");
+
+    reset();
+    synthesizeKey("KEY_Enter", {shiftKey: true, ctrlKey: true});
+    is(keydownFired, true,
+       kDescription + "keydown event should be fired when Ctrl + Shift + Enter key is pressed");
+    is(keypressFired, !kStrictKeyPressEvents,
+       kDescription + "keypress event shouldn't be fired when Ctrl + Shift + Enter key is pressed in strict keypress dispatching mode");
+    is(beforeinputFired, false,
+       kDescription + "beforeinput event shouldn't be fired when Ctrl + Shift + Enter key is pressed");
+    is(target.value, "",
+       kDescription + "Ctrl + Shift + Enter key should not cause inputting a line break");
+
+    reset();
+    synthesizeKey("KEY_Enter", {shiftKey: true, altKey: true});
+    is(keydownFired, true,
+       kDescription + "keydown event should be fired when Alt + Shift + Enter key is pressed");
+    is(keypressFired, !kStrictKeyPressEvents,
+       kDescription + "keypress event shouldn't be fired when Alt + Shift + Enter key is pressed in strict keypress dispatching mode");
+    is(beforeinputFired, false,
+       kDescription + "beforeinput event shouldn't be fired when Alt + Shift + Enter key is pressed");
+    is(target.value, "",
+       kDescription + "Alt + Shift + Enter key should not cause inputting a line break");
+
+    reset();
+    synthesizeKey("KEY_Enter", {shiftKey: true, metaKey: true});
+    is(keydownFired, true,
+       kDescription + "keydown event should be fired when Meta + Shift + Enter key is pressed");
+    is(keypressFired, !kStrictKeyPressEvents,
+       kDescription + "keypress event shouldn't be fired when Meta + Shift + Enter key is pressed in strict keypress dispatching mode");
+    is(beforeinputFired, false,
+       kDescription + "beforeinput event shouldn't be fired when Meta + Shift + Enter key is pressed");
+    is(target.value, "",
+       kDescription + "Meta + Shift + Enter key should not cause inputting a line break");
+
+    target.removeEventListener("keydown", onEvent);
+    target.removeEventListener("keypress", onEvent);
+    target.removeEventListener("beforeinput", onEvent);
+  }
+}
+
 function runTests()
 {
   testInitializingUntrustedEvent();
   testSynthesizedKeyLocation();
+  testEnterKeyPressEvent();
   SimpleTest.finish();
 }
 
 </script>
 </pre>
 </body>
 </html>
--- a/widget/TextEventDispatcher.cpp
+++ b/widget/TextEventDispatcher.cpp
@@ -691,17 +691,17 @@ TextEventDispatcher::DispatchKeyboardEve
                    static_cast<WidgetKeyboardEvent&>(original).mKeyValue);
       MOZ_ASSERT(keyEvent.mCodeValue ==
                    static_cast<WidgetKeyboardEvent&>(original).mCodeValue);
     }
   }
 
   if (sDispatchKeyPressEventsOnlySystemGroupInContent &&
       keyEvent.mMessage == eKeyPress &&
-      !keyEvent.IsInputtingText() && !keyEvent.IsInputtingLineBreak()) {
+      !keyEvent.ShouldKeyPressEventBeFiredOnContent()) {
     keyEvent.mFlags.mOnlySystemGroupDispatchInContent = true;
   }
 
   DispatchInputEvent(mWidget, keyEvent, aStatus);
   return true;
 }
 
 bool
--- a/widget/TextEvents.h
+++ b/widget/TextEvents.h
@@ -246,16 +246,39 @@ public:
     return mMessage == eKeyPress &&
            mKeyNameIndex == KEY_NAME_INDEX_Enter &&
            !(mModifiers & (MODIFIER_ALT |
                            MODIFIER_CONTROL |
                            MODIFIER_META |
                            MODIFIER_OS));
   }
 
+  /**
+   * ShouldKeyPressEventBeFiredOnContent() should be called only when the
+   * instance is eKeyPress event.  This returns true when the eKeyPress
+   * event should be fired even on content in the default event group.
+   */
+  bool ShouldKeyPressEventBeFiredOnContent() const
+  {
+    MOZ_DIAGNOSTIC_ASSERT(mMessage == eKeyPress);
+    if (IsInputtingText() || IsInputtingLineBreak()) {
+      return true;
+    }
+    // Ctrl + Enter won't cause actual input in our editor.
+    // However, the other browsers fire keypress event in any platforms.
+    // So, for compatibility with them, we should fire keypress event for
+    // Ctrl + Enter too.
+    return mMessage == eKeyPress &&
+           mKeyNameIndex == KEY_NAME_INDEX_Enter &&
+           !(mModifiers & (MODIFIER_ALT |
+                           MODIFIER_META |
+                           MODIFIER_OS |
+                           MODIFIER_SHIFT));
+  }
+
   virtual WidgetEvent* Duplicate() const override
   {
     MOZ_ASSERT(mClass == eKeyboardEventClass,
                "Duplicate() must be overridden by sub class");
     // Not copying widget, it is a weak reference.
     WidgetKeyboardEvent* result =
       new WidgetKeyboardEvent(false, mMessage, nullptr);
     result->AssignKeyEventData(*this, true);