735469 - Autocomplete list not filtered until space is entered

Status: VERIFIED FIXED

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

Description

[Chris Peterson [:cpeterson]]

STR:
1. On a Galaxy Nexus, load http://people.mozilla.org/~mwargers/tests/autocomplete_forms/textinput_form.html
2. In the "Submit Query" text box, enter a phrase like "apple juice" and hit the submit button.
3. Now enter a different phase like "banana peel".

AR:
On my Galaxy Nexus, the autocomplete menu does not filter the suggestion list until I enter a space. I see the "apple juice" suggestion until I enter the space after the word "banana".

ER:
On my Galaxy S II, autocomplete seems to work correctly. The "apple juice" suggestion is filtered after typing just the letter "b".

---

This problem may be related to the ICS IME's own autocomplete suggestions preempting our own.

Comment 2

[:Margaret Leibovic]

(In reply to Chris Peterson (:cpeterson) from comment #0)

> This problem may be related to the ICS IME's own autocomplete suggestions
> preempting our own.

We're generating our autocomplete suggestions based on DOM events, so I don't think this problem would be related to the ICS IME's autocomplete suggestions, unless that's interfering with how we're updating element.value. As I said in bug 730478, I noticed that element.value was wrong when I added some logging in here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3050

Comment 3

[Brian Nicholson (:bnicholson)]

I don't believe this is specific to the Galaxy Nexus; I have the same problem on my Droid Razr.

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: patch

After playing around with the IME code for a few days, I came up with this patch which seems to improve/fix several things:
- the autocompletion space issue described in this bug
- text that is being composed is now underlined
- deleting words in swype is fixed (previously, holding backspace to delete words in Swype resulted in partial deletions and other weird glitches)

We were creating an IME_SET_TEXT event for ranges, which resulted in setting the text twice. I replaced this with IME_ADD_RANGE.

For some reason, we were sending an IME_DELETE_TEXT event when count == 0 in the TextWatcher. I found that changing IME_SET_TEXT->IME_ADD_RANGE as described above caused text to not appear after deleting a word, and this IME_DELETE_TEXT event was the culprit. Removing this fixed the problem (and had the side effect of fixing Swype deletions).

Finally, with these changes, I noticed that when typing a letter and then deleting it, the autocomplete list didn't reappear (which I imagine we want to do since we're returning the text field to its initial state). I found that manually calling endComposition() fixed this.

Comment 5

[Chris Peterson [:cpeterson]]

Comment on attachment 607406 [details] [diff] [review]
patch

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

::: mobile/android/base/GeckoEvent.java
@@ +369,5 @@
>      public static GeckoEvent createIMERangeEvent(int offset, int count,
>                                                   int rangeType, int rangeStyles,
>                                                   int rangeForeColor, int rangeBackColor) {
>          GeckoEvent event = new GeckoEvent(IME_EVENT);
> +        event.InitIMERange(IME_ADD_RANGE, offset, count, rangeType, rangeStyles,

1. Should the other createIMERangeEvent(..., String text) method be changed from IME_SET_TEXT to IME_ADD_RANGE, too?

2. If so, then I think the createIMERangeEvent(..., String text) method should call this createIMERangeEvent() method instead of duplicating the InitIMERange() call.

3. And then InitIMERange() could be merge into its only caller, createIMERangeEvent().

Comment 6

[Chris Peterson [:cpeterson]]

Comment on attachment 607406 [details] [diff] [review]
patch

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

r=cpeterson.

You can ignore my previous feedback. I see that the other createIMERangeEvent() method should continue to use IME_SET_TEXT.

Comment 7

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/17b7af74d069

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/17b7af74d069

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/74abbab2458b

Comment 10

[Paul Feher]

Verified/fixed on:

Aurora Fennec 13.0a2 (2012-03-27)
Nightly Fennec 14.0a1 (2012-03-27)
Device: Samsung Nexus S
OS: Android 2.3.6