Bug 1458845 - Make TextEventDispatcher dispatch keypress event even if altKey is true on macOS r=smaug
authorMasayuki Nakano <masayuki@d-toybox.com>
Wed, 23 May 2018 22:27:17 +0900
changeset 476821 8128097dd7a739e26b1e796cd256555e003ae3e6
parent 476820 b19a39acb93990a0df7b6e696bca119cab3ac738
child 476822 e70e3d46d44190f66bc61b979c334424606578de
push id1757
push userffxbld-merge
push dateFri, 24 Aug 2018 17:02:43 +0000
treeherdermozilla-release@736023aebdb1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1458845
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 1458845 - Make TextEventDispatcher dispatch keypress event even if altKey is true on macOS r=smaug On macOS, option key is mapped to alt key, and it works like AltGr key on the other platforms. Since our editor doesn't accept keypress events as typing printable character if one of altKey, ctrlKey and metaKey of the events is true, widget for macOS sets those attributes to false when an editor has focus. On the other hand, if no editor has focus, our widget does not do this hack so that altKey and ctrlKey values of keypress events are always same as actual user operation and this is same behavior as the other browsers. Therefore, we need to keep setting altKey of keypress events to true if no editor has focus but we need to dispatch keypress events even on content unless the charCode is 0. So, only on macOS, WidgetKeyboardEvent::IsInputtingText() does not need to check altKey state. MozReview-Commit-ID: 4DMgdOfLqvQ
widget/TextEvents.h
widget/tests/mochitest.ini
widget/tests/test_keypress_event_with_alt_on_mac.html
--- a/widget/TextEvents.h
+++ b/widget/TextEvents.h
@@ -223,24 +223,67 @@ public:
   {
     return aMessage == eKeyDownOnPlugin || aMessage == eKeyUpOnPlugin;
   }
   bool IsKeyEventOnPlugin() const
   {
     return IsKeyEventOnPlugin(mMessage);
   }
 
+  // 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.
+  // <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 unset the modifier flag
-    //       from eKeyPress event.
+    //       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.
+    //       -- 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.
+    // 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 &&
            mCharCode &&
-           !(mModifiers & (MODIFIER_ALT |
+           !(mModifiers & (
+#ifndef XP_MACOSX
+                           // So, ignore MODIFIER_ALT only on macOS since
+                           // option key is used as AltGraph key on macOS.
+                           MODIFIER_ALT |
+#endif // #ifndef XP_MAXOSX
                            MODIFIER_CONTROL |
                            MODIFIER_META |
                            MODIFIER_OS));
   }
 
   bool IsInputtingLineBreak() const
   {
     return mMessage == eKeyPress &&
--- a/widget/tests/mochitest.ini
+++ b/widget/tests/mochitest.ini
@@ -1,9 +1,11 @@
 [DEFAULT]
 support-files = utils.js
 
 [test_assign_event_data.html]
 subsuite = clipboard
 skip-if = toolkit == "cocoa" || (toolkit == 'android' && debug) # Mac: Bug 933303, Android bug 1285414
+[test_keypress_event_with_alt_on_mac.html]
+skip-if = toolkit != "cocoa"
 [test_picker_no_crash.html]
 skip-if = toolkit != "windows" || e10s # Bug 1267491
 support-files = window_picker_no_crash_child.html
new file mode 100644
--- /dev/null
+++ b/widget/tests/test_keypress_event_with_alt_on_mac.html
@@ -0,0 +1,118 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <title>Testing if keypress event is fired when alt key is pressed</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/NativeKeyCodes.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css">
+</head>
+<body>
+<div id="display">
+  <input id="input">
+  <input id="password" type="password">
+  <input id="readonly-input" readonly>
+  <textarea id="textarea"></textarea>
+  <textarea id="readonly-textarea" readonly></textarea>
+  <button id="button">button</button>
+</div>
+<div id="content" style="display: none">
+</div>
+<pre id="test">
+</pre>
+
+<script class="testbody" type="application/javascript">
+SimpleTest.waitForExplicitFinish();
+
+async function testNativeKey(aKeyboardLayout, aNativeKeyCode, aModifiers,
+                             aChars, aUnmodifiedChars)
+{
+  // XXX Need to listen keyup event here because synthesizeNativeKey() does not
+  //     guarantee that its callback will be called after "keypress" and "keyup".
+  let waitForKeyUp = new Promise(resolve => {
+    document.addEventListener("keyup", resolve, {once: true});
+  });
+  let keypress;
+  document.addEventListener("keypress", (aKeyPressEvent) => {
+    keypress = aKeyPressEvent;
+  }, {once: true});
+  synthesizeNativeKey(aKeyboardLayout, aNativeKeyCode, aModifiers, aChars, aUnmodifiedChars);
+  await waitForKeyUp;
+  return keypress;
+}
+
+async function runTests()
+{
+  await SpecialPowers.pushPrefEnv({"set": [
+          ["dom.keyboardevent.keypress.dispatch_non_printable_keys_only_system_group_in_content", true],
+        ]});
+  const kTests =
+    [ { target: "input", isEditable: true },
+      { target: "password", isEditable: true },
+      { target: "readonly-input", isEditable: false },
+      { target: "textarea", isEditable: true },
+      { target: "readonly-textarea", isEditable: false },
+      { target: "button", isEditable: false } ];
+  for (const kTest of kTests) {
+    let element = document.getElementById(kTest.target);
+    element.focus();
+
+    const kDescription = kTest.target + ": ";
+
+    let keypress = await testNativeKey(KEYBOARD_LAYOUT_EN_US, MAC_VK_ANSI_A, {}, "a", "a");
+    ok(keypress,
+       kDescription + "'a' key press should cause firing keypress event");
+
+    keypress = await testNativeKey(KEYBOARD_LAYOUT_EN_US, MAC_VK_ANSI_A, {shiftKey: true}, "A", "A");
+    ok(keypress,
+       kDescription + "'a' key press with shift key should cause firing keypress event");
+    ok(keypress.shiftKey,
+       kDescription + "shiftKey of 'a' key press with shift key should be true");
+
+    // When a key inputs a character with option key, we need to unset altKey for our editor.
+    // Otherwise, altKey should be true for compatibility with the other browsers.
+    keypress = await testNativeKey(KEYBOARD_LAYOUT_EN_US, MAC_VK_ANSI_A, {altKey: true}, "\u00E5", "a");
+    ok(keypress,
+       kDescription + "'a' key press with option key should cause firing keypress event");
+    is(keypress.altKey, !kTest.isEditable,
+       kDescription + "altKey of 'a' key press with option key should be " + !kTest.isEditable);
+
+    keypress = await testNativeKey(KEYBOARD_LAYOUT_EN_US, MAC_VK_ANSI_A, {altKey: true, shiftKey: true}, "\u00C5", "A");
+    ok(keypress,
+       kDescription + "'a' key press with option key  and shift key should cause firing keypress event");
+    is(keypress.altKey, !kTest.isEditable,
+       kDescription + "altKey of 'a' key press with option key and shift key should be " + !kTest.isEditable);
+    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");
+    }
+
+    // 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");
+  }
+
+  SimpleTest.finish();
+}
+
+SimpleTest.waitForFocus(runTests);
+</script>
+</body>
+</html>
\ No newline at end of file