1137561 - Make KeyboardLayout, nsIMM32Handler and nsTextStore should use TextEventDispatcher

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

On Windows, we dispatch keyboard events from KeyboardLayout and composition events from nsIMM32Handler and nsTextStore. We should make them use TextEventDispatcher for managing the behavior in XP level.

Comment 1

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

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

Comment 2

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

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

Comment 3

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

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

Comment 4

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

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

Comment 5

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

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

Comment 6

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

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

Comment 7

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

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

Comment 8

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

Attachment: part.1 Implement WinTextEventDispatcherListener as a singleton class for using TextEventDispatcher at handling native keyboard and IME events

On Windows, we don't need to support multiple IME contexts. So, TextEventDispatcherListener can be singleton.

Comment 9

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

Attachment: part.2 Make TSFTextStore use TextEventDispatcher

TSFTextStore depends on one widget. So, it can store TextEventDispatcher (which is created per widget).

And also, on Windows, WidgetEvent::timeStamp is computed with current message's time. So, we need to create the method to compute timeStamp in nsWindow.

Comment 10

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

Attachment: part.3 Make IMMHandler use TextEventDispatcher

IMMHandler is a singleton class. So, it may related to multiple nsWindow instances at once. When nsWindow receives WM_* message, IMMHandler handles the message with received nsWindow instance.

IMMHandler should store TextEventDispatcher for composing window for quicker access but it needs to check if handling window is composing window with GetTextEventDispatcherFor().

Comment 11

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

Attachment: part.4 Make NativeKey use TextEventDispatcher

NativeKey is created at handling a WM_*KEYDOWN, WM_*KEYUP or WM_*CHAR in the stack. So, storing TextEventDispatcher at initializing makes the code simpler.

This patch just makes NativeKey use TextEventDispatcher but this won't work perfectly without following patches.

Comment 12

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

Attachment: part.5 TextEventDispatcher should decide if keypress events should be fired for specific keys

Each widget shouldn't decide which key should or shouldn't cause keypress events. TextEventDispatcher should decide it when TextEventDispatcher::MaybeDispatchKeypressEvents() is called.

Comment 13

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

Attachment: part.6 Store some strings which may be inputted by the key with some modifier state before dispatching keydown event

charCode at pressing Ctrl key and alternative char codes should be set when WinTextEventDispatcherListener::WillDispatchKeyboardEvent(). For using charCode information in the callback method, all necessary information should be stored into member variables of NativeKey.

Note that for making this simpler diff file, the members are not named like mFoo. They will be renamed by following patch.

Comment 14

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

Attachment: part.7 Rename whole members added by the previous patch

Renaming the members added by the previous patch.

Comment 15

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

Attachment: part.8 Implement WinTextEventDispatcherListener::WillDispatchKeyboardEvent()

This patch reimplement NativeKey::DispatchKeyPressEventsWithKeyboardLayout() as callback method, TextEventDispatcherListener::WillDispatchKeyboardEvent().

Note that most changes of this patch is unindentation. I'll post -w diff file of this patch for you.

Comment 16

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

Attachment: part.8 Implement WinTextEventDispatcherListener::WillDispatchKeyboardEvent() (-w)

Comment 17

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

Attachment: part.9 NativeKey should dispatch keypress events after removing following char messages if there are two or more characters to be inputted

When a keydown event causes multiple characters (e.g., dead key is used with invalid combination), current code handles WM_CHAR directly.

However, TextEventDispatcher::MaybeDispatchKeypressEvents() dispatches whole keypress events at handling WM_KEYDOWN. Of course, this causes duplicated inputs except the first character.

For avoiding this, at handling WM_KEYDOWN, NativeKey needs to remove all following WM_CHAR messages from the queue.

Comment 18

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

Comment on attachment 8720247 [details] [diff] [review]
part.1 Implement WinTextEventDispatcherListener as a singleton class for using TextEventDispatcher at handling native keyboard and IME events

Hi, Makoto-san, I'd like you to review the series of patches for refactoring keyboard event and composition event dispatchers.

Current strategy is, all native event handlers should use TextEventDispatcher for consistent behavior on all platforms. For example, don't dispatch keyboard events during composition, don't dispatch keypress event(s) for specific keys, etc. So, this reduces maintenance cost of updating keyboard event behavior for conforming new draft of UI Events and something similar related specs and improving compatibility with the other browsers.

First, users of TextEventDispatcher need to create an input transaction before dispatching every event since rights of created input transaction may be stolen by automated test or add-ons which use nsITextInputProcessor.

At creating the input transaction, TextEventDispatcher needs an instance of TextEventDispatcherListener which receives some notifications from TextEventDispatcher.

On Windows, we don't support multiple HIMC per process. So, we need only one instance of TextEventDispatcherListener at creating an input transaction.

Comment 19

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

Comment on attachment 8720251 [details] [diff] [review]
part.2 Make TSFTextStore use TextEventDispatcher

TSFTextStore depends on one widget. So, it can store TextEventDispatcher (which is created per widget).

And also, on Windows, WidgetEvent::timeStamp is computed with current message's time. So, we need to create the method to compute timeStamp in nsWindow.

Comment 20

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

Comment on attachment 8720254 [details] [diff] [review]
part.3 Make IMMHandler use TextEventDispatcher

IMMHandler is a singleton class. So, it may related to multiple nsWindow instances at once. When nsWindow receives WM_* message, IMMHandler handles the message with received nsWindow instance.

IMMHandler should store TextEventDispatcher for composing window for quicker access but it needs to check if handling window is composing window with GetTextEventDispatcherFor().

Comment 21

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

Comment on attachment 8720255 [details] [diff] [review]
part.4 Make NativeKey use TextEventDispatcher

NativeKey is created at handling a WM_*KEYDOWN, WM_*KEYUP or WM_*CHAR in the stack. So, storing TextEventDispatcher at initializing makes the code simpler.

This patch just makes NativeKey use TextEventDispatcher but this won't work perfectly without following patches.

Comment 22

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

Comment on attachment 8720258 [details] [diff] [review]
part.5 TextEventDispatcher should decide if keypress events should be fired for specific keys

Each widget shouldn't decide which key should or shouldn't cause keypress events. TextEventDispatcher should decide it when TextEventDispatcher::MaybeDispatchKeypressEvents() is called.

Comment 23

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

Comment on attachment 8720259 [details] [diff] [review]
part.6 Store some strings which may be inputted by the key with some modifier state before dispatching keydown event

charCode at pressing Ctrl key and alternative char codes should be set when WinTextEventDispatcherListener::WillDispatchKeyboardEvent(). For using charCode information in the callback method, all necessary information should be stored into member variables of NativeKey.

Note that for making this simpler diff file, the members are not named like mFoo. They will be renamed by following patch.

Comment 24

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

Comment on attachment 8720260 [details] [diff] [review]
part.7 Rename whole members added by the previous patch

Renaming the members added by the previous patch.

Comment 25

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

Comment on attachment 8720261 [details] [diff] [review]
part.8 Implement WinTextEventDispatcherListener::WillDispatchKeyboardEvent()

This patch reimplement NativeKey::DispatchKeyPressEventsWithKeyboardLayout() as callback method, TextEventDispatcherListener::WillDispatchKeyboardEvent().

Note that most changes of this patch is unindentation. See attachment 8720262 [details] [diff] [review] for the actual difference (i.e., except the white space changes).

Comment 26

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

Comment on attachment 8720263 [details] [diff] [review]
part.9 NativeKey should dispatch keypress events after removing following char messages if there are two or more characters to be inputted

When a keydown event causes multiple characters (e.g., dead key is used with invalid combination, typing same dead char like '`' twice), current code handles multiple WM_CHAR messages directly.

However, TextEventDispatcher::MaybeDispatchKeypressEvents() dispatches whole keypress events at handling WM_KEYDOWN because WidgetKeyboardEvent::mKeyValue has all characters which will be inputted with multiple WM_CHAR messages. Of course, this causes duplicated inputs except the first character.

For avoiding this, at handling WM_KEYDOWN, NativeKey needs to remove all following WM_CHAR messages from the queue.

Comment 27

[Makoto Kato [:m_kato]]

Comment on attachment 8720247 [details] [diff] [review]
part.1 Implement WinTextEventDispatcherListener as a singleton class for using TextEventDispatcher at handling native keyboard and IME events

Review of attachment 8720247 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/WinTextEventDispatcherListener.h
@@ +18,5 @@
> + * because we have only one input context per process (IMM can create
> + * multiple IM context but we don't support such behavior).
> + */
> +
> +class WinTextEventDispatcherListener : public TextEventDispatcherListener

add final keyword?

Comment 28

[Makoto Kato [:m_kato]]

What bug number to implement SetPendingComposition?

Comment 29

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

(In reply to Makoto Kato [:m_kato] from comment #28)
> What bug number to implement SetPendingComposition?

bug 1137572 (attachment 8720205 [details] [diff] [review]).

Comment 30

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

Attachment: part.1 Implement WinTextEventDispatcherListener as a singleton class for using TextEventDispatcher at handling native keyboard and IME events (r=m_kato)

Comment 31

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

Attachment: part.8 Implement WinTextEventDispatcherListener::WillDispatchKeyboardEvent()

Found an easy mistake...

Comment 32

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

Attachment: part.8 Implement WinTextEventDispatcherListener::WillDispatchKeyboardEvent() (-w)

Comment 33

[Makoto Kato [:m_kato]]

Comment on attachment 8720259 [details] [diff] [review]
part.6 Store some strings which may be inputted by the key with some modifier state before dispatching keydown event

Review of attachment 8720259 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/KeyboardLayout.h
@@ +514,5 @@
>     */
>    bool NeedsToHandleWithoutFollowingCharMessages() const;
> +
> +  /**
> +   * ComputeInputtingStringWithKeyboardLayout() computes 

remove tail space

Comment 34

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

Thank you, Makoto-san for your review!

My plan to land them is, after next merge (for risk management). So, if you have some urgent work for 47, you can put off the remaining reviews to the next week.

Comment 35

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/807b3c1dfc4799e019a04f2739c8e0f56bd0b6f1
Bug 1137561 part.1 Implement WinTextEventDispatcherListener as a singleton class for using TextEventDispatcher at handling native keyboard and IME events r=m_kato

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9ffc169e99cf0006cdc29810e1b41f884a1ff53
Bug 1137561 part.2 Make TSFTextStore use TextEventDispatcher r=m_kato

https://hg.mozilla.org/integration/mozilla-inbound/rev/24f0cbec55c3dc87a826145e584aa27da2d6be2f
Bug 1137561 part.3 Make IMMHandler use TextEventDispatcher r=m_kato

https://hg.mozilla.org/integration/mozilla-inbound/rev/545efe0fba8180b3443cbd9a21d7a60dcb68bfaf
Bug 1137561 part.4 Make NativeKey use TextEventDispatcher r=m_kato

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8d76794d055c60fc478896d47189e0581aae31f
Bug 1137561 part.5 TextEventDispatcher should decide if keypress events should be fired for specific keys r=m_kato

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b5b23d624d3c3f107dfde59aebe94ea03a9480e
Bug 1137561 part.6 Store some strings which may be inputted by the key with some modifier state before dispatching keydown event r=m_kato

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0d6c46f07ba0a04a20ddbdc5e991b37b86fe118
Bug 1137561 part.7 Rename whole members added by the previous patch r=m_kato

https://hg.mozilla.org/integration/mozilla-inbound/rev/5119dfa69d3063d2cd866ef75edbc34664d3b70e
Bug 1137561 part.8 Implement WinTextEventDispatcherListener::WillDispatchKeyboardEvent() r=m_kato

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c1f6fd7016cedd8a2cc775212bb49cc57aba308
Bug 1137561 part.9 NativeKey should dispatch keypress events after removing following char messages if there are two or more characters to be inputted r=m_kato

Comment 36

[Makoto Kato [:m_kato]]

Maybe, this causes bustage on VS2015 Update 1.

 1:10.58 c:/Development/hg.mozilla.org/mozilla-inbound/widget/windows/IMMHandler.cpp(1269): error C2662: 'RefPtr<mozilla::widget::TextEventDispatcher>::operator T(void) const &&': cannot convert 'this' pointer from 'RefPtr<mozilla::widget::TextEventDispatcher>' to 'const RefPtr<mozilla::widget::TextEventDispatcher> &&'
 1:10.58         with
 1:10.58         [
 1:10.58             T=mozilla::widget::TextEventDispatcher
 1:10.58         ]
 1:10.58 c:/Development/hg.mozilla.org/mozilla-inbound/widget/windows/IMMHandler.cpp(1269): note: You cannot bind an lvalue to an rvalue reference
 1:10.58

Comment 37

[Makoto Kato [:m_kato]]

Use mDispatcher.get() instead of mDispatcher ?

Comment 38

[Makoto Kato [:m_kato]]

Attachment: Follow up fix for VS2015

Comment 39

[Makoto Kato [:m_kato]]

Comment on attachment 8731137 [details] [diff] [review]
Follow up fix for VS2015

Follow up for VS 2015.

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/807b3c1dfc47
https://hg.mozilla.org/mozilla-central/rev/a9ffc169e99c
https://hg.mozilla.org/mozilla-central/rev/24f0cbec55c3
https://hg.mozilla.org/mozilla-central/rev/545efe0fba81
https://hg.mozilla.org/mozilla-central/rev/b8d76794d055
https://hg.mozilla.org/mozilla-central/rev/3b5b23d624d3
https://hg.mozilla.org/mozilla-central/rev/a0d6c46f07ba
https://hg.mozilla.org/mozilla-central/rev/5119dfa69d30
https://hg.mozilla.org/mozilla-central/rev/2c1f6fd7016c

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

As the patches on this bug have been merged to m-c already, please move the VS 2015 fix to bug 1257377.

Comment 42

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

Comment on attachment 8731137 [details] [diff] [review]
Follow up fix for VS2015

Thanks!

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22b32ab5ce8f

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/22b32ab5ce8f