Closed Bug 1149189 Opened 5 years ago Closed 5 years ago

Duplication when setting the same composing text multiple times

Categories

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

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: jchen, Assigned: jchen)

Details

Attachments

(3 files)

There's a bug that when InputConnection.setComposingText is called multiple times with the same composing text, the text ends up being duplicated.
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.
Attachment #8585555 - Flags: review?(esawin)
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.
Attachment #8585556 - Flags: review?(esawin)
Add a test to check setting the same composing text in testInputConnection.
Attachment #8585560 - Flags: review?(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.
Attachment #8585556 - Flags: review?(esawin) → review+
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?
Attachment #8585560 - Flags: review?(esawin) → review+
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!
Attachment #8585555 - Flags: review?(esawin) → review+
https://hg.mozilla.org/mozilla-central/rev/0bbd84b48882
https://hg.mozilla.org/mozilla-central/rev/051a7c4f26ff
https://hg.mozilla.org/mozilla-central/rev/e493bd1eda41
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
You need to log in before you can comment on or make changes to this bug.