1303273 - NativeKey::mCommittedCharsAndModifiers should be computed with following WM_CHAR messages as far as possible

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

NativeKey::mCommittedCharsAndModifiers is used for setting KeyboardEvent.key value of eKeyDown event and each KeyboardEvent.charCode value of eKeyPress events.  So, this is what will be inputted into editor if eKeyPress events cause some text.

However, it's initialized with KeyboardLayout::InitNativeKey() with KeyboardLayout's information in some cases.  Unfortunately, KeyboardLayout class doesn't support unusual KeyboardLayout, e.g., much complicated dead key sequence, known as non-printable keys mapped as printable keys, etc.  Therefore, changing NativeKey is now very risky for such special keyboard layout users.

If NativeKey decides mCommittedCharsAndModifiers only from following WM_CHAR messages, it becomes safer at least for inputting text.

After fixing bug 1300003, we can do this because NativeKey will store whole following WM_CHAR messages for a WM_KEYDOWN.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc4ce449c9e2

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6044d34a6a4c

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff9b822ee763

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7dcf0a9c2d9

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1303273 part.1 KeyboardLayout::IsSysKey() should check MODIFIER_ALT as a bit flag

This is a simple mistake and blocks following patchs' automated tests.

For example, when Alt+Shift+foo doesn't cause text, this returns false even though it should return true.

Review commit: https://reviewboard.mozilla.org/r/84242/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84242/

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1303273 part.2 KeyboardLayout::InitNativeKey() should initialize aNativeKey.mCommittedCharsAndModifiers with following WM_CHAR or WM_SYSCHAR messages which are not providing a control character

First, mCommittedCharsAndModifiers should be initialized with following char messages because the messages could be different from the information of current keyboard layout.

So, this patch guarantees that mCommittedCharsAndModifiers are same as the user expected when there is one or more WM_CHAR or WM_SYSCHAR messages.

Review commit: https://reviewboard.mozilla.org/r/84244/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84244/

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1303273 part.3 Dispatch eKeyPress events without NativeKey::HandleCharMessage() when it handles WM_(SYS)KEYDOWN message and there are following WM_(SYS)CHAR messages which includes non-control character

This patch creates NativeKey::DispatchKeyPressEventsWithRetrievedCharMessages() for dispatching eKeyPress event with mCommittedCharsAndModifiers when it stores following printable WM_(SYS)CHAR messages.

Using loop for dispatching eKeyPress event for every WM_(SYS)CHAR message is wrong because WidgetKeyboardEvent::mKeyValue is initialized with mCommittedCharsAndModifiers and it causes TextEventDispatcher dispatching multiple eKeyPress events at every call of MaybeDispatchKeypressEvents().  Therefore, if mKeyValue is "^^", eKeyPress event is dispatched 4 times --for the first message, eKeyPress events are fired for each "^" and for the second message, eKeyPress events are fired again for each "^"--.  Therefore, when it handles WM_(SYS)KEYDOWN and it causes inputting one or more printable characters, it's the easiest way not to use HandleCharMessage().

The new method calls TextEventDispatcher::MaybeDispatchKeypressEvents() only once and it requests to call the callback method with new argument of MaybeDispatchKeypressEvents() when it needs to dispatch 2 or more eKeyPress events.  Then, NativeKey::WillDispatchKeyboardEvent() can set each eKeyPress event to raw information of the message and proper modifier state.

With this change, we can dispatch multiple eKeyPress events with retrieved WM_(SYS)CHAR message information rather than retrieved information from active keyboard layout.  Therefore, NeedsToHandleWithoutFollowingCharMessages() doesn't return true even when mCommittedCharsAndModifiers stores two or more characters.

FYI: there is a bug in test_keycodes.xul. That is, Alt+'A' of Greek keyboard layout should cause WM_SYSCHAR with a corresponding Greek character but ASCII characters are specified.  Therefore, this patch includes the fix of these bugs

Review commit: https://reviewboard.mozilla.org/r/84246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84246/

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1303273 part.4 Add automated tests for bug 1293505, bug 1307703 and bug 1297985

Now, NativeKey respects following WM_CHAR message.  Therefore, we can create a test for bug 1293505 which a function key causes a printable character.

Additionally, bug 1307703 is now fixed by the previous patch.  So, let's add automated test for it too.

Finally, now, I found a way to test with some keyboard layouts which are not available on old Windows.  Therefore, we should add automated tests for bug 1297985 too.

Review commit: https://reviewboard.mozilla.org/r/84248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84248/

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1303273 part.5 UniCharsAndModifiers should hide mChars

Now, we have an security issue.  mCommittedCharsAndModifiers may be initialized with multiple WM_(SYS)CHAR messages.  However, if it's generated by odd (or malicious) middleware, mCommittedCharsAndModifiers may be overflown because it has only fixed array.  For fixing this issue, first, we should hide the members for making the users not depend on the design of UniCharsAndModifiers.

This patch changes UniCharsAndModifiers to a class and hiding mChars and adding |CharAt() const|.

Review commit: https://reviewboard.mozilla.org/r/84250/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84250/

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1303273 part.6 Hide UniCharsAndModifiers::mModifiers

Review commit: https://reviewboard.mozilla.org/r/84252/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84252/

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1303273 part.7 Hide UniCharsAndModifiers::mLength and make its type size_t

Review commit: https://reviewboard.mozilla.org/r/84254/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84254/

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1303273 part.8 UniCharsAndModifiers should use nsAutoString and AutoTArray to store characters and modifiers

Fortunately, UniCharsAndModifiers instances are only in stack.  Therefore, we can make it a stack class and use nsAutoString and AutoTArray for not using heap at handling inputs from usual keyboard layouts.

Review commit: https://reviewboard.mozilla.org/r/84256/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84256/

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1303273 part.9 NativeKey::mFollowingCharMsgs should be AutoTArray

Similarly, NativeKey is a stack class and mFollowingCharMsgs which stores following char messages of WM_(SYS)KEYDOWN should be AutoTArray for preventing to use heap at handling inputs with usual keyboard layout.

5 is enough size for handling usual keyboard layout because we support only 5 or less characters per dead key sequence.

Review commit: https://reviewboard.mozilla.org/r/84258/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84258/

Comment 14

[Makoto Kato [:m_kato]]

Comment on attachment 8798869 [details]
Bug 1303273 part.1 KeyboardLayout::IsSysKey() should check MODIFIER_ALT as a bit flag

https://reviewboard.mozilla.org/r/84242/#review83310

::: widget/windows/KeyboardLayout.cpp:3542
(Diff revision 1)
>    }
>  
>    // If the Alt key state isn't consumed, that means that the key with Alt
>    // doesn't cause text input.  So, the combination is a system key.
> -  return inputCharsAndModifiers.mModifiers[0] != MODIFIER_ALT;
> +  return (inputCharsAndModifiers.mModifiers[0] & MODIFIER_ALT) != 0;
>  }

nit: Use "!!(inputCharsAndModifiers.mModifiers[0] & MODIFIER_ALT)" insted.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8798869 [details]
Bug 1303273 part.1 KeyboardLayout::IsSysKey() should check MODIFIER_ALT as a bit flag

https://reviewboard.mozilla.org/r/84242/#review83310

> nit: Use "!!(inputCharsAndModifiers.mModifiers[0] & MODIFIER_ALT)" insted.

Okay, fixed locally. (I'd like it to be in coding style rules because I've changed it from |!!| to |!= 0| in other bugs, though...)

Comment 16

[Makoto Kato [:m_kato]]

Comment on attachment 8798870 [details]
Bug 1303273 part.2 KeyboardLayout::InitNativeKey() should initialize aNativeKey.mCommittedCharsAndModifiers with following WM_CHAR or WM_SYSCHAR messages which are not providing a control character

https://reviewboard.mozilla.org/r/84244/#review83346

Comment 17

[Makoto Kato [:m_kato]]

Comment on attachment 8798871 [details]
Bug 1303273 part.3 Dispatch eKeyPress events without NativeKey::HandleCharMessage() when it handles WM_(SYS)KEYDOWN message and there are following WM_(SYS)CHAR messages which includes non-control character

https://reviewboard.mozilla.org/r/84246/#review83350

Comment 18

[Makoto Kato [:m_kato]]

Comment on attachment 8798872 [details]
Bug 1303273 part.4 Add automated tests for bug 1293505, bug 1307703 and bug 1297985

https://reviewboard.mozilla.org/r/84248/#review83368

Comment 19

[Makoto Kato [:m_kato]]

Comment on attachment 8798873 [details]
Bug 1303273 part.5 UniCharsAndModifiers should hide mChars

https://reviewboard.mozilla.org/r/84250/#review83380

Comment 20

[Makoto Kato [:m_kato]]

Comment on attachment 8798874 [details]
Bug 1303273 part.6 Hide UniCharsAndModifiers::mModifiers

https://reviewboard.mozilla.org/r/84252/#review83388

Comment 21

[Makoto Kato [:m_kato]]

Comment on attachment 8798875 [details]
Bug 1303273 part.7 Hide UniCharsAndModifiers::mLength and make its type size_t

https://reviewboard.mozilla.org/r/84254/#review83394

Comment 22

[Makoto Kato [:m_kato]]

Comment on attachment 8798876 [details]
Bug 1303273 part.8 UniCharsAndModifiers should use nsAutoString and AutoTArray to store characters and modifiers

https://reviewboard.mozilla.org/r/84256/#review83398

Comment 23

[Makoto Kato [:m_kato]]

Comment on attachment 8798877 [details]
Bug 1303273 part.9 NativeKey::mFollowingCharMsgs should be AutoTArray

https://reviewboard.mozilla.org/r/84258/#review83400

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8798869 [details]
Bug 1303273 part.1 KeyboardLayout::IsSysKey() should check MODIFIER_ALT as a bit flag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84242/diff/1-2/

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8798870 [details]
Bug 1303273 part.2 KeyboardLayout::InitNativeKey() should initialize aNativeKey.mCommittedCharsAndModifiers with following WM_CHAR or WM_SYSCHAR messages which are not providing a control character

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84244/diff/1-2/

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8798871 [details]
Bug 1303273 part.3 Dispatch eKeyPress events without NativeKey::HandleCharMessage() when it handles WM_(SYS)KEYDOWN message and there are following WM_(SYS)CHAR messages which includes non-control character

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84246/diff/1-2/

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8798872 [details]
Bug 1303273 part.4 Add automated tests for bug 1293505, bug 1307703 and bug 1297985

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84248/diff/1-2/

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8798873 [details]
Bug 1303273 part.5 UniCharsAndModifiers should hide mChars

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84250/diff/1-2/

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8798874 [details]
Bug 1303273 part.6 Hide UniCharsAndModifiers::mModifiers

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84252/diff/1-2/

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8798875 [details]
Bug 1303273 part.7 Hide UniCharsAndModifiers::mLength and make its type size_t

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84254/diff/1-2/

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8798876 [details]
Bug 1303273 part.8 UniCharsAndModifiers should use nsAutoString and AutoTArray to store characters and modifiers

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84256/diff/1-2/

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8798877 [details]
Bug 1303273 part.9 NativeKey::mFollowingCharMsgs should be AutoTArray

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84258/diff/1-2/

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3446be6bb98
part.1 KeyboardLayout::IsSysKey() should check MODIFIER_ALT as a bit flag r=m_kato
https://hg.mozilla.org/integration/autoland/rev/259115245f85
part.2 KeyboardLayout::InitNativeKey() should initialize aNativeKey.mCommittedCharsAndModifiers with following WM_CHAR or WM_SYSCHAR messages which are not providing a control character r=m_kato
https://hg.mozilla.org/integration/autoland/rev/4e3211aa00ab
part.3 Dispatch eKeyPress events without NativeKey::HandleCharMessage() when it handles WM_(SYS)KEYDOWN message and there are following WM_(SYS)CHAR messages which includes non-control character r=m_kato
https://hg.mozilla.org/integration/autoland/rev/55817ab6a405
part.4 Add automated tests for bug 1293505, bug 1307703 and bug 1297985 r=m_kato
https://hg.mozilla.org/integration/autoland/rev/24569a596578
part.5 UniCharsAndModifiers should hide mChars r=m_kato
https://hg.mozilla.org/integration/autoland/rev/816a22bf25dd
part.6 Hide UniCharsAndModifiers::mModifiers r=m_kato
https://hg.mozilla.org/integration/autoland/rev/65d72f0e62aa
part.7 Hide UniCharsAndModifiers::mLength and make its type size_t r=m_kato
https://hg.mozilla.org/integration/autoland/rev/f34b67c757f7
part.8 UniCharsAndModifiers should use nsAutoString and AutoTArray to store characters and modifiers r=m_kato
https://hg.mozilla.org/integration/autoland/rev/927bfc865b55
part.9 NativeKey::mFollowingCharMsgs should be AutoTArray r=m_kato

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a3446be6bb98
https://hg.mozilla.org/mozilla-central/rev/259115245f85
https://hg.mozilla.org/mozilla-central/rev/4e3211aa00ab
https://hg.mozilla.org/mozilla-central/rev/55817ab6a405
https://hg.mozilla.org/mozilla-central/rev/24569a596578
https://hg.mozilla.org/mozilla-central/rev/816a22bf25dd
https://hg.mozilla.org/mozilla-central/rev/65d72f0e62aa
https://hg.mozilla.org/mozilla-central/rev/f34b67c757f7
https://hg.mozilla.org/mozilla-central/rev/927bfc865b55