1672095 - Inputting character isn't inserted at caret position.

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Makoto Kato [:m_kato]]

I don't investigate regression range yet. I found this yesterday.

Env

• Fenix Nightly 201018 17:01
• Android 10 (Xperia XZ2 Compact)
• GBoard

Step

1. Browse https://www.google.com
2. Type any string (ex. google)
3. set caret between characters.
4. Type any character. (ex. a) and commit this.
5. Set caret to tail
6. Type any character

Result

6.'s character is inserted after 3.'s character. This doesn't occurs on Fenix beta (82-beta6)

Comment 1

[Makoto Kato [:m_kato]]

I guess that mIMEMaskEventsCount state is invalid, but I don't know regression range.

Comment 2

[Makoto Kato [:m_kato]]

When destroying IME Content observer, we might not send NOTIFY_IME_OF_BLUR by bug 1612802...

Comment 3

[Makoto Kato [:m_kato]]

Nakano-san, I missed blur issue of application focus when reviewing bug 1612802. Before landing it, NOTIFY_IME_OF_BLUR call count was same as NOTIFY_IME_OF_FOCUS call count. Since we don't send NOTIFY_IME_OF_BLUR during IME Content Observer is being destroyed, NOTIFY_IME_OF_BLUR isn't called when application focus is lost.

Shouldn't we expect NOTIFY_IME_OF_BLUR is always called? I think that NOTIFY_IME_OF_BLUR is always called when focus is lost.

Since GeckoEditableSupport runs on child process, I need more hacks for this situation.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Ah, I forgot the Android widget specific issue. I'll post a patch if you wish.

Probably:

1. Rename CanSendNotificationToTheMainProcess to CanSendNotificationToWidget
2. If it's in the main process, return true.
3. If it's in a content process, but the widget is a PuppetWidget and has mNativeTextEventDispatcherListener, return true.
4. Otherwise, return !sCleaningUpForStoppingIMEStateManagement.

Then, it works in any type of processes.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Well, #2 and #3 could be implemented with a new API if there is no API to check whether the widget is a PuppetWidget or not, and/or whether the PuppetWidget has or does not have mNativeTextEventDispatcherListener. E.g., add nsIWidget::HasNativeTextInputHandler(), then, make nsBaseWidget::HasNativeTextInputHandler() return true, and make PuppetWidget::HasNativeTextInputHandler() return true only when it has mNativeTextEventDispatcherListener (i.e., only on Android).

Comment 6

[Makoto Kato [:m_kato]]

Attachment: Bug 1672095 - Always send NOTIFY_IME_OF_BLUR on GeckoView. r=masayuki

This is regression by bug 1672095. GeckoEditableSupport expects that
NOTIFY_IME_OF_BLUR is always sent. I would like to allow this even if
IME content observer is being destroyed.

To reproduce this, we have to lost application focus, I cannot write
geckoview-junit test.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e29df844f897
Always send NOTIFY_IME_OF_BLUR on GeckoView. r=masayuki

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/e29df844f897

Comment 9

[Makoto Kato [:m_kato]]

Comment on attachment 9183101 [details]
Bug 1672095 - Always send NOTIFY_IME_OF_BLUR on GeckoView. r=masayuki

Beta/Release Uplift Approval Request

• User impact if declined: After lost application focus on Fenix, inputted text is duplicated and user cannot insert text at caret position ever.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Set default keyboard to GBoard

2. Browser https://www.google.com/ by Fenix (or GeckoView example)
3. Input "abc def" to search box
4. Tap home button of Android.
5. Tap Fenix (or GeckoView example)
6. Tap search box on http://www.google.com
7. set caret to between "a" and "b" in search box
8. Type "g"

• Result
  6's "g" isn't inserted at caret position.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is regression by bug 1612802. I revert this change on Android only.
• String changes made/needed: No

Comment 10

[Pascal Chevrel:pascalc]

Comment on attachment 9183101 [details]
Bug 1672095 - Always send NOTIFY_IME_OF_BLUR on GeckoView. r=masayuki

83 regression with IME support that affects Fenix, let's take it in our next beta, thanks.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/85025a0e2a54

Comment 12

[Laurentiu Apahidean]

Verified as fixed on Firefox Preview Nightly 201029 (Build #2015772459) using a Google Pixel 3 XL (Android 9) and OnePlus 6T (Android 9).

Comment 14

[Laurentiu Apahidean]

Verified as fixed on Firefox Preview Beta 10/27 (Build #2015772179) using a Google Pixel 3 XL (Android 9) and OnePlus 6T (Android 9).