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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

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.

Comment 2

[Daniel Holbert [:dholbert]]

jchen: kgrandon suggested that I ping you about this bug & that you might be working on something similar (maybe the same bug) already.

Comment 3

[Mark Capella [:capella]]

mid-air ... sorry about that  :/

Comment 4

[Mark Capella [:capella]]

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

Comment 5

[Mark Capella [:capella]]

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.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Still not recoverd perfectly)]

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.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Still not recoverd perfectly)]

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?

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Still not recoverd perfectly)]

Attachment: Proposal fix

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

Comment 9

[Mark Capella [:capella]]

Backing out the patch mentioned in comment #7 does fix the problem.

Comment 10

[Mark Capella [:capella]]

Comment on attachment 8625716 [details] [diff] [review]
Proposal fix

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

The problem still exists after applying this patch.

Comment 11

[Kevin Brosnan [:kbrosnan]]

[Tracking Requested - why for this release]:

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Still not recoverd perfectly)]

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

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Still not recoverd perfectly)]

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.

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

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

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Adding a tracking flag for FF41. This bug has a negative end-user impact.

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Ignore restarting the same content observer in EventStateManager (v1)

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

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Still not recoverd perfectly)]

Comment on attachment 8627757 [details] [diff] [review]
Ignore restarting the same content observer in EventStateManager (v1)

Good catch!

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95af75ab24cd

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/95af75ab24cd

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

jchen - 41 is marked as affected. What do you think about uplifting the fix? (Note that dup bug 1175838 also marks 40 as affected.)

Comment 22

[(inactive) Jim Chen [:jchen] [:darchons]]

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

Comment 23

[Daniel Holbert [:dholbert]]

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

Comment 24

[Daniel Holbert [:dholbert]]

[needinfo'd reporter on bug 1175838 RE status of Firefox 40.]

Comment 25

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/364a4843ad22

Comment 27

[Teodora Vermesan (:TeoVermesan)]

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)