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.

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

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.

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().

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.

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

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.

Renaming the members added by the previous patch.

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.

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 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 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 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 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 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 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 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 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 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 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?

What bug number to implement SetPendingComposition?

(In reply to Makoto Kato [:m_kato] from comment #28) > What bug number to implement SetPendingComposition? bug 1137572 (attachment 8720205 [details] [diff] [review]).

Found an easy mistake...

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

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.

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

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

Use mDispatcher.get() instead of mDispatcher ?

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

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

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

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