IME inserts duplicated commit string after composing character in password editor is masked with ATOK

VERIFIED FIXED in Firefox 67

Status

()

defect
P3
normal
VERIFIED FIXED
10 months ago
3 months ago

People

(Reporter: masayuki, Assigned: m_kato)

Tracking

({inputmethod})

unspecified
Firefox 67
ARM64
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment)

STR:
1. Go to https://jsfiddle.net/d_toybox/nf34bwou/1/
2. Type a character with ATOK or ATOK Pro.
3. Wait until the character is masked.
4. Type next character.

Actual Result:
The first character is duplicated before the second character. E.g., if you type "a" and "b", the result becomes "aab".

Expected Result:
IME shouldn't insert the composing character newly.  Instead, replace the masked character.

I'm not sure whether this is a regression or not because I usually don't type anything in password field from Firefox for Android (because of password manager synced with desktop).

I cannot reproduce this bug with Google Japanese Input nor Wnn.

I tried to reproduce this bug on Windows and macOS with commenting out LookAndFeel::GetEchoPassword() in TextEditRules. However, on them, original range is replaced as expected by coming commit character.

One of odd behavior on our side is, editor does not use IME selection for masked character. But I'm not sure if this is the cause since I didn't reproduce this on desktop.

I guess that some IME may specify range to replace composition string but others may not specify explicitly?
Sounds exactly like another bug we have on file.
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1490391
Nakano-san, could you verify whether this issue is fixed by the latest Firefox Nightly?
Flags: needinfo?(masayuki)
(TextEditRules::HideLastPasswordInputInternal doesn't commit composition. So it might be editor bug)
Unfortunately, not has been fixed yet.
Status: RESOLVED → REOPENED
Flags: needinfo?(masayuki)
Resolution: DUPLICATE → ---
Well, I've been concerning about that HideLastPasswordInputInternal() removes all IME selection. I'll try to restore it and check whether it fixes this bug or not.
Oh, with the IME selection restore patch, Google Japanese Input also broken. With the build, next character is inserted before the IME selection every time. I guess that IMEContentObserver notifies IME of text change in password field with the masked characters and that causes committing composition on IMEs only in them if we don't see this bug with current Nightly. So, it seems that we need to change IMEContentObserver with the value rather than the text node.
Hmm, even if I prevent notifying IME of text change caused by making password with this patch:
https://hg.mozilla.org/try/rev/0f086ccef5d9aad920bf0ebd49f31d58c1fc8bdc
, new character is inserted before IME selection. So, perhaps, widget for Android does not work as expected by me.
I updated my personal testcase for input events to listen to keyboard events and composition events too.
https://d-toybox.com/studio/lib/input_event_viewer.html

Then, I see really odd DOM events with ATOK. When I type "a",

keydown ("Process"), compositionstart, compoisitonupdate ("a"), keyup ("Process") are fired synchronously.

Then, oddly, keydown ("Process") and keyup ("Process") are fired asynchronously. I.e., neither with composition events nor keypress event.

Then, hiding the composing "a". At this time, we don't receive any input from widget.

Finally, next type "d" causes:

compositionend ("a"), keydown ("a"), keypress ("a"), keyup ("a"), keydown ("Process"), compositionstart, compositionupdate ("d").

So, this must be a bug of widget for Android.
Thanks, Nakano-san.

But this seems to depend on IME's behavior.  I think that ATOK commits string by imm.updateSelection(view, selStart, selEnd, -1, -1) (https://searchfox.org/mozilla-central/rev/fd62b95c187a40b328d9e7fd9d848833a6942b57/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/SessionTextInput.java#208).  But other IMEs such as Google Japanese Input cancel composition.  We need a workaround for ATOK support.
Assignee: nobody → m_kato
Log:

12-11 14:51:29.232 28300 28512 D GeckoInputConnection: < onSelectionChange
12-11 14:51:29.232 28300 28300 D GeckoInputConnection: > getView()
12-11 14:51:29.232 28300 28300 D GeckoInputConnection: < getView: org.mozilla.geckoview.GeckoView{bb5389 VFE...... .F...... 0,0-720,582 #7f070037 app:id/gecko_view}
12-11 14:51:30.730 28324 28343 I GeckoEditableSupport: IME: NotifyIMEOfTextChange: s=0, oe=1, ne=1
12-11 14:51:30.742 28324 28343 D GeckoEditableChild: onTextChange(•, 0, 1, 1)
12-11 14:51:30.746 28300 28316 D GeckoEditable: onTextChange("\u2022", 0, 1)
12-11 14:51:30.748 28324 28343 D GeckoEditableChild: onSelectionChange(1, 1)
12-11 14:51:30.750 28300 28316 D GeckoEditable: onSelectionChange(1, 1)
12-11 14:51:30.751 28300 28512 D GeckoInputConnection: > onTextChange()
12-11 14:51:30.751 28300 28512 D GeckoInputConnection: < onTextChange
12-11 14:51:30.752 28300 28512 D GeckoInputConnection: > onSelectionChange()
12-11 14:51:30.752 28300 28512 D GeckoEditable: getSpanStart(android.text.Selection$START@17ba032) = 1
12-11 14:51:30.752 28300 28512 D GeckoEditable: getSpanStart(android.text.Selection$END@51ea183) = 1
12-11 14:51:30.753 28300 28512 D GeckoEditable: getSpanStart(android.view.inputmethod.ComposingText@6b3d3bc) = -1
12-11 14:51:30.753 28300 28512 D GeckoEditable: getSpanEnd(android.view.inputmethod.ComposingText@6b3d3bc) = -1
12-11 14:51:30.753 28300 28512 D GeckoInputConnection: < onSelectionChange
12-11 14:51:30.754 28300 28300 D GeckoInputConnection: > getView()
12-11 14:51:30.755 28300 28300 D GeckoInputConnection: < getView: org.mozilla.geckoview.GeckoView{bb5389 VFE...... .F...... 0,0-720,582 #7f070037 app:id/gecko_view}
12-11 14:51:37.890 28300 28512 D GeckoInputConnection: > beginBatchEdit()
12-11 14:51:37.890 28300 28512 D GeckoInputConnection: < beginBatchEdit: true
12-11 14:51:37.891 28300 28512 D GeckoInputConnection: > commitText("a", 1)
12-11 14:51:37.891 28300 28512 D GeckoEditable: getSpanStart(android.view.inputmethod.ComposingText@6b3d3bc) = -1
12-11 14:51:37.891 28300 28512 D GeckoEditable: getSpanEnd(android.view.inputmethod.ComposingText@6b3d3bc) = -1
12-11 14:51:37.891 28300 28512 D GeckoEditable: getSpanStart(android.text.Selection$START@17ba032) = 1
12-11 14:51:37.891 28300 28512 D GeckoEditable: getSpanStart(android.text.Selection$END@51ea183) = 1
12-11 14:51:37.892 28300 28512 D GeckoEditable: length() = 2
12-11 14:51:37.892 28300 28512 D GeckoEditable: getSpanStart(android.text.Selection$START@17ba032) = 1
12-11 14:51:37.892 28300 28512 D GeckoEditable: getSpanStart(android.text.Selection$END@51ea183) = 1
12-11 14:51:37.892 28300 28512 D GeckoEditable: offer: Action(TYPE_REPLACE_TEXT)
12-11 14:51:37.893 28300 28512 D GeckoEditable: icSendComposition(): no composition
12-11 14:51:37.893 28300 28512 D GeckoEditable: sending: KeyEvent { action=ACTION_DOWN, keyCode=KEYCODE_A, scanCode=0, metaState=0, flags=0x0, repeatCount=0, eventTime=439837756, downTime=439837756, deviceId=-1, source=0x101 }
12-11 14:51:37.895 28300 28512 D GeckoEditable: sending: KeyEvent { action=ACTION_UP, keyCode=KEYCODE_A, scanCode=0, metaState=0, flags=0x0, repeatCount=0, eventTime=439837756, downTime=439837756, deviceId=-1, source=0x101 }
...
Well, looks like that the last 2 lines caused the redundant "a" key press event.

Looks like that it's called by here:
https://searchfox.org/mozilla-central/rev/fd62b95c187a40b328d9e7fd9d848833a6942b57/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java#545,583,590-593

But according to the log, we don't commit composition in icMaybeSendComposition().

Next, in GeckoEditableSupport::OnKeyEvent(), calls RemoveComposition(COMMIT_IME_COMPOSITION):
https://searchfox.org/mozilla-central/rev/fd62b95c187a40b328d9e7fd9d848833a6942b57/widget/android/GeckoEditableSupport.cpp#668,717-718

This would cause dispatching eCompositionCommitAsIs:
https://searchfox.org/mozilla-central/rev/fd62b95c187a40b328d9e7fd9d848833a6942b57/widget/TextEventDispatcher.cpp#346,380

So, perhaps, we shouldn't commit existing composition in this case, instead, we should cancel it before dispatching the set of key events.

From another point of view, dispatching keyboard events here sounds odd to me because we've already had non-empty composition. So, instead of dispatching keyboard events, we should commit composition with given string, perhaps.
Priority: -- → P3
Summary: IME inserts duplicated commit string after composing character in password editor is masked → IME inserts duplicated commit string after composing character in password editor is masked with ATOK

Nakano-san, do you have ATOK professional? Since I don't have one, I cannot know input method name of ATOK Pro such as com.justsystems.atokmobile.pv.service/.AtokInputMethodService.

  • ATOK ... com.justsystems.atokmobile.service/.AtokInputMethodService
  • ATOK Passport ... com.justsystems.atokmobile.pv.service/.AtokInputMethodService

I guess that ATOK Pro is com.justsystems.atokmobile2.pv.service/.AtokInputMethodService.

Flags: needinfo?(masayuki)

When removing composing text, we call InputMethodManager.updateSelection(start,
end, -1, -1). But ATOK (Japanese input method by Justsystem) series do nothing.
So shadow text and current text becomes mismatched.

For workaround, we need call restartInput to remove composing text if using
ATOK series.

(In reply to Makoto Kato [:m_kato] from comment #17)

Nakano-san, do you have ATOK professional? Since I don't have one, I cannot know input method name of ATOK Pro such as com.justsystems.atokmobile.pv.service/.AtokInputMethodService.

  • ATOK ... com.justsystems.atokmobile.service/.AtokInputMethodService
  • ATOK Passport ... com.justsystems.atokmobile.pv.service/.AtokInputMethodService

I guess that ATOK Pro is com.justsystems.atokmobile2.pv.service/.AtokInputMethodService.

Yes, it is.

On the other hand, I have ATOK for ASUS, its service name is com.atok.mobile.im.asus.service. Perhaps, we should ask him.

Flags: needinfo?(masayuki)

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #20)

Yes, it is.

Thanks.

On the other hand, I have ATOK for ASUS, its service name is com.atok.mobile.im.asus.service. Perhaps, we should ask him.

Ahhh, why don't they use com.justsystems.atokmobile XXXXXX... I will add this name to workaround code.

Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/390f162b7d9b
Restart input method to remove composition on some IMEs. r=geckoview-reviewers,esawin
Status: REOPENED → RESOLVED
Closed: 10 months ago5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

Verified with ATOK for ASUS and ATOK Passport, but still reproduced with ATOK Passport Professional. I'll file a bug for the latter.

Status: RESOLVED → VERIFIED
No longer depends on: 1540628
Regressions: 1540628
You need to log in before you can comment on or make changes to this bug.