Bug 1615977 - Don't modify key value while pressing a numpad key with `Alt` but the virtual keycode is a function key r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 27 Feb 2020 03:21:36 +0000
changeset 515824 eb02d91fd3b8cbe8cf7b6356eac45123b9ee50de
parent 515823 7fc17499b72099c0fa5d27f76c619229c01f2516
child 515825 b3dfa17c6305c657f8c3b3c95121c19f005d451c
push id37162
push usernbeleuzu@mozilla.com
push dateThu, 27 Feb 2020 09:49:56 +0000
treeherdermozilla-central@9e8d5431c412 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1615977, 1606655
milestone75.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 1615977 - Don't modify key value while pressing a numpad key with `Alt` but the virtual keycode is a function key r=m_kato Inputting Unicode scalar value with `Alt` + numpad keys is available even without NumLock state. However, `Alt` + function key on numpad should also be worked because user may intent to perform it. Therefore, this patch stops the hacking for bug 1606655 when given virtual keycode value is a function key, but this means that users cannot type a Unicode scalar value without NumLock key state if the value includes `7` (`Home`), `4` (`ArrowLeft`), `6` (`ArrowRight`) because Firefox UI handles they are shortcut keys (for "Go home", "Go back" and "Go forward"). Unfortunately, I have no idea how to solve this conflict (if it's second key or latter key after pressing `Alt` key, we could do that with a boolean flag, but I don't like to make it only for this kind of edge case unless a lot of users want to do it). Differential Revision: https://phabricator.services.mozilla.com/D63782
widget/windows/KeyboardLayout.cpp
widget/windows/KeyboardLayout.h
--- a/widget/windows/KeyboardLayout.cpp
+++ b/widget/windows/KeyboardLayout.cpp
@@ -3490,22 +3490,28 @@ void NativeKey::ComputeInputtingStringWi
   }
 
   // If user is inputting a Unicode character with typing Alt + Numpad
   // keys, we shouldn't set alternative key codes because the key event
   // shouldn't match with a mnemonic.  However, we should set only
   // mUnshiftedString for keeping traditional behavior at WM_SYSKEYDOWN.
   // FYI: I guess that it's okay that mUnshiftedString stays empty.  So,
   //      if its value breaks something, you must be able to just return here.
-  if (IsTypingUnicodeScalarValue()) {
-    MOZ_ASSERT(mMsg.message == WM_SYSKEYDOWN);
-    MOZ_ASSERT(!mCommittedCharsAndModifiers.IsEmpty());
-    char16_t num = mCommittedCharsAndModifiers.CharAt(0);
-    MOZ_ASSERT(num >= '0' && num <= '9');
-    mUnshiftedString.Append(num, MODIFIER_NONE);
+  if (MaybeTypingUnicodeScalarValue()) {
+    if (!mCommittedCharsAndModifiers.IsEmpty()) {
+      MOZ_ASSERT(mMsg.message == WM_SYSKEYDOWN);
+      char16_t num = mCommittedCharsAndModifiers.CharAt(0);
+      MOZ_ASSERT(num >= '0' && num <= '9');
+      mUnshiftedString.Append(num, MODIFIER_NONE);
+      return;
+    }
+    // If user presses a function key without NumLock or 3rd party utility
+    // synthesizes a function key on numpad, we should handle it as-is because
+    // the user's intention may be performing `Alt` + foo.
+    MOZ_ASSERT(!KeyboardLayout::IsPrintableCharKey(mVirtualKeyCode));
     return;
   }
 
   ModifierKeyState capsLockState(mModKeyState.GetModifiers() &
                                  MODIFIER_CAPSLOCK);
 
   mUnshiftedString =
       keyboardLayout->GetUniCharsAndModifiers(mVirtualKeyCode, capsLockState);
@@ -3979,17 +3985,25 @@ void KeyboardLayout::InitNativeKey(Nativ
     }
   }
 
   // If the aNativeKey is in a sequence to input a Unicode character with
   // Alt + numpad keys, we should just set the number as the inputting charcter.
   // Note that we should compute the key value from the virtual key code
   // because they may be mapped to alphabets, but they should be treated as
   // Alt + [0-9] even by web apps.
-  if (aNativeKey.IsTypingUnicodeScalarValue()) {
+  // However, we shouldn't touch the key value if given virtual key code is
+  // not a printable key because it may be synthesized by 3rd party utility
+  // or just NumLock is unlocked and user tries to use shortcut key.  In the
+  // latter case, we cannot solve the conflict issue with Alt+foo shortcut key
+  // and inputting a Unicode scalar value like reported to bug 1606655, though,
+  // I have no better idea.  Perhaps, `Alt` shouldn't be used for shortcut key
+  // combination on Windows.
+  if (aNativeKey.MaybeTypingUnicodeScalarValue() &&
+      KeyboardLayout::IsPrintableCharKey(aNativeKey.mVirtualKeyCode)) {
     // If the key code value is mapped to a Numpad key, let's compute the key
     // value with it.  This is same behavior as Chrome.  In strictly speaking,
     // I think that the else block's computation is better because it seems
     // that Windows does not refer virtual key code value, but we should avoid
     // web-compat issues.
     char16_t num = '0';
     if (aNativeKey.mVirtualKeyCode >= VK_NUMPAD0 &&
         aNativeKey.mVirtualKeyCode <= VK_NUMPAD9) {
--- a/widget/windows/KeyboardLayout.h
+++ b/widget/windows/KeyboardLayout.h
@@ -603,18 +603,22 @@ class MOZ_STACK_CLASS NativeKey final {
    * strange WM_KEYDOWN/WM_KEYUP/WM_CHAR message pattern.  So, when this
    * returns true, the caller needs to be careful for processing the messages.
    */
   bool IsIMEDoingKakuteiUndo() const;
 
   /**
    * This returns true if user types a number key in numpad with Alt key
    * to input a Unicode character from its scalar value.
+   * Note that inputting Unicode scalar value is available without NumLock.
+   * Therefore, this returns true even if user presses a function key on
+   * numpad without NumLock, but that may be intended to perform a shortcut
+   * key like Alt + Home.
    */
-  bool IsTypingUnicodeScalarValue() const {
+  bool MaybeTypingUnicodeScalarValue() const {
     return !mIsExtended && IsSysKeyDownOrKeyUpMessage() && IsAlt() &&
            !IsControl() && !IsShift() &&
            ((mScanCode >= 0x004F && mScanCode <= 0x0052) ||  // Numpad0-3
             (mScanCode >= 0x004B && mScanCode <= 0x004D) ||  // Numpad4-6
             (mScanCode >= 0x0047 && mScanCode <= 0x0049));   // Numpad7-9
   }
 
   bool IsKeyDownMessage() const {