Last Comment Bug 695444 - Form history
: Form history
Status: VERIFIED FIXED
[QA+]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P3 normal (vote)
: ---
Assigned To: Sriram Ramasubramanian [:sriram]
:
Mentors:
Depends on: 710837 710835 711096 711177 711181 711185 711198 711216 711227 713027 717619 717679 718684
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-18 12:36 PDT by Erin Lancaster [:elan]
Modified: 2014-10-31 10:24 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
wip (3.53 KB, patch)
2011-11-02 15:37 PDT, :Margaret Leibovic
mark.finkle: feedback+
Details | Diff | Review
stock browser on gingerbread (53.28 KB, image/png)
2011-11-04 11:23 PDT, Mark Finkle (:mfinkle) (use needinfo?)
no flags Details
WIP (25.15 KB, patch)
2011-12-06 12:49 PST, Sriram Ramasubramanian [:sriram]
no flags Details | Diff | Review
WIP v2 (26.94 KB, patch)
2011-12-06 16:04 PST, :Margaret Leibovic
no flags Details | Diff | Review
WIP v3 (27.27 KB, patch)
2011-12-06 16:42 PST, Sriram Ramasubramanian [:sriram]
no flags Details | Diff | Review
patch (27.30 KB, patch)
2011-12-07 19:13 PST, :Margaret Leibovic
mark.finkle: review+
Details | Diff | Review
patch to land (un-bitrotted) (27.28 KB, patch)
2011-12-14 11:59 PST, :Margaret Leibovic
no flags Details | Diff | Review

Description Erin Lancaster [:elan] 2011-10-18 12:36:51 PDT
* UI popup needed for auto-file
Comment 1 :Margaret Leibovic 2011-11-01 16:02:08 PDT
I started to look into this today, but I got a little overwhelmed/confused by trying to figure out what XUL fennec was doing and what changes we would need to make to get this to work on native fennec.

Mark, do you have a general idea of what needs to be done here? I think I could get rolling once I have some more guidance.
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-01 20:48:18 PDT
Margaret, the biggest issue with form history on mobile has always been the UI. We don't want to use the autocomplete popup that desktop uses. We ended up making our own "touch friendly" XUL arrowbox UI.

In XUL Fennec, you should see the Form Assistant (spread across some chrome JS and forms.js content script) trying to determine if history is available for a textbox and if so, displaying the history in a popup list.

Looking at the core FormHistory component, I see we need to instaniate the service to loadFrameScript. We do this for e10s and non-e10s:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/nsFormHistory.js#139

The frame script hooks up a listener, so we can save data when forms are submitted.

The UI used for desktop Firefox comes from a <panel id="PopupAutoComplete"/> in browser.xul that is eventually attached to a <browser> when created. Then a FormFillController is created and attached:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml#508

Since mobile doesn't use the panel approach and Java UI in particular won't use XUL, we handle things differently. We basically wait for a text field to become focused, then we use the nsIFormAutoComplete service to manually build a list of "suggestions" and show those in a popup UI:
http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/common-ui.js#811

We update the list on "keypress" (doesn't work well with IME) and "text" (works well with IME). I guess we could use some kind of list widget to show the list.

First things first, let's see if we can:
* Get the service started so we save data on submit
* Track accessing autocomplete-able textboxes (see forms.js)
* Get a list of suggestions for a textbox, updated as well type

Once we have this, we can work with UX to get a way to display the list. Sound OK?
Comment 3 :Margaret Leibovic 2011-11-02 09:20:06 PDT
(In reply to Mark Finkle (:mfinkle) from comment #2)
> Once we have this, we can work with UX to get a way to display the list.
> Sound OK?

Sounds like a plan! Thanks for the detailed comment.
Comment 4 :Margaret Leibovic 2011-11-02 15:37:03 PDT
Created attachment 571481 [details] [diff] [review]
wip

This patch does the basic first steps outlined in comment 2. I just tested with the google search box, but the dump statement shows we're getting autocomplete results like we should.

I just took the bare minimum from forms.js and common-ui.js to make this work. I figure we can add more functionality back in one piece at a time.
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-04 10:36:24 PDT
Comment on attachment 571481 [details] [diff] [review]
wip

> var FormAssistant = {
>+  init: function() {
>+    addEventListener("text", this, false);

BrowserApp.deck.addEventListener(...)

just to be more explicit

Looks good. I think we also need to handle the case of when a textbox gets focus. We usually "show" the suggestions, just to minimize user typing if suggestions exist.
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-04 11:23:13 PDT
Created attachment 572028 [details]
stock browser on gingerbread

example of what stock browser on gingerbread does for UI
Comment 7 Madhava Enros [:madhava] 2011-11-04 11:24:55 PDT
That example (of the native browser) is kind of minimum good UX, but it would do.
Comment 8 :Margaret Leibovic 2011-12-04 23:14:15 PST
Assigning to Sriram, since he's graciously taken over my work on this :)
Comment 9 Sriram Ramasubramanian [:sriram] 2011-12-06 12:49:53 PST
Created attachment 579419 [details] [diff] [review]
WIP

This WIP has the logic for showing the autocomplete list -- positioned relative to the textbox.
The width adjusts based on the size of the textbox
  - if the textbox extends outside the screen, the list uses a "fill_parent" mode
  - if not, the list's width is shrunk to the size of the textbox

The height is constant now (100dp) which needs some cleanup.
If the list might be hidden by the keyboard, the list is tried to be show above the textbox. In case it can't be shown about the textbox, it default to be hidden box the keyboard. -- This could be fixed by playing with height more.

There needs some cleanup in showing. I need to take a look at it to optimize few bits there.

Todo:
1. Fixing the height
2. Sending event back to Gecko
3. Updating textbox with the string from Java
4. Cleaning up browser.js
Comment 10 :Margaret Leibovic 2011-12-06 16:04:01 PST
Created attachment 579504 [details] [diff] [review]
WIP v2

I'm still seeing a few issues with the element value not being set correctly in the "FormAssist:AutoComplete" observer in FormAssistant, but this looks like it's most of the way there.
Comment 11 Sriram Ramasubramanian [:sriram] 2011-12-06 16:42:14 PST
Created attachment 579524 [details] [diff] [review]
WIP v3

Added some optimization to AutoCompletePopup.java. If it is already shown, the width and height are not calculated again for every event received from Gecko.

Also added a bit of animation ;)
Comment 12 :Margaret Leibovic 2011-12-07 19:13:26 PST
Created attachment 579938 [details] [diff] [review]
patch

This patch uses .blur() to remove focus from the input box before trying to set its value, and it appears to solve our problem.

A few minor changes from the last WIP:
-I renamed showPopup to show to be consistent with hide
-I got rid of our own isShowing method, since isShown is already a View method
Comment 13 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-07 23:07:44 PST
Comment on attachment 579938 [details] [diff] [review]
patch

I am running with this patch for a bit and will review soon.
Comment 14 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-13 19:14:56 PST
Comment on attachment 579938 [details] [diff] [review]
patch

This seems to work OK for a first pass. I think we can look into a few things as followups:

* I can see the popup list flicker as I type. I mean it will briefly appear then disappear because there is no match based on what is in the editbox
* Bluring might not be the best solution. We should ask around to see if anyone (alexp or lucasr) have ideas for other ways to fix setting the value.

Sorry for taking too long on this
Comment 15 :Margaret Leibovic 2011-12-14 11:59:16 PST
Created attachment 581737 [details] [diff] [review]
patch to land (un-bitrotted)
Comment 16 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-12-14 12:06:45 PST
http://hg.mozilla.org/mozilla-central/rev/b7bafe0967a9
Comment 17 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2012-01-12 10:17:36 PST
Verified as the form autocomplete feature is working now.

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