Bug 1293638 NativeKey::WillDispatchKeyboardEvent() should set alternative charCode values properly when other shift state inputs longer text r=m_kato a=ritu
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 01 Sep 2016 14:26:02 +0900
changeset 350107 a4ee689323092f6ac3c81b9d4d4ea3a9f5d8276f
parent 350106 c62d6e8a740719270dcc4b64148ae8c694343e4a
child 350108 245983b752c0db9d954a8ebd928d15a0bff3ae1b
push id1230
push userjlund@mozilla.com
push dateMon, 31 Oct 2016 18:13:35 +0000
treeherdermozilla-release@5e06e3766db2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato, ritu
bugs1293638
milestone50.0a2
Bug 1293638 NativeKey::WillDispatchKeyboardEvent() should set alternative charCode values properly when other shift state inputs longer text r=m_kato a=ritu There are 2 bugs and this patch fixes them once. First, NativeKey::WillDispatchKeyboardEvent() is used to setting alternative charCode values for every eKeyPress event. However, for supporting "reserved" shortcut keys, now, it sets alternative charCode values to eKeyDown too. However, they are really different. eKeyPress events are fired for every character to be inputted by a key press sequence. On the other hand, eKeyDown event is fired only once for a key sequence. Therefore, now, NativeKey::WillDispatchKeyboardEvent() needs to set alternative charCode values for all characters inputted by the key sequence to eKeyDown event. The other is not a new bug. NativeKey::WillDispatchKeyboardEvent() sets the last eKeyPress event's special alternative charCode values, such as unshifted Latin character, shifted Latin character and some character which can be computed from virtual keycode. This is performed when given index is the last index of the longest input string of the key. However, the value includes different shift key state. I.e., when different shift key causes longer text input, NativeKey::WillDispatchKeyboardEvent() won't set the special alternative charCode values to any eKeyPress events. For example, when Ctrl+T is pressed with Arabic keyboard layout, its unshifted input string length is 1 but shifted input string length is 2. Then, eKeyPress event is fired only once, but NativeKey::WillDispatchKeyboardEvent() waits second eKeyPress event. Therefore, this patch makes the method append alternative charCodes for all remaining characters and detect the last event correctly with mCommittedCharsAndModifiers (it's used for KeyboardEvent.key value of eKeyDown event and the count of eKeyPress events is same as the value's length). MozReview-Commit-ID: 6adUnmi5KYy
widget/tests/test_keycodes.xul
widget/windows/KeyboardLayout.cpp
--- a/widget/tests/test_keycodes.xul
+++ b/widget/tests/test_keycodes.xul
@@ -3435,16 +3435,18 @@ function* runReservedKeyTests()
   if (IS_MAC) {
     // Cmd+T is reserved for opening new tab.
     yield testReservedKey({layout:KEYBOARD_LAYOUT_EN_US, keyCode:MAC_VK_ANSI_T,
                            modifiers:{metaKey:1}, chars:"t", unmodifiedChars:"t"});
   } else if (IS_WIN) {
     // Ctrl+T is reserved for opening new tab.
     yield testReservedKey({layout:KEYBOARD_LAYOUT_EN_US, keyCode:WIN_VK_T,
                            modifiers:{ctrlKey:1}, chars:"\u0014"});
+    yield testReservedKey({layout:KEYBOARD_LAYOUT_ARABIC, keyCode:WIN_VK_T,
+                           modifiers:{ctrlKey:1}, chars:"\u0014"});
   }
 
   finializeKeyElementTest();
 }
 
 function* runTextInputTests()
 {
   var textbox = document.getElementById("textbox");
--- a/widget/windows/KeyboardLayout.cpp
+++ b/widget/windows/KeyboardLayout.cpp
@@ -2343,21 +2343,33 @@ NativeKey::WillDispatchKeyboardEvent(Wid
              std::max(mShiftedString.mLength, mUnshiftedString.mLength));
   uint32_t skipUniChars = longestLength - mInputtingStringAndModifiers.mLength;
   uint32_t skipShiftedChars = longestLength - mShiftedString.mLength;
   uint32_t skipUnshiftedChars = longestLength - mUnshiftedString.mLength;
   if (aIndex >= longestLength) {
     return;
   }
 
+  // Check if aKeyboardEvent is the last event for a key press.
+  // So, if it's not an eKeyPress event, it's always the last event.
+  // Otherwise, check if the index is the last character of
+  // mCommittedCharsAndModifiers.
+  bool isLastIndex =
+    aKeyboardEvent.mMessage != eKeyPress ||
+    mCommittedCharsAndModifiers.IsEmpty() ||
+    mCommittedCharsAndModifiers.mLength - 1 == aIndex;
+
   nsTArray<AlternativeCharCode>& altArray =
     aKeyboardEvent.mAlternativeCharCodes;
 
-  uint16_t shiftedChar = 0, unshiftedChar = 0;
-  if (skipUniChars <= aIndex) {
+  // Set charCode and adjust modifier state for every eKeyPress event.
+  // This is not necessary for the other keyboard events because the other
+  // keyboard events shouldn't have non-zero charCode value and should have
+  // current modifier state.
+  if (aKeyboardEvent.mMessage == eKeyPress && skipUniChars <= aIndex) {
     // XXX Modifying modifier state of aKeyboardEvent is illegal, but no way
     //     to set different modifier state per keypress event except this
     //     hack.  Note that ideally, dead key should cause composition events
     //     instead of keypress events, though.
     if (aIndex - skipUniChars  < mInputtingStringAndModifiers.mLength) {
       ModifierKeyState modKeyState(mModKeyState);
       // If key in combination with Alt and/or Ctrl produces a different
       // character than without them then do not report these flags
@@ -2376,29 +2388,54 @@ NativeKey::WillDispatchKeyboardEvent(Wid
 
     // The mCharCode was set from mKeyValue. However, for example, when Ctrl key
     // is pressed, its value should indicate an ASCII character for backward
     // compatibility rather than inputting character without the modifiers.
     // Therefore, we need to modify mCharCode value here.
     aKeyboardEvent.SetCharCode(uniChar);
   }
 
-  if (skipShiftedChars <= aIndex) {
-    shiftedChar = mShiftedString.mChars[aIndex - skipShiftedChars];
-  }
-  if (skipUnshiftedChars <= aIndex) {
-    unshiftedChar = mUnshiftedString.mChars[aIndex - skipUnshiftedChars];
+  // We need to append alterntaive charCode values:
+  //   - if the event is eKeyPress, we need to append for the index because
+  //     eKeyPress event is dispatched for every character inputted by a
+  //     key press.
+  //   - if the event is not eKeyPress, we need to append for all characters
+  //     inputted by the key press because the other keyboard events (e.g.,
+  //     eKeyDown are eKeyUp) are fired only once for a key press.
+  size_t count;
+  if (aKeyboardEvent.mMessage == eKeyPress) {
+    // Basically, append alternative charCode values only for the index.
+    count = 1;
+    // However, if it's the last eKeyPress event but different shift state
+    // can input longer string, the last eKeyPress event should have all
+    // remaining alternative charCode values.
+    if (isLastIndex) {
+      count = longestLength - aIndex;
+    }
+  } else {
+    count = longestLength;
   }
-
-  if (shiftedChar || unshiftedChar) {
-    AlternativeCharCode chars(unshiftedChar, shiftedChar);
-    altArray.AppendElement(chars);
-  }
-
-  if (aIndex == longestLength - 1) {
+  for (size_t i = 0; i < count; ++i) {
+    uint16_t shiftedChar = 0, unshiftedChar = 0;
+    if (skipShiftedChars <= aIndex + i) {
+      shiftedChar = mShiftedString.mChars[aIndex + i - skipShiftedChars];
+    }
+    if (skipUnshiftedChars <= aIndex + i) {
+      unshiftedChar = mUnshiftedString.mChars[aIndex + i - skipUnshiftedChars];
+    }
+
+    if (shiftedChar || unshiftedChar) {
+      AlternativeCharCode chars(unshiftedChar, shiftedChar);
+      altArray.AppendElement(chars);
+    }
+
+    if (!isLastIndex) {
+      continue;
+    }
+
     if (mUnshiftedLatinChar || mShiftedLatinChar) {
       AlternativeCharCode chars(mUnshiftedLatinChar, mShiftedLatinChar);
       altArray.AppendElement(chars);
     }
 
     // Typically, following virtual keycodes are used for a key which can
     // input the character.  However, these keycodes are also used for
     // other keys on some keyboard layout.  E.g., in spite of Shift+'1'