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.

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.

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

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

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.

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

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

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

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