Closed Bug 764193 Opened 9 years ago Closed 8 years ago

Allow form autocomplete popup after user starts typing

Categories

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

15 Branch
ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 17
Tracking Status
firefox14 --- unaffected
firefox15 + wontfix
firefox16 + verified
firefox17 --- verified
blocking-fennec1.0 --- -
fennec 15+ ---

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Depends on 3 open bugs)

Details

Attachments

(2 files)

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.
This bug should probably be blocking-fennec1.0=.N+
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → .N+
Blocks: 764034
Target Milestone: --- → Firefox 16
Blocks: 725007
Depends on: 768105
Depends on: 768106
Depends on: 768108
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.
blocking-fennec1.0: .N+ → ?
tracking-fennec: --- → 15+
blocking-fennec1.0: ? → -
WIP: Commit composition string before sending autocomplete notification.
Priority: -- → P2
If we fix this bug in Firefox 16, we should uplift the fix for TouchPal bug 768106, too.
Depends on: 775850
Blocks: 775854
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)
Attachment #644203 - Flags: review?(blassey.bugs)
Attachment #644203 - Flags: review?(blassey.bugs) → review+
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
(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). :)
(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`.
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.
Target Milestone: Firefox 16 → Firefox 17
(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.
https://hg.mozilla.org/mozilla-central/rev/b6b76a38e015
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Aurora/Beta approval?
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
Attachment #644203 - Flags: approval-mozilla-aurora?
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.
Attachment #644203 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Correct me if I'm wrong, looks like this is wont-fix on mozilla-beta 15
Thanks for fixing the status. You are correct.
Blocks: 768105
No longer depends on: 768105
You need to log in before you can comment on or make changes to this bug.