1199658 - Text is duplicated at every key stroke when typing new post on a Facebook page

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[Hallvord R. M. Steen [:hallvors]]

1) Be the owner of a Facebook "page"
2) Try to write a new post on the Facebook page in Firefox on Android
3) For every key "stroke" from the on-screen keyboard, input is multiplicated. Typing test appears like this:

T

TTe

TTeTTes

TTeTTesTTeTTest

Even backspace makes the text duplicate itself before it starts deleting.

Comment 1

[Hallvord R. M. Steen [:hallvors]]

Attachment: fb-typing.html

rough demo, loading lots of CSS and JS from Facebook still.

It likely has something to do with the way FB manipulates the selection and what the IME thinks the state of the selection is not being in sync..?

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Jim, do you know what's going on here?

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

Sounds a bit like http://people.mozilla.org/~kbrosnan/tmp/1144174/facebook.mp4

Comment 4

[Hallvord R. M. Steen [:hallvors]]

Attachment: 1199658.htm - demo, minimal

Deceptively simple - I was *so* close to finding it during early experiments but unfortunately I gave up and went down the longwinded minimise-from-full-site approach. Briefly: 

textarea.oninput = function(){this.value = this.value}

Comment 5

[Hallvord R. M. Steen [:hallvors]]

It's not unlikely to be similar to bug 549674 (just guessing though) but the fix for that one doesn't handle this case.

Comment 6

[Hallvord R. M. Steen [:hallvors]]

Also, it would be nice to give this rather high priority given that we're talking about a problem that will annoy users on the most popular site on earth ;)

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I don't understand what happens. When sets the value attribute, editor requests to commit composition to IME via TextComposition. If IME doesn't commit composition synchronously, TextComposition will eat the following commit event which is dispatched asynchronously. So, it sounds like that widget sends commit string without composition after commit??

It might help that the testcase records compositionstart, compositionupdate, text and compositionend event.

Comment 8

[Hallvord R. M. Steen [:hallvors]]

Attachment: 1199658.htm - demo with more logging

Comment 9

[Hallvord R. M. Steen [:hallvors]]

Like this?

The event log.. wow! It sure looks like a mess :-p

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Hallvord R. M. Steen [:hallvors] from comment #9)
> Like this?
> 
> The event log.. wow! It sure looks like a mess :-p

Yes. Ideally, the events should be:

> compositionstart on [object HTMLTextAreaElement]
> compositionupdate on [object HTMLTextAreaElement]
> text on [object HTMLTextAreaElement]
> input event on [object HTMLTextAreaElement]
> text on [object HTMLTextAreaElement]
> compositionend on [object HTMLTextAreaElement]
> input event on [object HTMLTextAreaElement]

Comment 11

[Hallvord R. M. Steen [:hallvors]]

I request tracking because it's an annoying issue on Facebook. It may only affect people who "own" pages - but similar JS may also be used elsewhere.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 43. Do you know if other versions are affected?

Comment 13

[Hallvord R. M. Steen [:hallvors]]

All versions are affected. If a fix earlier than in 43 is possible it would of course be good.

Comment 14

[Michael Ellis]

PE: Bug Confirmed on m.facebook when using Firefox Beta. Unable to reproduce elsewhere thus far.  We will reach out to our partners @ fb with this information. 

@ Harald, anything to add?

Comment 15

[Hallvord R. M. Steen [:hallvors]]

FWIW we should fix this on our side - while setting .value on every keystroke seems excessive I'm pretty sure Facebook has valid use cases for setting .value while the user is typing, and I don't think we can recommend any good workarounds.

Comment 16

[:Harald Kirschner :digitarald]

+1 for :hallvors recommendation to fix this on our side. As far as I can tell it seems like a regression on our side.

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Margaret, can you help find an owner for this one?

Comment 18

[:Margaret Leibovic]

Eugen, would you be able to take a look at this?

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I want to see the log of the testcase because I cannot reproduce this bug with my Android phone.

When the webpage sets .value during composition, TextComposition::RequestToCommit() should be called. If the native IME commits composition synchronously, this bug shouldn't be reproduced because the committed string is overwritten by the setting value. If the native IME commits composition asynchronously, eCompositionCommit(AsIs) event is dispatched by nsWindow after actually the value is modified. However, in this case, TextComposition should discard the delayed composition event. Since nsWindow for Android directly references TextComposition (of course, it shouldn't do that!), nsWindow does something wrong which is not expected by TextComposition.

Comment 20

[Mark Capella [:capella]]

I can also repro this on my GS3 and N7 using |1199658.htm - demo with more logging|
   https://bugzilla.mozilla.org/attachment.cgi?id=8655943

The onscreen debug info shows:

compositionstart on [object HTMLTextAreaElement]
compositionupdate on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
compositionend on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
compositionstart on [object HTMLTextAreaElement]
compositionupdate on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
compositionend on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
compositionstart on [object HTMLTextAreaElement]
compositionupdate on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
compositionend on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
compositionstart on [object HTMLTextAreaElement]
compositionupdate on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
compositionend on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
compositionstart on [object HTMLTextAreaElement]
compositionupdate on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
compositionend on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
compositionstart on [object HTMLTextAreaElement]
compositionupdate on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
compositionend on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
compositionstart on [object HTMLTextAreaElement]
compositionupdate on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
compositionend on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
compositionstart on [object HTMLTextAreaElement]
compositionupdate on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
compositionend on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
compositionstart on [object HTMLTextAreaElement]
compositionupdate on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
compositionend on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
compositionstart on [object HTMLTextAreaElement]
compositionupdate on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
compositionend on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
compositionstart on [object HTMLTextAreaElement]
compositionupdate on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]
text on [object HTMLTextAreaElement]
compositionend on [object HTMLTextAreaElement]
input event on [object HTMLTextAreaElement]

Comment 21

[Mark Capella [:capella]]

forgot to mention, I see basically no messages in logcat, and I get this behaviour on both Swift keyboard and Google keyboard.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you for the log. Looks like that it starts composition after .value is changed.

http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp?rev=cdee9be7cdc1#1929

> 1915     case AndroidGeckoEvent::IME_REPLACE_TEXT:
> 1916     case AndroidGeckoEvent::IME_COMPOSE_TEXT:
> 1917         {
> 1918             /*
> 1919                 Replace text in Gecko thread from ae->Start() to ae->End()
> 1920                   with the string ae->Characters()
> 1921 
> 1922                 Selection updates are masked so the result of our temporary
> 1923                   selection event is not passed on to Java
> 1924             */
> 1925             AutoIMEMask selMask(mIMEMaskSelectionUpdate);
> 1926             const auto composition(GetIMEComposition());
> 1927             MOZ_ASSERT(!composition || !composition->IsEditorHandlingEvent());
> 1928 
> 1929             if (!mIMEKeyEvents.IsEmpty() || !composition ||
> 1930                 uint32_t(ae->Start()) !=
> 1931                     composition->NativeOffsetOfStartComposition() ||
> 1932                 uint32_t(ae->End()) !=
> 1933                     composition->NativeOffsetOfStartComposition() +
> 1934                     composition->String().Length()) {

In such case, GetIMEComposition() returns non-nullptr. Then, even if mIMEKeyEvents is empty, this condition could be true.

Or, another possible buggy point is here:

http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#2087

2087             if (!composition ||
2088                 uint32_t(ae->Start()) !=
2089                     composition->NativeOffsetOfStartComposition() ||
2090                 uint32_t(ae->End()) !=
2091                     composition->NativeOffsetOfStartComposition() +
2092                     composition->String().Length()) {

If those conditions become true, composition is restarted with eCompositionStart.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Isn't this a regression of bug 1149172? Or the trigger could be bug 549674?

I guess that reverting attachment 8585527 [details] [diff] [review] fixes this bug.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Ugly visual glitch, tracking.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I tested with a build backing out attachment 8585527 [details] [diff] [review]. However, I can reproduce it (but the symptom is not exactly same, though).

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The patched build is here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a46db1fdb26d

Comment 27

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

While this is a valid issue, I have been waiting to get more end-user feedback on this. So far it does not seem to be severe enough to need a dot release. This is now a wontfix for 41.

Comment 28

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

Attachment: Don't commit Java composition when Gecko side commits its composition;

I think this patch is a decent workaround.

Right now, when the Gecko side commits its composition, we notify the
Java side and the Java side commits its composition as well. However,
some keyboards have trouble dealing with us manually committing the
composition. To better support these keyboards, this patch makes it so
that even if the Gecko composition is committed, the Java composition
is still kept. Because the Gecko composition on Android is used only for
display purposes, it doesn't matter too much when the Gecko composition
is out-of-sync with the Java composition.

Comment 29

[Eugen Sawin [:esawin]]

So how do we make sure that the text reflects the changes on the Java side?
I assume the old band-aid of restarting the keyboard wouldn't help in this case?

Comment 30

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

(In reply to Eugen Sawin [:esawin] from comment #29)
> So how do we make sure that the text reflects the changes on the Java side?
> I assume the old band-aid of restarting the keyboard wouldn't help in this
> case?

The text is still the same on both sides, and is not affected by this patch. This patch only changes whether or not part of the text is marked as a composition or not.

Comment 31

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

Attachment: Add test for text duplication when JS resets input value (v1)

Reproducing this bug in a testcase is actually not as easy.

This patch adds a test to testInputConnection that reproduces this bug. Because
this bug involves JS running during editing, another input field is used on the
test page. Additional code was added to testInputConnection,
GeckoInputConnection, and GeckoViewComponent in order to reproduce the bug in
the new input field.

Comment 32

[Eugen Sawin [:esawin]]

Comment on attachment 8677574 [details] [diff] [review]
Add test for text duplication when JS resets input value (v1)

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

That's a lot of "production" code for a test, but it should be worth it given that we've regressed on similar bugs in the past.

Comment 33

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

(In reply to Eugen Sawin [:esawin] from comment #32)
> Comment on attachment 8677574 [details] [diff] [review]
> Add test for text duplication when JS resets input value (v1)
> 
> Review of attachment 8677574 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That's a lot of "production" code for a test, but it should be worth it
> given that we've regressed on similar bugs in the past.

Yeah. Unfortunately I didn't find a good way to avoid adding a private command to GeckoInputConnection.

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Too late for 42. Looks at the size of the patch, I think we should take this asap in aurora if we want to have to check potential regressions.

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71bd7be01147
https://hg.mozilla.org/integration/mozilla-inbound/rev/90ca2ac0a32f

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/71bd7be01147
https://hg.mozilla.org/mozilla-central/rev/90ca2ac0a32f

Comment 37

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

Comment on attachment 8676372 [details] [diff] [review]
Don't commit Java composition when Gecko side commits its composition;

Approval Request Comment

[Feature/regressing bug #]: N/A

[User impact if declined]: Duplicated text in some input fields like the Facebook comment box.

[Describe test coverage new/current, TreeHerder]: Locally, m-c

[Risks and why]: Some risk; the altered behavior could cause regressions but that is unlikely.

[String/UUID change made/needed]: None

Comment 38

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

Jim, based on comment 36, I think this patch is already on Aurora44. Are you sure we still need to uplift it?

Comment 39

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

Sorry, we just need it for Beta then.

Comment 40

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

Comment on attachment 8676372 [details] [diff] [review]
Don't commit Java composition when Gecko side commits its composition;

Clearing out the aurora flag as this is already on FF44

Comment 41

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8676372 [details] [diff] [review]
Don't commit Java composition when Gecko side commits its composition;

Let's aim to get this on beta 2 so we can have time to spot possible regressions before release.

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

It may also be good to verify this fix since we know how to reproduce it.

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/eae5e25b18b5
https://hg.mozilla.org/releases/mozilla-beta/rev/6a0b1216e1a5

Comment 44

[miralobontiu]

Hi,

I was not able to reproduce the issue.

Tested with:
Browser / Version: Firefox Mobile Nightly 64.0a1 (2018-09-11)
Operating System: HTC 10 (Android 8)

Therefore, I will remove the qe-verify+ flag.

Thank you!