1149189 - Duplication when setting the same composing text multiple times

Status: RESOLVED FIXED

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

Description

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

There's a bug that when InputConnection.setComposingText is called multiple times with the same composing text, the text ends up being duplicated.

Comment 1

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

Attachment: Backout e75d824c730d "Force Gecko update on endBatchEdit for range updates"

I think this bug was the real cause for bug 1133802, so I want to backout the fix from bug 1133802. But let me know if you think we should keep the fix.

Comment 2

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

Attachment: Add dummy text change when setting composition to the same text (v1)

Basically when we set the same composing text again, we don't generate a text change notification that the Java side relies on. This patch makes us manually generate a text change notification.

Comment 3

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

Attachment: Add test for setting the same composing text (v1)

Add a test to check setting the same composing text in testInputConnection.

Comment 4

[Eugen Sawin [:esawin]]

Comment on attachment 8585556 [details] [diff] [review]
Add dummy text change when setting composition to the same text (v1)

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

Looks good.

::: widget/android/nsWindow.cpp
@@ +2260,5 @@
> +    return NS_OK;
> +}
> +
> +void
> +nsWindow::AddIMETextChange(IMEChange aChange) {

Let's accept it as a const reference, no need for a copy.

::: widget/android/nsWindow.h
@@ +187,5 @@
> +                       "IMEChange initialized with wrong notification");
> +            MOZ_ASSERT(aIMENotification.mTextChangeData.IsInInt32Range(),
> +                       "The text change notification is out of range");
> +        }
> +        bool IsEmpty()

Make that const, while we're at it.

@@ +197,3 @@
>      nsRefPtr<mozilla::TextComposition> GetIMEComposition();
>      void RemoveIMEComposition();
> +    void AddIMETextChange(IMEChange aChange);

Const reference argument.

Comment 5

[Eugen Sawin [:esawin]]

Comment on attachment 8585560 [details] [diff] [review]
Add test for setting the same composing text (v1)

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

Looks OK, we could expand it a little more.

::: mobile/android/base/tests/testInputConnection.java
@@ +93,5 @@
> +            ic.deleteSurroundingText(6, 0);
> +            assertTextAndSelectionAt("Can clear text", ic, "", 0);
> +
> +            // Bug 1133802, duplication when setting the same composing text more than once.
> +            ic.setComposingText("baz", 1);

Can we assert here before finishing?

@@ +94,5 @@
> +            assertTextAndSelectionAt("Can clear text", ic, "", 0);
> +
> +            // Bug 1133802, duplication when setting the same composing text more than once.
> +            ic.setComposingText("baz", 1);
> +            ic.setComposingText("baz", 1);

How about alternating identical/non-identical text to expand the test case?

Comment 6

[Eugen Sawin [:esawin]]

Comment on attachment 8585555 [details] [diff] [review]
Backout e75d824c730d "Force Gecko update on endBatchEdit for range updates"

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

Sending the dummy text change notification makes the forced syncing redundant, so let's get rid of it!

Comment 7

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

Addressed review comments.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bbd84b48882
https://hg.mozilla.org/integration/mozilla-inbound/rev/051a7c4f26ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/e493bd1eda41

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0bbd84b48882
https://hg.mozilla.org/mozilla-central/rev/051a7c4f26ff
https://hg.mozilla.org/mozilla-central/rev/e493bd1eda41