The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla9

Status

()

Core
Widget: Android
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mbrubeck, Assigned: alexp)

Tracking

({inputmethod, mobile, relnote})

Trunk
mozilla9
All
Android
inputmethod, mobile, relnote
Points:
---

Firefox Tracking Flags

(fennec+)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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".
(Reporter)

Comment 1

6 years ago
Assigning this to myself, but let me know if anyone else wants to work on it.
Assignee: nobody → mbrubeck
(Reporter)

Comment 2

6 years ago
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.
Matt, are you actively looking at this? If not, you can assign it to alexp.
tracking-fennec: ? → 8+
(Reporter)

Updated

6 years ago
Assignee: mbrubeck → alexp
(Assignee)

Comment 4

6 years ago
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.
I don't have this ime, but I believe that this may be fixed by bug 667927.

Comment 6

6 years ago
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.
We support Android 2.0 or later into single binary, so we cannot use API L9

Comment 8

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

Comment 9

6 years ago
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...
(Assignee)

Comment 10

6 years ago
(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!
(Reporter)

Updated

6 years ago
Depends on: 676275
(Assignee)

Comment 11

6 years ago
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.
Duplicate of this bug: 676427
(Reporter)

Updated

6 years ago
Keywords: relnote
(Assignee)

Comment 13

6 years ago
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?
Attachment #551230 - Flags: feedback?(nchen)
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.
Attachment #551230 - Flags: feedback?(nchen)
tracking-fennec: 8+ → +
(Assignee)

Comment 15

6 years ago
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.
Attachment #551230 - Attachment is obsolete: true
Attachment #554283 - Flags: feedback?(nchen)
(Assignee)

Comment 16

6 years ago
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.
Attachment #554283 - Attachment is obsolete: true
Attachment #554283 - Flags: feedback?(nchen)
(Assignee)

Comment 17

6 years ago
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.
(Assignee)

Comment 18

6 years ago
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.
Attachment #555299 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 19

6 years ago
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.
Attachment #557031 - Attachment is obsolete: true
(Assignee)

Comment 20

6 years ago
Created attachment 561070 [details] [diff] [review]
Patch v5

Minor change with update to the latest code.
Attachment #560646 - Attachment is obsolete: true
Attachment #561070 - Flags: review?(doug.turner)
(Assignee)

Updated

6 years ago
Attachment #561070 - Flags: review?(doug.turner) → review?(blassey.bugs)
Attachment #561070 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 21

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/878c2c7f8159
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
(Assignee)

Updated

6 years ago
Duplicate of this bug: 687174
https://hg.mozilla.org/mozilla-central/rev/878c2c7f8159
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
(Reporter)

Updated

6 years ago
Summary: Backspace key in Swiftkey X causes characters to be duplicated → Backspace key in Swype or Swiftkey X causes characters to be duplicated
You need to log in before you can comment on or make changes to this bug.