Closed Bug 1177011 Opened 6 years ago Closed 6 years ago

In IMDB search field, text is duplicated each time you type a character

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(firefox40 unaffected, firefox41+ fixed, firefox42 verified, fennec41+)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox40 --- unaffected
firefox41 + fixed
firefox42 --- verified
fennec 41+ ---

People

(Reporter: dholbert, Assigned: jchen)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

STR:
 1. Visit imdb.com in Firefox for Android
 2. Tap their search field, near the upper right of the page.
 3. Type some text (e.g. "abc")

ACTUAL RESULTS:
Instead of "abc", the search field ends up saying "abcababa"

With each character you type, the previous contents of the search box get duplicated to the right of the cursor. Or something like that.
BROKEN: current Nightly, 41.0a1 (2015-06-23)
WORKING: current Aurora, 40.0a2 (2015-06-23)

So, this is a new regression in 41, it seems.
tracking-fennec: --- → ?
jchen: kgrandon suggested that I ping you about this bug & that you might be working on something similar (maybe the same bug) already.
Flags: needinfo?(nchen)
tracking-fennec: ? → ---
mid-air ... sorry about that  :/
tracking-fennec: --- → ?
I can repro this on my phone device, but not for my tablet. The last good chgset is:
249292:9345403c7628

This seems to be regressed by the four changesets for bug 1172466, a core/IME related change:

https://hg.mozilla.org/mozilla-central/rev/c215b9a30b73
https://hg.mozilla.org/mozilla-central/rev/83a659053a52
https://hg.mozilla.org/mozilla-central/rev/6e481878f2b0
https://hg.mozilla.org/mozilla-central/rev/a633cd8ac3d9
Flags: needinfo?(masayuki)
I can add, I observe this on the phone device in different ways based on keyboard...

For the stock Google keyboard:

   If I type the single char |i|, I wind up with two chars |ii|, and the cursor blinks between them.

For SwiftKeyboard:

   If I type two chars |im|, I wind up with |mi|, and the cursor blinks between them.
If the bug is the cause of this regression, that's odd except if the patches have some bugs. The patches make Gecko send selection change notification to IME asynchronously if it's in reflow. If Android's IME needs synchronous selection change notification or something is necessary, we cannot provide it now...

If it's necessary, nsWindow for Android should compute expected selection range and use it until selection change notification comes.
Flags: needinfo?(masayuki)
FYI: If the expected notification won't be sent asynchronously, it's a bug of the patches, otherwise, we need to fix this bug in Android widget side.

Anyway, I guess that backing out https://hg.mozilla.org/mozilla-central/rev/a633cd8ac3d9 fixes the bug, isn't it?
Flags: needinfo?(markcapella)
Attached patch Proposal fix (obsolete) — Splinter Review
If the patches are buggy, i.e., we failed to notify IME in some cases, this patch may fix this bug. Otherwise, we must need to hack in nsWindow for Android...
Attachment #8625716 - Flags: feedback?(markcapella)
Backing out the patch mentioned in comment #7 does fix the problem.
Flags: needinfo?(markcapella)
Comment on attachment 8625716 [details] [diff] [review]
Proposal fix

Review of attachment 8625716 [details] [diff] [review]:
-----------------------------------------------------------------

The problem still exists after applying this patch.
Attachment #8625716 - Flags: feedback?(markcapella)
[Tracking Requested - why for this release]:
(In reply to Mark Capella [:capella] from comment #10)
> Comment on attachment 8625716 [details] [diff] [review]
> Proposal fix
> 
> Review of attachment 8625716 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The problem still exists after applying this patch.

Thank you for your test. I guess that nsWindow for Android should notify IME of selection change itself immediately after dispatching NS_COMPOSITION_(CHANGE|COMMIT|COMMIT_AS_IS).
But if it causes to query selection range, it may fail due to under reflow. So, in such case, nsWindow should notify IME of emulated selection range.
One shortcoming of the Android IME code is it expects synchronous selection change notifications. In some situations, we don't pass the selection change notification to the Java side, as controlled by nsWindow::mIMEMaskSelectionUpdate. We do this in, for example, [1] and [2]. However, if the selection change notification is asynchronous, mIMEMaskSelectionUpdate stops working, and we end up passing unexpected selection change notifications to the Java side. I suspect these extra notifications are confusing the keyboard.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp?rev=7b0df70e27ea#1675
[2] http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp?rev=7b0df70e27ea#1781
Flags: needinfo?(nchen)
Adding a tracking flag for FF41. This bug has a negative end-user impact.
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Oh, it's actually a bug in IMEContentObserver/EventStateManager. When IMEContentObserver::MaybeReinitialize calls IMEContentObserver::Init, EventStateManager::OnStartToObserveContent is called again, which releases the existing (same) IMEContentObserver [1]. That causes mESM to be set to nullptr [2], and notifications are not sent because [3] fails.

One solution is to check inside EventStateManager::OnStartToObserveContent, and don't release the existing IMEContentObserver if it's the same as the new IMEContentObserver.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager.cpp?rev=291614a686f1#446
[2] http://mxr.mozilla.org/mozilla-central/source/dom/events/IMEContentObserver.cpp?rev=79800010be78#328
[3] http://mxr.mozilla.org/mozilla-central/source/dom/events/IMEContentObserver.cpp?rev=79800010be78#1063
Attachment #8627757 - Flags: review?(masayuki)
Comment on attachment 8627757 [details] [diff] [review]
Ignore restarting the same content observer in EventStateManager (v1)

Good catch!
Attachment #8627757 - Flags: review?(masayuki) → review+
tracking-fennec: ? → 41+
https://hg.mozilla.org/mozilla-central/rev/95af75ab24cd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Duplicate of this bug: 1175838
jchen - 41 is marked as affected. What do you think about uplifting the fix? (Note that dup bug 1175838 also marks 40 as affected.)
Flags: needinfo?(nchen)
Comment on attachment 8627757 [details] [diff] [review]
Ignore restarting the same content observer in EventStateManager (v1)

Hm not sure about 40. The regressing bug, bug 1172466, landed in 41, so this particular fix applies to 41+. 

Approval Request Comment
[Feature/regressing bug #]: Bug 1172466
[User impact if declined]: Input broken on certain sites such as Google and IMDB
[Describe test coverage new/current, TreeHerder]: Locally, m-c
[Risks and why]: Very small; the patch is very small and fixes an obvious bug
[String/UUID change made/needed]: None
Flags: needinfo?(nchen)
Attachment #8627757 - Flags: approval-mozilla-aurora?
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #21)
> jchen - 41 is marked as affected. What do you think about uplifting the fix?
> (Note that dup bug 1175838 also marks 40 as affected.)
(In reply to Jim Chen [:jchen] [:darchons] from comment #22)
> Hm not sure about 40. The regressing bug, bug 1172466, landed in 41, so this
> particular fix applies to 41+.

According to my testing in comment 1, this bug here is a new problem in Firefox 41. Firefox 40 is unaffected.

If dup bug 1175838 affects Firefox 40, it might not actually be a dupe. (Though: it looks like that bug's claim for Firefox 40 was just a guess, per bug 1175838 comment 14)
[needinfo'd reporter on bug 1175838 RE status of Firefox 40.]
Comment on attachment 8627757 [details] [diff] [review]
Ignore restarting the same content observer in EventStateManager (v1)

Thanks for confirming that 40 is not affected. Safe fix. Let's get this on Aurora. Aurora+
Attachment #8627757 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reproduced the issue on 06-23 build
Verified as fixed using:
Device: Nexus 4 (Android 5.1)
Build: Firefox for Android 42.0a1 (2015-07-19)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.