1137563 - Make TextInputHandler use TextEventDispatcher

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

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

Currently, TextInputHandler dispatches both keyboard events and composition events directly. However, using TextEventDispatcher, some code can be shared in XP level. So, we should do it.

Comment 1

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

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

Comment 2

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

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

Comment 3

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

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

Comment 4

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

Attachment: part.1 Implement TextEventDispatcherListener in TextEventInputHandlerBase and IMEInputHandler

On Mac, TextInputHandler (<- IMEInputHandler <- TextInputHandlerBase) handles both IME and keyboard events and it's created per nsChildView. So, it's a good class to implement TextEventDispatcherListener.

Note that nsCocoaWindow which represents top level window doesn't handle input events. So, it never has TextInputHandler. Therefore, it's okay nsCocoaWindow::GetTextEventDispatcherListener() to return nullptr.

Comment 5

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

Attachment: part.2 IMEInputHandler should use TextEventDispatcher

TextEventDispatcher should be stored strongly by TextInputHandlerBase which is the most super class of TextInputHandler. Strong reference makes callers of the methods of TextEventDispatcher simpler (only necessary for kungFuDeathGrip of TextInputHandler itself, though).

Comment 6

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

Attachment: part.3 TextInputHandler should use TextEventDispatcher

This patch makes TextInputHandler use TextEventDispatcher at dispatching WidgetKeyboardEvents simply. I'll post -w patch of this for you.

Comment 7

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

Attachment: part.3 TextInputHandler should use TextEventDispatcher (-w)

Comment 8

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

Attachment: part.4 Implement IMEInputHandler::WillDispatchKeyboardEvent()

This patch implements TextEventDispatcherListener::WillDispatchKeyboardEvent().

On Mac, when a keydown event or something other input causes text input, InsertText() is called with actual text. We need to store the text into a KeyEventState instance which stores handling keyboard event information (Note that Cocoa API allows nested keyboard event handling. Actually, ATOK does this at Kakutei-Undo). Then, we can know the string even with WillDispatchKeyboardEvent() which is a callback at dispatching WidgetKeyboardEvent.

Note that InitKeyEvent() won't set charCode because it's automatically computed by TextEventDispatcher with mKeyNameIndex and mKeyValue (and if it's necessary to overwrite due to platform specific rules, it's done by WillDispatchKeyboardEvent()). Therefore, IsNormalCharInputtingEvent() needs to check if it's a printable key without charCode value (and isChar).

Note that I also fixes a bug in test_keycodes.xul here. The [aNativeKeyEvent characters] becomes "&;". So, the ";" is unexpected character, just a typo. This is now causes additional keypress event because the value is set to WidgetKeyboardEvent.mKeyValue and keypress events are dispatched for every character in it by TextEventDispatcher.

Comment 9

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

Attachment: part.5 Set charCode of dead key's keypress event on Mac to the dead char

I realized that KeyboardEvent.key value (WidgetKeyboardEvent::mKeyNameIndex and WidgetKeyboardEvent::mKeyValue) is wrong if the key event is caused by dead key.

If it's a dead key event, the key value should be "Dead" but TextInputHandler sets [aNativeKeyEvent unmodifiedCharacters]. Therefore, TextEventDispatcher tries to dispatch with the unmodified key's character (e.g., Option + e in US should cause "`" like character, but tried to be dispatched with "e"). Then, the charCode should be replaced with the string which will be inputted as composition string (this can be retrieved with TranslateToChar()) in WillDispatchKeyboardEvent().

Therefore, this patch changes current behavior for automated tests.

However, keypress events shouldn't be fired when dead key doesn't commit any text (that's the behavior on other platforms and other browsers).

So, I'll fix this dead key issue completely in another bug. But in this bug, I just need to fix new orange. Therefore, I need this patch even though this changes the behavior. (Without this patch, we will dispatch odd keypress event whose keyCode and charCode are 0.)

Comment 10

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

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

Comment 11

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

Comment on attachment 8720587 [details] [diff] [review]
part.1 Implement TextEventDispatcherListener in TextEventInputHandlerBase and IMEInputHandler

On Mac, TextInputHandler (<- IMEInputHandler <- TextInputHandlerBase) handles both IME and keyboard events and it's created per nsChildView. So, it's a good class to implement TextEventDispatcherListener.

Note that nsCocoaWindow which represents top level window doesn't handle input events. So, it never has TextInputHandler. Therefore, it's okay nsCocoaWindow::GetTextEventDispatcherListener() to return nullptr.

Comment 12

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

Comment on attachment 8720591 [details] [diff] [review]
part.2 IMEInputHandler should use TextEventDispatcher

TextEventDispatcher should be stored strongly by TextInputHandlerBase which is the most super class of TextInputHandler. Strong reference makes callers of the methods of TextEventDispatcher simpler (only necessary for kungFuDeathGrip of TextInputHandler itself, though).

Comment 13

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

Comment on attachment 8720592 [details] [diff] [review]
part.3 TextInputHandler should use TextEventDispatcher

This patch makes TextInputHandler use TextEventDispatcher at dispatching WidgetKeyboardEvents simply.

See attachment 8720592 [details] [diff] [review] for checking actual changes except only whitespaces changes.

Comment 14

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

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #13)
> See attachment 8720592 [details] [diff] [review] for checking actual changes
> except only whitespaces changes.

Oops, I meant attachment 8720593 [details] [diff] [review].

Comment 15

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

Comment on attachment 8720596 [details] [diff] [review]
part.4 Implement IMEInputHandler::WillDispatchKeyboardEvent()

This patch implements TextEventDispatcherListener::WillDispatchKeyboardEvent().

On Mac, when a keydown event or something other input causes text input, InsertText() is called with actual text. We need to store the text into a KeyEventState instance which stores handling keyboard event information (Note that Cocoa API allows nested keyboard event handling. Actually, ATOK does this at Kakutei-Undo). Then, we can know the string even with WillDispatchKeyboardEvent() which is a callback at dispatching WidgetKeyboardEvent.

Note that InitKeyEvent() won't set charCode because it's automatically computed by TextEventDispatcher with mKeyNameIndex and mKeyValue (and if it's necessary to overwrite due to platform specific rules, it's done by WillDispatchKeyboardEvent()). Therefore, IsNormalCharInputtingEvent() needs to check if it's a printable key without charCode value (and isChar).

Note that I also fixes a bug in test_keycodes.xul here. The [aNativeKeyEvent characters] becomes "&;". So, the ";" is unexpected character, just a typo. This is now causes additional keypress event because the value is set to WidgetKeyboardEvent.mKeyValue and keypress events are dispatched for every character in it by TextEventDispatcher.

Comment 16

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

Comment on attachment 8720603 [details] [diff] [review]
part.5 Set charCode of dead key's keypress event on Mac to the dead char

I realized that KeyboardEvent.key value (WidgetKeyboardEvent::mKeyNameIndex and WidgetKeyboardEvent::mKeyValue) is wrong if the key event is caused by dead key.

If it's a dead key event, the key value should be "Dead" but TextInputHandler sets [aNativeKeyEvent unmodifiedCharacters]. Therefore, TextEventDispatcher tries to dispatch with the unmodified key's character (e.g., Option + e in US should cause "`" like character, but tried to be dispatched with "e"). Then, the charCode should be replaced with the string which will be inputted as composition string (this can be retrieved with TranslateToChar()) in WillDispatchKeyboardEvent().

Therefore, this patch changes current behavior for automated tests.

However, keypress events shouldn't be fired when dead key doesn't commit any text (that's the behavior on other platforms and other browsers).

So, I'll fix this dead key issue completely in bug 1249184. But in this bug, I just need to fix new orange. Therefore, I need this patch even though this changes the behavior. (Without this patch, we will dispatch odd keypress event whose keyCode and charCode are 0.)

Comment 17

[Makoto Kato [:m_kato]]

Comment on attachment 8720596 [details] [diff] [review]
part.4 Implement IMEInputHandler::WillDispatchKeyboardEvent()

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

::: widget/cocoa/TextInputHandler.h
@@ +310,5 @@
>     *
> +   * @param aNativeKeyEvent     The native key event which causes our keyboard
> +   *                            event(s).
> +   * @param aKeyEvent             A Gecko key event which was partially
> +   *                              initialized with aNativeKeyEvent.

fix indent?

@@ +587,5 @@
>    private:
>      RefPtr<TextInputHandlerBase> mHandler;
>    };
>  
> +  class AutoInsertStringClearer

Could you add MOZ_STACK_CLASS?

Comment 18

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

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

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e4ee08ece89e8f9906203f7bdba0eacf17aa3c5
Bug 1137563 part.1 Implement TextEventDispatcherListener in TextEventInputHandlerBase and IMEInputHandler r=m_kato

https://hg.mozilla.org/integration/mozilla-inbound/rev/46411dbcf7902202689f16dc4a5f2d91b1432108
Bug 1137563 part.2 IMEInputHandler should use TextEventDispatcher r=m_kato

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8b11f1455eeb17e50a8553365428cd6f79c0c79
Bug 1137563 part.3 TextInputHandler should use TextEventDispatcher r=m_kato

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0f93cbdbc17f7ea0b8b533f6e0d93845bef96c0
Bug 1137563 part.4 Implement IMEInputHandler::WillDispatchKeyboardEvent() r=m_kato

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb7c36e2ef5d48262bc8566da9ea37623e7d0883
Bug 1137563 part.5 Set charCode of dead key's keypress event on Mac to the dead char r=m_kato

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1e4ee08ece89
https://hg.mozilla.org/mozilla-central/rev/46411dbcf790
https://hg.mozilla.org/mozilla-central/rev/d8b11f1455ee
https://hg.mozilla.org/mozilla-central/rev/c0f93cbdbc17
https://hg.mozilla.org/mozilla-central/rev/eb7c36e2ef5d