1334594 - Using the OS X Hiragana IME, moving the cursor mid-composition causes a compositionend to be fired but the user cannot keep typing until pressing enter.

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[charleyroy]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.95 Safari/537.36

Steps to reproduce:

Note: I am running OS X 10.12.2 to reproduce this. 

For testing purposes, I've created this fiddle: https://jsfiddle.net/fuoexep0/2/

0. Open developer console
1. Add content to the contenteditable div (eg. 'aaaaaa')
2. Switch to the OS X Hiragana IME.
3. Set cursor to end of contenteditable div.
4. Press 'n' followed by 'e' to create a Japanese composition.
5. Click within the contenteditable div to move the cursor.
-- Observe at this point that the compositionend event has been fired and the cursor has moved --
6. Try typing Hiragana (eg. pressing 'n' followed by 'e' again)


Actual results:

No text appears at the new cursor. If you keep typing, the IME suggestion box eventually re-appears where you were initially editing.

Note 1: This doesn't happen when using the Zhuyin IME (eg. you can use '1' + '8' as a testing string, this starts a composition but then moving the cursor allows the user to type at the new location)

Note 2: This doesn't happen on Windows 7 + Firefox 51 branch using the Microsoft Hiragana IME.

Note 3: The compositionend events for both Hiragana and Zhuyin both seem similar, however the Hiragana compositionend's timeStamp field is 0 when this bug occurs.


Expected results:

The initial composition should have been committed and the user should be able to type  at the new cursor position.

Comment 1

[charleyroy]

Also note that I cannot reproduce this on TextEdit, which is what led me to believe this was a browser-based issue.

Comment 2

[Steven Saviano]

Hey Desi,

We are trying to cleanup some of our IME bugs in Google Docs. This is one that we looked at that would be great to see if can be resolved. jfyi

Thanks!

Comment 3

[Desigan Chinniah [:cyberdees] [:dees] [London - GMT]]

(In reply to Steven Saviano from comment #2)
> Hey Desi,
> 
> We are trying to cleanup some of our IME bugs in Google Docs. This is one
> that we looked at that would be great to see if can be resolved. jfyi
> 
> Thanks!

Thanks for highlighting, Steven. I've tagged and flagged this for followup. Could you also ping the Mailing List pointing out this Bug.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Log at commiting "ち" with click and type "と"

I reproduce this bug only with Apple Japanese Input, so, cannot reproduce this with ATOK 2016 nor Google Japanese Input. Checking this log briefly, we failed to commit the first composition only with Apple Japanese Input, but widget succeeded to dispatch eCompositionCommitAsIs event as expected. Hmm...

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

> IMEStateManager OnMouseButtonEventInEditor(aPresContext=0x129c86800, aContent=0x12f6f3a00, aMouseEvent=0x1270dfeb8), sPresContext=0x129c86800, sContent=0x12f6f3a00
> IMEStateManager   OnMouseButtonEventInEditor(), mouse event (type=mousedown, button=0) is not consumed
> IMEStateManager NotifyIME(aMessage=REQUEST_TO_COMMIT_COMPOSITION, aPresContext=0x129c86800, aOriginIsRemote=false)
> IMEStateManager NotifyIME(aNotification={ mMessage=REQUEST_TO_COMMIT_COMPOSITION }, aWidget=0x129cabc00, aOriginIsRemote=false), sFocusedIMEWidget=0x129cabc00, sRemoteHasFocus=false
> IMEStateManager   NotifyIME(), composition=0x128036040, composition->IsSynthesizedForTests()=false
> TextInputHandlerWidgets 129c65600 IMEInputHandler::CommitIMEComposition, mIMECompositionString=ち(U+3061)
> TextInputHandlerWidgets 129c65600 IMEInputHandler::KillIMEComposition, mView=129736df0, mWidget=129cabc00, inputContext=12f0c12c0, mIsIMEComposing=TRUE, Destroyed()=FALSE, IsFocused()=TRUE
> TextInputHandlerWidgets 129c65600 IMEInputHandler::SendCommittedText, mView=129736df0, mWidget=129cabc00, inputContext=12f0c12c0, mIsIMEComposing=TRUE
> IMEStateManager DispatchCompositionEvent(aNode=0x12f6f3a00, aPresContext=0x129c86800, aCompositionEvent={ mMessage=eCompositionCommitAsIs, mNativeIMEContext={ mRawNativeIMEContext=0x2F0C12C0, mOriginProcessID=0x0 }, mWidget(0x129cabc00)={ GetNativeIMEContext()={ mRawNativeIMEContext=0x2F0C12C0, mOriginProcessID=0x0 }, Destroyed()=false }, mFlags={ mIsTrusted=true, mPropagationStopped=false } }, aIsSynthesized=true), tabParent=0
> IMEStateManager NotifyIME(aNotification={ mMessage=NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED }, aWidget=0x129cabc00, aOriginIsRemote=false), sFocusedIMEWidget=0x129cabc00, sRemoteHasFocus=false
> TextInputHandlerWidgets 129c65600 IMEInputHandler::SelectedRange, Destroyed()=FALSE, mSelectedRange={ location=1, length=0 }
> TextInputHandlerWidgets 129c65600 IMEInputHandler::SelectedRange, Destroyed()=FALSE, mSelectedRange={ location=1, length=0 }
> TextInputHandlerWidgets 129c65600 IMEInputHandler::SelectedRange, Destroyed()=FALSE, mSelectedRange={ location=1, length=0 }
> IMEStateManager OnMouseButtonEventInEditor(aPresContext=0x129c86800, aContent=0x12f6f3a00, aMouseEvent=0x1283d7298), sPresContext=0x129c86800, sContent=0x12f6f3a00
> IMEStateManager   OnMouseButtonEventInEditor(), mouse event (type=mouseup, button=0) is not consumed
> IMEStateManager OnClickInEditor(aPresContext=0x129c86800, aContent=0x12f6f3a00, aMouseEvent=0x1283d7338), sPresContext=0x129c86800, sContent=0x12f6f3a00
> IMEStateManager GetNewIMEState(aPresContext=0x129c86800, aContent=0x12f6f3a00), sInstalledMenuKeyboardListener=false
> IMEStateManager   GetNewIMEState() returns { mEnabled=ENABLED, mOpen=DONT_CHANGE_OPEN_STATE }
> IMEStateManager SetIMEState(aState={ mEnabled=ENABLED, mOpen=DONT_CHANGE_OPEN_STATE }, aContent=0x12f6f3a00 (TabParent=0x0), aWidget=0x129cabc00, aAction={ mCause=CAUSE_MOUSE, mFocusChange=FOCUS_NOT_CHANGED })
> IMEStateManager SetInputContext(aWidget=0x129cabc00, aInputContext={ mIMEState={ mEnabled=ENABLED, mOpen=DONT_CHANGE_OPEN_STATE }, mHTMLInputType="search", mHTMLInputInputmode="", mActionHint="" }, aAction={ mCause=CAUSE_MOUSE, mAction=FOCUS_NOT_CHANGED }), sActiveTabParent=0x0
> TextInputHandlerWidgets 137020fc0 IMEInputHandler::GetValidAttributesForMarkedText
> TextInputHandlerWidgets 137020fc0 IMEInputHandler::GetValidAttributesForMarkedText
> TextInputHandlerWidgets 137020fc0 IMEInputHandler::GetValidAttributesForMarkedText
> TextInputHandlerWidgets 
> TextInputHandlerWidgets 129c65600 TextInputHandler::HandleKeyDownEvent, aNativeEvent=1297aaa60, type=NSKeyDown, keyCode=1 (0x1), modifierFlags=0x100, characters="と(U+3068)", charactersIgnoringModifiers="と(U+3068)"

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Ah, no. Looks like IMEInputHandler::SendCommittedText() doesn't kick TextInputHandler::InsertText(). Therefore, widget keeps composition but in content, IMEStateManager commits composition because it assumes that the commit request will be handled synchronously.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

This is a regression of bug 1180564. We've already dropped NSTextInput protocol, instead, we implemented NSTextInputClient protocol.  However, IMEInputHandler::SendCommittedText() still uses insertText: of NSTextInput.  Instead, it should call insertText:replacementRange: of NSTextInputClient.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1334594 Call NSTextInputClient's insertText:replacementRange: instead of NSTextInput's insertText: from IMEInputHandler::SendCommittedText()

Bug 1180564 dropped NSTextInput protocl implementation from ChildView but IMEInputHandler::SendCommittedText() still calls it via mView.  Therefore, it fails to commit composition in widget level.  Instead, we it should call insertText:replacementRange: of NSTextInputClient protocol.

Additionally, this patch adds a last resort path. If calling insertText:replacementRange: didn't commit composition in widget level, it calls TextInputHandler::InsertText() directly.

Review commit: https://reviewboard.mozilla.org/r/110364/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110364/

Comment 11

[Makoto Kato [:m_kato]]

Comment on attachment 8834399 [details]
Bug 1334594 Call NSTextInputClient's insertText:replacementRange: instead of NSTextInput's insertText: from IMEInputHandler::SendCommittedText()

https://reviewboard.mozilla.org/r/110364/#review111864

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64b970234605
Call NSTextInputClient's insertText:replacementRange: instead of NSTextInput's insertText: from IMEInputHandler::SendCommittedText() r=m_kato

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/64b970234605

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8834399 [details]
Bug 1334594 Call NSTextInputClient's insertText:replacementRange: instead of NSTextInput's insertText: from IMEInputHandler::SendCommittedText()

Approval Request Comment
[Feature/Bug causing the regression]: Regression of bug 1180564.
[User impact if declined]: Some IMEs commit active composition by themselves when user clicked in active view but the others, e.g., Apple Japanese Input, doesn't do so. Instead, we need to commit composition by ourselves. However, it's failed only in widget level but succeeded in content level.  Therefore, the mismatching state causes not being able to input with IME anymore after committing composition with a click.
[Is this code covered by automated tests?]: No. It's impossible for now.
[Has the fix been verified in Nightly?]: Yes. I confirmed this fix on OS X 10.9, 10.10, 10.11 and macOS 10.12 with ATOK 2011 or 2016, Google Japanese Input and Apple Japanese Input (or Kotoeri on 10.9).
[Needs manual test from QE? If yes, steps to reproduce]: No, but optionally, you can test with other IMEs:
1. Type something in an editor, e.g., in the search bar, and you see underlined text (If it's not underlined, you cannot test with this STR)
2. Click in the editor, then, the underlined text should be committed (underlines should have gone).
3. Type something again, then, you see new composing string (underlined text).
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Changing point runs only when committing composition by ourselves.  The old code did nothing due to the method has gone by bug 1180564. Instead, this calls new method instead.
[String changes made/needed]: No.

Comment 15

[Gerry Chang [:gchang]]

Hi [e-mail],
could you help verify if this issue is fixed as expected on the latest Nightly build? Thanks!

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8834399 [details]
Bug 1334594 Call NSTextInputClient's insertText:replacementRange: instead of NSTextInput's insertText: from IMEInputHandler::SendCommittedText()

OSX IME fix, aurora53+, beta52+

Comment 17

[charleyroy]

Hi Gerry,
Just checked on the latest nightly and it seems fixed! Thank you so much for the quick response - really appreciate it!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8ee7930272cb

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/878de02bf5c6

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/878de02bf5c6

Comment 21

[Desigan Chinniah [:cyberdees] [:dees] [London - GMT]]

Has this now landed? And can it be marked as RESOLVED?

Comment 22

[Ryan VanderMeulen [:RyanVM]]

See comment 13 and on. VERIFIED supersedes RESOLVED.