Closed Bug 1672095 Opened 1 month ago Closed 1 month ago

Inputting character isn't inserted at caret position.

Categories

(GeckoView :: General, defect, P1)

Unspecified
Android

Tracking

(firefox82 unaffected, firefox83 verified, firefox84 verified)

RESOLVED FIXED
84 Branch
Tracking Status
firefox82 --- unaffected
firefox83 --- verified
firefox84 --- verified

People

(Reporter: m_kato, Assigned: m_kato)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

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)

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

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

Keywords: regression
Regressed by: 1612802

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.

Flags: needinfo?(masayuki)

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.

Flags: needinfo?(masayuki)

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

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.

Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/e29df844f897
Always send NOTIFY_IME_OF_BLUR on GeckoView. r=masayuki
Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch

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
  1. Browser https://www.google.com/ by Fenix (or GeckoView example)
  2. Input "abc def" to search box
  3. Tap home button of Android.
  4. Tap Fenix (or GeckoView example)
  5. Tap search box on http://www.google.com
  6. set caret to between "a" and "b" in search box
  7. 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
Attachment #9183101 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

Attachment #9183101 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Flags: qe-verify+
Duplicate of this bug: 1673452

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

Duplicate of this bug: 1672566
You need to log in before you can comment on or make changes to this bug.