764193 - Allow form autocomplete popup after user starts typing

Status: VERIFIED FIXED

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

Description

[Chris Peterson [:cpeterson]]

In bug 758820, I disabled the form autocomplete popup after the user starts typing. This bug is track work necessary to re-enable form autocomplete after the user starts typing.

Some example problems when form autocomplete is enabled:
  * Most VKBs with suggestions will ignore autocomplete text when an autocomplete entry is selected mid-word.
  * Swype will select the current word or move the cursor when an autocomplete entry is selected mid-word.

Comment 1

[Chris Peterson [:cpeterson]]

This bug should probably be blocking-fennec1.0=.N+

Comment 2

[Chris Peterson [:cpeterson]]

I don't think I will be able to fix this bug and all its blocking bugs before Fennec 14.1.

Form autocomplete is currently disabled after the user starts typing in a text input form. If we re-enable the form autocomplete popup, numerous IME-specific bugs are revealed. Swype, Hacker's Keyboard, TouchPal Keyboard, and OpenWnn Plus all have problems and they all misbehave differently. Thus, I don't think we can safely re-enable the form autocomplete popup (after the user starts typing) until ALL those IME-specific bugs are fixed.

We currently display the form autocomplete popup until the user starts typing, so we are not losing a lot of functionality when we disable the popup after the user starts typing.

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: WIP-enable-autocomplete-popup.patch

WIP: Commit composition string before sending autocomplete notification.

Comment 4

[Chris Peterson [:cpeterson]]

If we fix this bug in Firefox 16, we should uplift the fix for TouchPal bug 768106, too.

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: blocklist-some-IMEs.patch

Blocklist form autocomplete for some IMEs:

 * Google Japanese Input (bug 775850)
 * OpenWnn Plus (bug 768108)
 * Simeji (bug 768108)
 * Swype (bug 755909)
 * Swype Beta (bug 755909)

Comment 6

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

Comment on attachment 644203 [details] [diff] [review]
blocklist-some-IMEs.patch

>diff --git a/mobile/android/base/GeckoInputConnection.java b/mobile/android/base/GeckoInputConnection.java

>+            if (DEBUG && blocklisted) {
>+                Log.d(LOGTAG, "FormAssist: Blocklisting \"" + mCurrentInputMethod + "\" IME");
>+            }
>+            GeckoApp.mAppContext.mFormAssistPopup.block(blocklisted);

Should this be null checked? I am more paranoid since we made things non-static in GeckoApp.


>+    private static String getCurrentInputMethod() {
>+        return Secure.getString(GeckoApp.mAppContext.getContentResolver(),
>+                                Secure.DEFAULT_INPUT_METHOD);

Same

Comment 7

[Chris Peterson [:cpeterson]]

(In reply to Mark Finkle (:mfinkle) from comment #6)
> >+            GeckoApp.mAppContext.mFormAssistPopup.block(blocklisted);
> 
> Should this be null checked? I am more paranoid since we made things
> non-static in GeckoApp.

I'll null check mFormAssistPopup, but I'm concerned that we're uglifying our code because we don't have a good understanding of GeckoApp's Activity lifetime (which probably an important thing to know). :)

Comment 8

[Chris Peterson [:cpeterson]]

(In reply to Mark Finkle (:mfinkle) from comment #6)
> >+    private static String getCurrentInputMethod() {
> >+        return Secure.getString(GeckoApp.mAppContext.getContentResolver(),
> >+                                Secure.DEFAULT_INPUT_METHOD);
> 
> Same

GeckoApp initializes `mAppContext = this` in onCreate() and never resets `mAppContext = null`.

Comment 9

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b76a38e015

Form autocomplete popup has been re-enabled, but some IMEs are now blocklisted until their individual bugs are fixed.

Comment 10

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

(In reply to Chris Peterson (:cpeterson) from comment #7)
> (In reply to Mark Finkle (:mfinkle) from comment #6)
> > >+            GeckoApp.mAppContext.mFormAssistPopup.block(blocklisted);
> > 
> > Should this be null checked? I am more paranoid since we made things
> > non-static in GeckoApp.
> 
> I'll null check mFormAssistPopup, but I'm concerned that we're uglifying our
> code because we don't have a good understanding of GeckoApp's Activity
> lifetime (which probably an important thing to know). :)

Perhaps we could add getters instead of accessing the data members directly. The getters could new the data member if it is null.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b6b76a38e015

Comment 12

[Aaron Train [:aaronmt]]

Aurora/Beta approval?

Comment 13

[Chris Peterson [:cpeterson]]

Comment on attachment 644203 [details] [diff] [review]
blocklist-some-IMEs.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Form autocomplete will still be disabled once user starts typing in text field.
Testing completed (on m-c, etc.): Baked on m-c for 3 days without complaints or new autocomplete bugs.
Risk to taking this patch (and alternatives if risky): Some buggy IMEs break form autocomplete, but this patch blocklists those IMEs for now: Swype, Swype Beta, OpenWnn Plus, and Google Japanese Input Beta.
String or UUID changes made by this patch: None

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 644203 [details] [diff] [review]
blocklist-some-IMEs.patch

Looks low risk enough to land on Aurora at this point in the cycle. Will add qawanted to verify this does what's intended.

Comment 15

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1ea57f0d6d85

Comment 16

[Aaron Train [:aaronmt]]

Correct me if I'm wrong, looks like this is wont-fix on mozilla-beta 15

Comment 17

[Chris Peterson [:cpeterson]]

Thanks for fixing the status. You are correct.