672661 - Backspace key in Swype or Swiftkey X causes characters to be duplicated

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Matt Brubeck (:mbrubeck)]

Steps to reproduce:
1. Install and activate the "Swiftkey X Phone" keyboard from https://market.android.com/details?id=com.touchtype.swiftkey
2. Open a page with an input field, e.g. data:text/html,<input>
3. In the input field, type two words (e.g. "foo bar").
4. Press backspace repeatedly to delete one character at a time.

Expected results: After deleting " bar", the field contains "foo".

Actual results: After deleting " bar", the field changes to contain "ffoo".

Comment 1

[Matt Brubeck (:mbrubeck)]

Assigning this to myself, but let me know if anyone else wants to work on it.

Comment 2

[Matt Brubeck (:mbrubeck)]

Paul, is TouchType aware of this problem?  I haven't investigated yet whether it is a SwiftKey bug or a Firefox bug, but I'd be happy to work with your developers on it either way.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Matt, are you actively looking at this? If not, you can assign it to alexp.

Comment 4

[Alex Pakhotin (:alexp)]

I'm looking into this issue, not much progress, just some update after analyzing the debug logs.

SwiftKey X generates much more IME events compared to other virtual keyboards. In my tests the log for the same actions with SwiftKey was about 2 times longer than with the stock keyboard and 3 times longer compared to Smart Keyboard. Apparently we do not handle such sequence of events properly.

One distinctive feature of SwiftKey X is that with this keyboard Gecko receives IME_SET_TEXT event on a Backspace, while with other keyboards, there is no such event. Somehow the text received with that SET_TEXT is appended to the existing string instead of overwriting it. I need more debugging to understand why this happens.

Comment 5

[Makoto Kato [:m_kato]]

I don't have this ime, but I believe that this may be fixed by bug 667927.

Comment 6

[Chetan]

Hi all,

I made these comment on bug 617298, but I believe they belong here:


Hi guys,

We just [] tested Firefox and Firefox Beta from the Android market and with the new SwiftKey Beta (not yet available to the world). It seems to work well apart from two things. On >= Android 2.3 we use InputConnection.setComposingRegion to set the current word being edited (e.g. after backspacing) but Firefox ignores it! On < 2.3 we delete the characters and re-insert the text to mimic this behaviour, but this seems to cause Firefox to get stuck in a loop of entering and removing the text.

It seems like Firefox has its own implementation of InputConnection. Can you update your implementation to resolve these issues?

Regards,
Chetan. (Software Engineer @ TouchType)


---


Currently Firefox builds with Android API level 5 (see: https://mxr.mozilla.org/mozilla-central/source/embedding/android/AndroidManifest.xml.in). InputConnection.setComposingRegion was introduced in API level 9.

Firefox must be compiled against API >= 9 and https://mxr.mozilla.org/mozilla-central/source/embedding/android/GeckoInputConnection.java must override and implement setComposingRegion. See http://developer.android.com/reference/android/view/inputmethod/BaseInputConnection.html#setComposingRegion(int,%20int) for documentation.
(BaseInputConnection already implements this but Firefox must override this implementation to pass the event to Gecko.)

Regards,
Chetan.


-----

Makoto Kato, I believe the fix in bug 667927 will not have any effect on this bug.

Regards,
Chetan.

Comment 7

[Makoto Kato [:m_kato]]

We support Android 2.0 or later into single binary, so we cannot use API L9

Comment 8

[Chetan]

You can compile against API level 9 (or even 13, for that matter) but still target API 5.

Comment 9

[Matt Brubeck (:mbrubeck)]

Our automated builders currently use the Android SDK r8.  We will need to update them to r9 or later in order to use methods from API level 9.  We need to do this anyway (for other reasons like bug 675901 and bug 664149), so I'll file a bug to get this started...

Comment 10

[Alex Pakhotin (:alexp)]

(In reply to comment #6)
> I made these comment on bug 617298, but I believe they belong here:

Thank you Chetan - I missed that bug, as it was marked as fixed a while ago.

> On >= Android 2.3 we use
> InputConnection.setComposingRegion to set the current word being edited
> (e.g. after backspacing) but Firefox ignores it! On < 2.3 we delete the
> characters and re-insert the text to mimic this behaviour, but this seems to
> cause Firefox to get stuck in a loop of entering and removing the text.

OK, now it starts making sense to me. Thanks!

Comment 11

[Alex Pakhotin (:alexp)]

I've tried Android 2.2. There are still some issues with SwiftKey X in Fennec, but at least it's possible to delete the entered text with Backspace.

Comment 13

[Alex Pakhotin (:alexp)]

Attachment: [WIP] Draft fix

This WIP fix seems to improve the behavior with SwiftKey X on Android 2.3+. Needs more testing though.
Jim, what do you think about this approach?

Comment 14

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

Comment on attachment 551230 [details] [diff] [review]
[WIP] Draft fix

The concept looks good. See, IME is not hard :)

>+    @Override
>+    public boolean setComposingRegion (int start, int end) {
>+        //Log.d("GeckoAppJava", "IME: setComposingRegion(start=" + start + ", end=" + end + ")");
>+        int selStart = mCompositionSelStart;
>+
>+        finishComposingText();
>+
>+        mCompositionStart = start;
>+        mCompositionSelStart = 0;
>+        mCompositionSelLen = end - start;
>+
>+        mForcedCompositionStart = start;
>+        mForcedCompositionEnd = end;
>+
>         return true;
>     }

You shouldn't need to set mCompositionStart, mCompositionSelStart, and mCompositionSelLen here because they're reset in setComposingText

Also, I still think you should start a composition inside setComposingRegion, instead of waiting until setComposingText. That way, Gecko is on the same page. For example, if you just call setComposingRegion, without calling setComposingText, Gecko will not drawing the underlines because it thinks we are not in a composition, but setComposingRegion implies we are.

Comment 15

[Alex Pakhotin (:alexp)]

Attachment: [WIP] Fix

This patch fixes the issue on Android 2.3.
Jim, could you have a look please - it's based on what we discussed today.

I still need to debug it on 2.2.

Comment 16

[Alex Pakhotin (:alexp)]

Attachment: [WIP] Patch v3

Another approach - without changing setComposingText function, but call it from setComposingRegion. Also added handlers for beginBatchEdit/endBatchEdit to postpone the notifications from Gecko until the end of the batch operation to avoid message ping-pong between Gecko and IME.

This implementation still has issues in some cases, for example sometimes punctuation marks somehow disappear when entered.

Comment 17

[Alex Pakhotin (:alexp)]

Now it looks like I hit an IPC bug. The problem with some characters entering happens in the content process only. An edit box in the chrome process works fine.
Might be bug 653895.

Comment 18

[Alex Pakhotin (:alexp)]

Attachment: [WIP] Patch v4

Another WIP patch.
This one fixes a problem on Android 2.2.
Most of the time works on the 2.3+ as well, but in some specific cases there are still issues when entering punctuation marks. This seems to be related to IPC, as it happens only in the remote process. Text boxes in the chrome process work fine.

Comment 19

[Alex Pakhotin (:alexp)]

Attachment: Patch v5

The patch fixes the backspace issue on both 2.2 and 2.3+ devices.

The way how we currently handle IME events in the Java/bridge layer is very fragile, and a fix for one case frequently breaks something else. This is due to asynchronous nature of the IME-related events and IPC (no problem in the chrome process).
With the latest patch the only problem I've noticed in my tests is that sometimes on 2.3 the last entered symbol stays selected. It happens quite rarely, and seems to affect only some special symbols.

This is as far as we can go without major changes. Bug 653895, when fixed, should improve the situation.

Comment 20

[Alex Pakhotin (:alexp)]

Attachment: Patch v5

Minor change with update to the latest code.

Comment 21

[Alex Pakhotin (:alexp)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/878c2c7f8159

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/878c2c7f8159