Hideo Oshima Reporter
	Description • 9 years ago

I use fcitx and mozc for input method.
It can undo commit by Ctral + Backspace but cannot do with Firefox gtk3 build.

Steps to reproduce:
1. Click search box.
2. Hit もじら and Enter to commit.
3. Hit Ctrl + Backspace.

Actual result:
もじら is eliminated.

Expected result:
Undo commit for もじら.

Environment:
OS: Debian unstable (uptodate)
Toolkit: GTK3 3.10.1-1 / GLib 2.42.1-1
Input Method: fcitx 1:4.2.8.5-2

	Alice0775 White
	Comment 1 • 9 years ago

Pushlog: 
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=11fc11a90d6b&tochange=73b8079308bd

Regressed by: Bug 1065835

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 2 • 9 years ago

Comment on attachment 8574378 [details]
Input Method log

> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): OnKeyEvent, aCaller=7fd45fd89f80, aKeyDownEventWasSent=FALSE
> 2146944896[7fd47eb33a00]:     aEvent: type=GDK_KEY_PRESS, keyval=Control_L, unicode=0x0
> 2146944896[7fd47eb33a00]:     filterThisEvent=FALSE (isFiltered=NO, mFilterKeyEvent=YES)
> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): OnKeyEvent, aCaller=7fd45fd89f80, aKeyDownEventWasSent=FALSE
> 2146944896[7fd47eb33a00]:     aEvent: type=GDK_KEY_PRESS, keyval=BackSpace, unicode=0x8

Pressed Ctrl+Backspace here.

> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): OnRetrieveSurroundingNative, aContext=7fd46240d6b0, current context=7fd46240d6b0
> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): GetCurrentParagraph, mCompositionState=NotComposing
> 2146944896[7fd47eb33a00]:         selOffset=3, selLength=0
> 2146944896[7fd47eb33a00]:     aText=もじら, aText.Length()=3, aCursorPos=3
> 2146944896[7fd47eb33a00]:     filterThisEvent=TRUE (isFiltered=YES, mFilterKeyEvent=YES)
> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): OnDeleteSurroundingNative, aContext=7fd46240d6b0, current context=7fd46240d6b0
> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): DeleteText, aContext=7fd46240d6b0, aOffset=-3, aNChars=3, mCompositionState=NotComposing
> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): OnSelectionChange(aCaller=0x7fd45fd89f80), mCompositionState=NotComposing

Committed string is not removed.

> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): ResetIME, mCompositionState=NotComposing, mIsIMFocused=YES
> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): OnSelectionChange(aCaller=0x7fd45fd89f80), mCompositionState=NotComposing
> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): ResetIME, mCompositionState=NotComposing, mIsIMFocused=YES

gtk_im_context_reset() is called for notifying IME of caret position change which is caused by removing the committed string.

> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): OnStartCompositionNative, aContext=7fd46240d6b0, current context=7fd46240d6b0
> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): DispatchCompositionStart, aContext=7fd46240d6b0
> 2146944896[7fd47eb33a00]:     mCompositionStart=0

"compositionstart"

> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): SetCursorPosition, aContext=7fd46240d6b0, aTargetOffset=4294967295
> 2146944896[7fd47eb33a00]:     FAILED, aTargetOffset is wrong offset

Although, this error is our bug, but shouldn't affect any IME behavior in this case (this causes not notifying IME of caret rect).

> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): OnChangeCompositionNative, aContext=7fd46240d6b0
> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): GetCompositionString, result="もじら"
> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): DispatchCompositionChangeEvent, aContext=7fd46240d6b0
> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): CreateTextRangeArray, aContext=7fd46240d6b0, aLastDispatchedData="もじら" (length=3)
> 2146944896[7fd47eb33a00]:     mStartOffset=0, mEndOffset=3, mRangeType=NS_TEXTRANGE_CONVERTEDTEXT
> 2146944896[7fd47eb33a00]:     mStartOffset=3, mEndOffset=3, mRangeType=NS_TEXTRANGE_CARETPOSITION
> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): SetCursorPosition, aContext=7fd46240d6b0, aTargetOffset=0

Now, creating composition string as you expected.

> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): OnChangeCompositionNative, aContext=7fd46240d6b0
> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): GetCompositionString, result=""

However, IME sets empty composition string here.

> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): DispatchCompositionChangeEvent, aContext=7fd46240d6b0
> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): CreateTextRangeArray, aContext=7fd46240d6b0, aLastDispatchedData="" (length=0)
> 2146944896[7fd47eb33a00]:     preedit_string is null
> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): SetCursorPosition, aContext=7fd46240d6b0, aTargetOffset=0
> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): OnEndCompositionNative, aContext=7fd46240d6b0
> 2146944896[7fd47eb33a00]: GtkIMModule(7fd45fe4d780): DispatchCompositionCommitEvent, aContext=7fd46240d6b0, aCommitString=0, ("")

"compositionend"


It might be caused by calling gtk_im_context_reset()... However, if so, fcitx behaves incorrectly because applications should call it when caret position is changed:
https://developer.gnome.org/gtk3/unstable/GtkIMContext.html#gtk-im-context-reset

I guess that we shouldn't call it when the change is caused by the IME.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 3 • 9 years ago

The cause is resetting IM context during performing to delete surrounding text which is requested by IM. According to the document of gtk_im_context_reset(), https://developer.gnome.org/gtk3/unstable/GtkIMContext.html#gtk-im-context-reset , every selection change should be notified to IME with it. However, mozc will discard the composition which will be started after deleting surrounding text.

We should prevent to call it during receving "delete_surrounding" signal. Although, this patch isn't perfect. E.g., if selection is actually changed by JS at removing the text, we should notify IME of it. But it's impossible with current design. If somebody will report such bug, we should work on this.

For now, we should make Kakutei-Undo available.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 4 • 9 years ago

And I'd like to know how Yukawa-san thinks.

Yukawa-san, do you think applications shouldn't call gtk_im_context_reset()? See above comment for the detail.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 5 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) from comment #4)
> And I'd like to know how Yukawa-san thinks.
> 
> Yukawa-san, do you think applications shouldn't call gtk_im_context_reset()?

Oops, I meant "while IME tries to remove surrounding text".

	Yohei Yukawa
	Comment 6 • 9 years ago

(CC-ing Weng, who is the author of fcitx-mozc.  Actually he has been maintaining fcitx itself)

tl;dr I have the feeling that we could fix this issue in fcitx-mozc/ibus-mozc side, but so far trying to keep the previous behavior in Firefox side sounds safer, because it usually takes several months to years for some major Linux distros to import new version of Mozc.  I'll take a look when I have time anyway.

Just for the record, here is the code where ibus-mozc and fcitx-mozc revert the on-going composition when they receive gtk_im_context_reset().
https://code.google.com/p/mozc/source/browse/trunk/src/unix/ibus/mozc_engine.cc?r=495#438
https://gitorious.org/fcitx/mozc/source/c37b4cb71230553b14053b458b0974b9931263fd:src/unix/fcitx/fcitx_mozc.cc#L189

According to our internal commit log, this behavior was introduced in ibus-mozc to address an issue found in Chromium OS (http://crosbug.com/4596) about 5 years ago, when Chromium OS still had used IBus.  Note that Revert-Commit feature (Ctrl-Backspace) was not supported in ibus-mozc at that time.  After that, Chromium OS discontinued using IBus, while ibus-mozc started supporting Revert-Commit feature.

	Weng Xuetian
	Comment 7 • 9 years ago

I think there's some misunderstanding of the gtk documentation. The documentation intends to say that reset needs to be called if the cursor position is changed outside the scope of input method.

For example: when you're typing in a text field in the middle of the composition, you use mouse to click some where else in the same text field. Without checking the implementation of native gtk3 text field, as far as I try text entry example in gtk3-demo, it does seem to do so. (It doesn't send reset in gtk2 case which can be considered as a bug)

If the cursor change is initiated or caused by input method, it should not call reset. The meaning of reset to input method is to discard anything you are typing.

	Yohei Yukawa
	Comment 8 • 9 years ago

(In reply to Weng Xuetian from comment #7)
> If the cursor change is initiated or caused by input method, it should not
> call reset. The meaning of reset to input method is to discard anything you
> are typing.

Fair enough.  One thing I can add is that ideally this kind of expected behavior should be clearly explained in the spec document, like JavaDoc of Android IME APIs.
http://developer.android.com/reference/android/view/inputmethod/InputConnection.html#deleteSurroundingText(int, int)
> IME authors: ...
> Editor authors: ...

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 9 • 9 years ago

Thank you, Yukawa-san and Weng Xuetian.

I understand that (although, GTK document should define it clearer). I feel it does make sense that applications shouldn't call gtk_im_context_reset() when the cursor change is caused by input method.

However, I have a concern. I guess that "delete-surronding" signal may be sent by any middleware. In other words, applications cannot distinguish the sender if it's an input method or not. Therefore, there *might* be some edge cases.

Additionally, in this case, I feel odd since a call of gtk_im_context_reset() canceling a composition which hasn't started yet.

But anyway, Gecko will stop calling it if a cursor position change is caused by "delete-surrounding" signal.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 10 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e80c5e0aba2

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/7e80c5e0aba2

	Hideo Oshima Reporter
	Comment 12 • 9 years ago

I verified to fix with recent nightly build.
Thanks!

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 13 • 9 years ago

Comment on attachment 8575215 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 1065835
[User impact if declined]: Mozc users cannot undo committing composition with Ctrl + Backspace. This should be fixed on 38 ESR.
[Describe test coverage new/current, TreeHerder]: already landed on m-c (about 10 days ago).
[Risks and why]: The risk is low because we just stop notifying IME of caret position change if it's caused by IME. That was not notified before bug 1065835 was fixed.
[String/UUID change made/needed]: Nothing.

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/5ac36d22cce1