1300003 - NativeKey should remove all following WM_CHAR messages at handling WM_KEYDOWN first

Status: RESOLVED FIXED

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

Description

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

NativeKey currently removes WM_CHAR messages from the queue after dispatching eKeyDown event. However, that means that following WM_CHAR messages may be gone during dispatching eKeyDown event, e.g., if it causes entering into modal dialog.

Additionally, we cannot log all WM_CHAR messages when we have two or more characters at a keypress.

For solving these issues, NativeKey should remove all following WM_CHAR messages at starting to handle WM_KEYDOWN and cache them until eKeyPress events are dispatched.

Comment 1

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

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

Comment 2

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

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

Comment 3

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

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

Comment 4

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

Attachment: Bug 1300003 part.1 NativeKey should remove following char messages before dispatching a keydown event

In some cases, we may receive following char message of WM_*KEYDOWN *during* dispatching an eKeyDown event. In such case, NativeKey shouldn't dispatch eKeyPress event twice (one is caused by receiving WM_*CHAR message, the other is caused by post dispatching eKeyDown event code).

This patch makes NativeKey consume following WM_*CHAR messages before dispatching eKeyDown.  Therefore, the former case will never occur.

For doing that, NativeKey stores following WM_*CHAR messages in mFollowingCharMsgs and uses it when it needs to dispatch eKeyPress events for each WM_*CHAR message.

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

Comment 5

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

Attachment: Bug 1300003 part.2 Don't continue to dispatch eKeyPress event at handling WM_KEYDOWN or following WM_CHAR messages if focused window is changed during dispatching an event

While dispatching an event, focused widget may be changed.  In such case, NativeKey shouldn't continue to dispatch remaining events (eKeyPress events) for preventing to dispatch to input text in unexpected website.

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

Comment 6

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

Attachment: Bug 1300003 part.3 NativeKey::GetFollowingCharMessage() should always remove following WM_CHAR message

GetFollowingCharMessage() is now called only when removing following char message.  Therefore, it does not need to have the optional argument indicating if removing the found message.

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

Comment 7

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

Attachment: Bug 1300003 part.4 Remove NativeKey::mIsFollowedByNonControlCharMessage because calling NativeKey::IsFollowedByNonControlCharMessage() is enough fast

NativeKey is now removing and storing following char messages when it's created for a keydown message.  Therefore, IsFollowedByNonControlCharMessage() just refers the stored messages and it's enough fast.  So, we can get rid of mIsFollowedByNonControlCharMessage.

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

Comment 8

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

Attachment: Bug 1300003 part.5 Remove odd WM_CHAR messages which are caused by ATOK or WXG (both of them are Japanese IME)

NativeKey removes odd WM_CHAR messages which are caused by ATOK or WXG if the user tries to do "Kakutei-Undo" (meaning "undo the last commit") after dispatching eKeyDown event.

However, NativeKey should remove them from the queue first because there are some problems:

* If focus is moved during dispatching an eKeyDown event, we'll fail to remove the messages from the queue.
* If dispatching eKeyDown advance native event loop due to entering to a modal loop or dosing synchronous XHR, the message pattern is broken before removing the odd messages from the queue.

After removing the odd char messages, NativeKey should store them with mRemovedOddCharMsgs because NativeKey needs to dispatch them to focused windowless plugin.

Note that this patch also fixes a bug of the loop to remove the odd WM_CHAR messages.  Old code removes only one WM_CHAR messages, but we need to remove all WM_CHAR messages before WM_KEYUP.  This must be a regression.

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

Comment 9

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

Attachment: Bug 1300003 part.6 NativeKey shouldn't try to dispatch plugin events for removed char messages when mWidget won't dispatch plugin events

Currently, NativeKey::DispatchPluginEventsAndDiscardsCharMessages() calls nsWindowBase::DispatchPluginEvent() and nsWindowBase::DispatchPluginEvent() does nothing when windowless plugin doesn't have focus.  However, this is unclear when other developers read the code of DispatchPluginEventsAndDiscardsCharMessages() and it causes unnecessary virtual calls.  So, it should try to dispatch plugin events only when a windowless plugin has focus.

For making the check safer, nsWindowBase should expose the method to check it as ShouldDispatchPluginEvent().  Unfortunately, we cannot mark this method as const due to PluginHasFocus() needs to be not const method, though.  Then, for loops in DispatchPluginEventsAndDiscardsCharMessages() should check it at each try.

This change helps to log the behavior (working on bug 1297013) without noise.

Additionally, this patch renames the method to MaybeDispatchPluginEventsForRemovedCharMessages() because it doesn't remove any char messages anymore.

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

Comment 10

[Makoto Kato [:m_kato]]

Comment on attachment 8791563 [details]
Bug 1300003 part.1 NativeKey should remove following char messages before dispatching a keydown event

https://reviewboard.mozilla.org/r/78972/#review77768

Comment 11

[Makoto Kato [:m_kato]]

Comment on attachment 8791564 [details]
Bug 1300003 part.2 Don't continue to dispatch eKeyPress event at handling WM_KEYDOWN or following WM_CHAR messages if focused window is changed during dispatching an event

https://reviewboard.mozilla.org/r/78974/#review78284

Comment 12

[Makoto Kato [:m_kato]]

Comment on attachment 8791565 [details]
Bug 1300003 part.3 NativeKey::GetFollowingCharMessage() should always remove following WM_CHAR message

https://reviewboard.mozilla.org/r/78976/#review78298

Comment 13

[Makoto Kato [:m_kato]]

Comment on attachment 8791566 [details]
Bug 1300003 part.4 Remove NativeKey::mIsFollowedByNonControlCharMessage because calling NativeKey::IsFollowedByNonControlCharMessage() is enough fast

https://reviewboard.mozilla.org/r/78978/#review78300

Comment 14

[Makoto Kato [:m_kato]]

Comment on attachment 8791567 [details]
Bug 1300003 part.5 Remove odd WM_CHAR messages which are caused by ATOK or WXG (both of them are Japanese IME)

https://reviewboard.mozilla.org/r/78980/#review78364

Comment 15

[Makoto Kato [:m_kato]]

Comment on attachment 8791568 [details]
Bug 1300003 part.6 NativeKey shouldn't try to dispatch plugin events for removed char messages when mWidget won't dispatch plugin events

https://reviewboard.mozilla.org/r/78982/#review78374

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1c503e563219
part.1 NativeKey should remove following char messages before dispatching a keydown event r=m_kato
https://hg.mozilla.org/integration/autoland/rev/bf88f2c838e2
part.2 Don't continue to dispatch eKeyPress event at handling WM_KEYDOWN or following WM_CHAR messages if focused window is changed during dispatching an event r=m_kato
https://hg.mozilla.org/integration/autoland/rev/e4426fbaff8e
part.3 NativeKey::GetFollowingCharMessage() should always remove following WM_CHAR message r=m_kato
https://hg.mozilla.org/integration/autoland/rev/4eb2eab93848
part.4 Remove NativeKey::mIsFollowedByNonControlCharMessage because calling NativeKey::IsFollowedByNonControlCharMessage() is enough fast r=m_kato
https://hg.mozilla.org/integration/autoland/rev/4364160065bc
part.5 Remove odd WM_CHAR messages which are caused by ATOK or WXG (both of them are Japanese IME) r=m_kato
https://hg.mozilla.org/integration/autoland/rev/949fe298aefd
part.6 NativeKey shouldn't try to dispatch plugin events for removed char messages when mWidget won't dispatch plugin events r=m_kato

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1c503e563219
https://hg.mozilla.org/mozilla-central/rev/bf88f2c838e2
https://hg.mozilla.org/mozilla-central/rev/e4426fbaff8e
https://hg.mozilla.org/mozilla-central/rev/4eb2eab93848
https://hg.mozilla.org/mozilla-central/rev/4364160065bc
https://hg.mozilla.org/mozilla-central/rev/949fe298aefd

Comment 18

[Gerry Chang [:gchang]]

Mark 51 as fix-optional. If it's worth uplift to 51, feel free to nominate it.