Last Comment Bug 672661 - Backspace key in Swype or Swiftkey X causes characters to be duplicated
: Backspace key in Swype or Swiftkey X causes characters to be duplicated
Status: RESOLVED FIXED
[inbound]
: inputmethod, mobile, relnote
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: mozilla9
Assigned To: Alex Pakhotin (:alexp)
:
:
Mentors:
: 676427 687174 (view as bug list)
Depends on: 676275
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-19 15:09 PDT by Matt Brubeck (:mbrubeck)
Modified: 2012-02-10 15:35 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
[WIP] Draft fix (4.76 KB, patch)
2011-08-05 21:02 PDT, Alex Pakhotin (:alexp)
no flags Details | Diff | Splinter Review
[WIP] Fix (6.43 KB, patch)
2011-08-18 18:26 PDT, Alex Pakhotin (:alexp)
no flags Details | Diff | Splinter Review
[WIP] Patch v3 (7.38 KB, patch)
2011-08-23 19:21 PDT, Alex Pakhotin (:alexp)
no flags Details | Diff | Splinter Review
[WIP] Patch v4 (9.15 KB, patch)
2011-08-30 16:30 PDT, Alex Pakhotin (:alexp)
no flags Details | Diff | Splinter Review
Patch v5 (9.44 KB, patch)
2011-09-16 16:26 PDT, Alex Pakhotin (:alexp)
no flags Details | Diff | Splinter Review
Patch v5 (9.36 KB, patch)
2011-09-19 16:22 PDT, Alex Pakhotin (:alexp)
blassey.bugs: review+
Details | Diff | Splinter Review

Description Matt Brubeck (:mbrubeck) 2011-07-19 15:09:37 PDT
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) 2011-07-19 15:24:58 PDT
Assigning this to myself, but let me know if anyone else wants to work on it.
Comment 2 Matt Brubeck (:mbrubeck) 2011-07-19 15:26:52 PDT
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?) 2011-07-21 10:23:24 PDT
Matt, are you actively looking at this? If not, you can assign it to alexp.
Comment 4 Alex Pakhotin (:alexp) 2011-07-22 21:26:49 PDT
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] 2011-08-03 00:46:56 PDT
I don't have this ime, but I believe that this may be fixed by bug 667927.
Comment 6 Chetan 2011-08-03 02:31:17 PDT
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] 2011-08-03 03:23:24 PDT
We support Android 2.0 or later into single binary, so we cannot use API L9
Comment 8 Chetan 2011-08-03 03:29:21 PDT
You can compile against API level 9 (or even 13, for that matter) but still target API 5.
Comment 9 Matt Brubeck (:mbrubeck) 2011-08-03 08:13:32 PDT
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) 2011-08-03 09:21:22 PDT
(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) 2011-08-03 10:25:45 PDT
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 12 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-08-04 10:31:58 PDT
*** Bug 676427 has been marked as a duplicate of this bug. ***
Comment 13 Alex Pakhotin (:alexp) 2011-08-05 21:02:13 PDT
Created attachment 551230 [details] [diff] [review]
[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 Jim Chen [:jchen] [:darchons] 2011-08-08 13:32:38 PDT
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) 2011-08-18 18:26:06 PDT
Created attachment 554283 [details] [diff] [review]
[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) 2011-08-23 19:21:33 PDT
Created attachment 555299 [details] [diff] [review]
[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) 2011-08-25 19:13:54 PDT
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) 2011-08-30 16:30:05 PDT
Created attachment 557031 [details] [diff] [review]
[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) 2011-09-16 16:26:14 PDT
Created attachment 560646 [details] [diff] [review]
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) 2011-09-19 16:22:18 PDT
Created attachment 561070 [details] [diff] [review]
Patch v5

Minor change with update to the latest code.
Comment 22 Alex Pakhotin (:alexp) 2011-09-23 19:49:19 PDT
*** Bug 687174 has been marked as a duplicate of this bug. ***
Comment 23 Ed Morley [:emorley] 2011-09-23 20:49:31 PDT
https://hg.mozilla.org/mozilla-central/rev/878c2c7f8159

Note You need to log in before you can comment on or make changes to this bug.