Make KeyboardLayout, nsIMM32Handler and nsTextStore should use TextEventDispatcher

RESOLVED FIXED in Firefox 48

Status

()

Core
Widget: Win32
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({inputmethod})

Trunk
mozilla48
x86_64
Windows 8.1
inputmethod
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(11 attachments, 3 obsolete attachments)

19.24 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
18.67 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
19.44 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
2.29 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
11.48 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
12.78 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
3.15 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
11.80 KB, patch
Details | Diff | Splinter Review
14.46 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
11.52 KB, patch
Details | Diff | Splinter Review
1.01 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
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.
Depends on: 1137572
Depends on: 1229693
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea2b43e0bd52
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1999ce3b7ff8
https://treeherder.mozilla.org/#/jobs?repo=try&revision=57c9cae26a7e
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe15f8335e3c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc381d44ced0
https://treeherder.mozilla.org/#/jobs?repo=try&revision=243944a1b3f7
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b2687fcccc1
Created attachment 8720247 [details] [diff] [review]
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.
Status: NEW → ASSIGNED
Created 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.
Created 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().
Created 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.
Attachment #8720255 - Attachment description: Make NativeKey use TextEventDispatcher → part.4 Make NativeKey use TextEventDispatcher
Created 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.
Created 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.
Created attachment 8720260 [details] [diff] [review]
part.7 Rename whole members added by the previous patch

Renaming the members added by the previous patch.
Created 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. I'll post -w diff file of this patch for you.
Created attachment 8720262 [details] [diff] [review]
part.8 Implement WinTextEventDispatcherListener::WillDispatchKeyboardEvent() (-w)
Created 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), 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.
Attachment #8720247 - Flags: review?(m_kato)
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.
Attachment #8720251 - Flags: review?(m_kato)
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().
Attachment #8720254 - Flags: review?(m_kato)
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.
Attachment #8720255 - Flags: review?(m_kato)
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.
Attachment #8720258 - Flags: review?(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

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.
Attachment #8720259 - Flags: review?(m_kato)
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.
Attachment #8720260 - Flags: review?(m_kato)
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).
Attachment #8720261 - Flags: review?(m_kato)
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.
Attachment #8720263 - Flags: review?(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?
Attachment #8720247 - Flags: review?(m_kato) → review+
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]).
Created attachment 8723962 [details] [diff] [review]
part.1 Implement WinTextEventDispatcherListener as a singleton class for using TextEventDispatcher at handling native keyboard and IME events (r=m_kato)
Attachment #8720247 - Attachment is obsolete: true
Created attachment 8724022 [details] [diff] [review]
part.8 Implement WinTextEventDispatcherListener::WillDispatchKeyboardEvent()

Found an easy mistake...
Attachment #8720261 - Attachment is obsolete: true
Attachment #8720261 - Flags: review?(m_kato)
Attachment #8724022 - Flags: review?(m_kato)
Created attachment 8724026 [details] [diff] [review]
part.8 Implement WinTextEventDispatcherListener::WillDispatchKeyboardEvent() (-w)
Attachment #8720262 - Attachment is obsolete: true
Attachment #8724026 - Flags: review?(m_kato)
Attachment #8724026 - Flags: review?(m_kato)
Attachment #8720254 - Flags: review?(m_kato) → review+
Attachment #8720255 - Flags: review?(m_kato) → review+
Attachment #8720258 - Flags: review?(m_kato) → review+
Attachment #8720251 - Flags: review?(m_kato) → review+
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
Attachment #8720259 - Flags: review?(m_kato) → review+
Attachment #8720260 - Flags: review?(m_kato) → review+
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.
Attachment #8724022 - Flags: review?(m_kato) → review+
Attachment #8720263 - Flags: review?(m_kato) → review+
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 ?
Created attachment 8731137 [details] [diff] [review]
Follow up fix for VS2015
Comment on attachment 8731137 [details] [diff] [review]
Follow up fix for VS2015

Follow up for VS 2015.
Attachment #8731137 - Flags: review?(masayuki)

Comment 40

2 years ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1257377
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!
Attachment #8731137 - Flags: review?(masayuki) → review+
Duplicate of this bug: 1257377

Comment 44

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

Comment 45

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/22b32ab5ce8f

Updated

2 years ago
Depends on: 1257479

Updated

2 years ago
Duplicate of this bug: 1257479

Updated

2 years ago
No longer depends on: 1257479
Depends on: 1258153

Updated

2 years ago
Depends on: 1263389
Depends on: 1261880
See Also: → bug 1293505
See Also: → bug 1297985
Depends on: 1297985
See Also: bug 1297985
Depends on: 1293505
See Also: bug 1293505
Depends on: 1293638

Updated

a year ago
Depends on: 1306868
Blocks: 914133
You need to log in before you can comment on or make changes to this bug.