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

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
4 years ago
6 months ago

People

(Reporter: hsteen, Assigned: jchen)

Tracking

unspecified
Firefox 44
Points:
---

Firefox Tracking Flags

(firefox41- wontfix, firefox42+ wontfix, firefox43+ fixed, firefox44+ fixed, fennec43+)

Details

(URL)

Attachments

(4 attachments, 1 obsolete attachment)

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

Comment 1

4 years ago
Posted file fb-typing.html (obsolete) —
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..?
Jim, do you know what's going on here?
Flags: needinfo?(nchen)
(Reporter)

Comment 4

4 years ago
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}
Attachment #8654133 - Attachment is obsolete: true
(Reporter)

Comment 5

4 years ago
It's not unlikely to be similar to bug 549674 (just guessing though) but the fix for that one doesn't handle this case.
See Also: → bug 549674
(Reporter)

Comment 6

4 years ago
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 ;)
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.
(Reporter)

Comment 9

4 years ago
Like this?

The event log.. wow! It sure looks like a mess :-p
Flags: needinfo?(masayuki)
(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]
Flags: needinfo?(masayuki)
(Reporter)

Comment 11

4 years ago
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.
tracking-fennec: --- → ?
tracking-firefox43: --- → ?
Tracking for 43. Do you know if other versions are affected?
status-firefox42: --- → ?
status-firefox43: --- → affected
tracking-firefox43: ? → +
(Reporter)

Comment 13

4 years ago
All versions are affected. If a fix earlier than in 43 is possible it would of course be good.
tracking-fennec: ? → 43+

Comment 14

4 years ago
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?
Assignee: nobody → administration
Blocks: 869192
Flags: needinfo?(hkirschner)
(Reporter)

Comment 15

4 years ago
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.
+1 for :hallvors recommendation to fix this on our side. As far as I can tell it seems like a regression on our side.
Flags: needinfo?(hkirschner)
status-firefox41: --- → affected
status-firefox42: ? → affected
status-firefox44: --- → affected
tracking-firefox41: --- → ?
tracking-firefox42: --- → ?
tracking-firefox44: --- → +
Margaret, can you help find an owner for this one?
Flags: needinfo?(margaret.leibovic)
Eugen, would you be able to take a look at this?
Flags: needinfo?(margaret.leibovic) → needinfo?(esawin)
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.
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]
forgot to mention, I see basically no messages in logcat, and I get this behaviour on both Swift keyboard and Google keyboard.
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.
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.
Ugly visual glitch, tracking.
tracking-firefox42: ? → +
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).
Assignee: administration → nchen
Status: NEW → ASSIGNED
Flags: needinfo?(nchen)
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.
status-firefox41: affected → wontfix
tracking-firefox41: ? → -
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.
Attachment #8676372 - Flags: review?(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?
Flags: needinfo?(esawin)
(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.
Attachment #8676372 - Flags: review?(esawin) → review+
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.
Attachment #8677574 - Flags: review?(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.
Attachment #8677574 - Flags: review?(esawin) → review+
(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.
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.
status-firefox42: affected → wontfix
https://hg.mozilla.org/mozilla-central/rev/71bd7be01147
https://hg.mozilla.org/mozilla-central/rev/90ca2ac0a32f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
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
Attachment #8676372 - Flags: approval-mozilla-beta?
Attachment #8676372 - Flags: approval-mozilla-aurora?
Jim, based on comment 36, I think this patch is already on Aurora44. Are you sure we still need to uplift it?
Flags: needinfo?(nchen)
Sorry, we just need it for Beta then.
Flags: needinfo?(nchen)
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
Attachment #8676372 - Flags: approval-mozilla-aurora?
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.
Attachment #8676372 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
It may also be good to verify this fix since we know how to reproduce it.
Flags: qe-verify+

Comment 44

6 months ago
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!
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.