Closed
Bug 946022
(rAc-android)
Opened 10 years ago
Closed 7 years ago
Implement base requestAutocomplete UI for Android
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(fennec+)
RESOLVED
INVALID
Tracking | Status | |
---|---|---|
fennec | + | --- |
People
(Reporter: bnicholson, Unassigned)
References
(Depends on 1 open bug)
Details
(Keywords: feature)
Attachments
(1 file, 9 obsolete files)
23.81 KB,
patch
|
liuche
:
review+
mfinkle
:
feedback+
|
Details | Diff | Splinter Review |
Implementation specific to Firefox for Android; see bug 939351 for the core bug.
Reporter | ||
Comment 1•10 years ago
|
||
Not much here yet, but it'd be good to take an early look since I haven't done a lot of core development. I don't know who the best reviewer is, so I'm just requesting review from smaug since we had a brief discussion about this feature in #content.
Attachment #8342063 -
Flags: feedback?(bugs)
Reporter | ||
Comment 2•10 years ago
|
||
A rough, very much WIP patch. I'd like to split this so that a generic implementation can be used across products, and I'm not sure of the best way forward. Ideas I've considered so far: * Have a product-generic AutofillController along with product-specific AutofillPopup implementations. AutofillController calls AutofillPopup.show(), where (on Android) AutofillPopup will message the Java UI, somehow refer back to the AutofillController (or another component) for validation, then asynchronously call back to AutofillController after the form is submitted. * Alternatively, have a product-generic AutofillUtils that contains many util functions for iterating input fields on the page, validation, possibly storage, etc. We then have multiple product-specific AutofillControllers that use AutofullUtils to implement the correct behavior. Other questions: * Do we want validation to be implemented as a shared core component, or should it be product-specific? Currently, I'm leaning toward shared since validation might get pretty complicated, and we may not want to reimplement this logic for every product. The disadvantage of this would be that we'll have to do validation in Gecko, then update the UI asynchronously on Android. * Same question for storage -- again, I'm leaning toward a shared component. I'm not sure why the SQL databases would differ on any products; they'll all need to hold the billing/shipping/payment data, along with any sync data. Again, the has the disadvantage that data will need to be sent asynchronously between Gecko and the UI.
Attachment #8342070 -
Flags: feedback?(mark.finkle)
Attachment #8342070 -
Flags: feedback?(bugs)
Reporter | ||
Comment 3•10 years ago
|
||
Sample build for testing requestAutocomplete: https://dl.dropboxusercontent.com/u/35559547/fennec-autofill.apk This (mostly) works with the requestAutocomplete demo linked to by the docs: https://googledrive.com/host/0B28BnxIvH5DuWGc3Mm5sZE9DekE/ To make things easier to test, I've included some dummy data in the build to prepopulate the form.
Comment 4•10 years ago
|
||
Comment on attachment 8342063 [details] [diff] [review] Part 1: requestAutocomplete API changes There shouldn't be reason to add anything to nsIDOMHTMLFormElement.idl New stuff should go to .webidl. If we're planning to implement this first only on Android, we sure should have a pref for void requestAutocomplete(); So you should add [Pref="your.pref.name"] See https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#.5BPref.3Dprefname.5D In the DOM C++ code, use {} always with if
Attachment #8342063 -
Flags: feedback?(bugs)
Comment 5•10 years ago
|
||
Comment on attachment 8342063 [details] [diff] [review] Part 1: requestAutocomplete API changes >+[scriptable, uuid(cbf47f9d-a13d-4fad-8221-8964086b1b8a)] >+interface nsIAutofillController : nsISupports >+{ >+ void autocomplete(in nsIDOMHTMLFormElement form); >+}; I think we should reuse requestAutocomplete() for the method name, since this is actually a request.
Updated•10 years ago
|
relnote-firefox:
--- → ?
Comment 6•10 years ago
|
||
Comment on attachment 8342070 [details] [diff] [review] Part 2: WIP requestAutocomplete implementation >+ observe: function observe(aSubject, aTopic, aData) { >+ // Null data means the user cancelled; abort and send autocomplete error event. >+ if (!aData) { >+ Services.obs.removeObserver(this, "Autofill:Submit"); Is it guaranteed that we end up removing the observer always? Even in the strangest cases, since if not, we leak. >+ autocomplete: function autocomplete(aForm) { >+ this._win = aForm.ownerDocument.defaultView; >+ this._form = aForm; >+ >+ if (this._isActive || aForm.autocomplete === "off") { >+ let evt = new this._win.AutocompleteErrorEvent("autocompleteerror", { bubbles: true, reason: "disabled" }); this._win can be null here. Should requestAutocomplete() work with elements which aren't in document or not in a document with defaultView? + // TODO: Pull user and payment info from database + _getStoredData: function _getStoredData() { What will that look like? I hope it doesn't block the caller. Can we read the data off-the-Gecko-and-main-thread?
Attachment #8342070 -
Flags: feedback?(bugs)
Reporter | ||
Comment 7•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #6) > Comment on attachment 8342070 [details] [diff] [review] > Part 2: WIP requestAutocomplete implementation > > > >+ observe: function observe(aSubject, aTopic, aData) { > >+ // Null data means the user cancelled; abort and send autocomplete error event. > >+ if (!aData) { > >+ Services.obs.removeObserver(this, "Autofill:Submit"); > Is it guaranteed that we end up removing the observer always? Even in the > strangest cases, since if not, > we leak. Yes, we always need to remove this observer, so we need to be careful that we go through the completion path in all circumstances. Aside from the leak you pointed out, not reaching this code would also prevent this._isActive from getting reset, which would prevent any future requestAutocomplete requests from working. (The _isActive flag, in case you were curious, is used as specified by the algorithm in http://wiki.whatwg.org/wiki/RequestAutocomplete.) > >+ autocomplete: function autocomplete(aForm) { > >+ this._win = aForm.ownerDocument.defaultView; > >+ this._form = aForm; > >+ > >+ if (this._isActive || aForm.autocomplete === "off") { > >+ let evt = new this._win.AutocompleteErrorEvent("autocompleteerror", { bubbles: true, reason: "disabled" }); > this._win can be null here. Should requestAutocomplete() work with elements > which aren't in document or not in a document > with defaultView? Good question; the spec doesn't mention anything about forms not yet added to the document. I'll add yet another TODO comment to investigate. > + // TODO: Pull user and payment info from database > + _getStoredData: function _getStoredData() { > What will that look like? I hope it doesn't block the caller. Can we read > the data off-the-Gecko-and-main-thread? Yes, this should be async and off the Gecko thread. This will need to use some kind of protected database since it will be storing credit card information, though I'm not really familiar with our current options. I imagine this will overlap with the new Firefox accounts work -- Richard, any thoughts on how/where this data should be stored? Olli, thanks for the feedback so far! Do you have recommendations for how to break this up between generic and Android components? Do either of the ideas in comment 2 make sense to you?
Flags: needinfo?(rnewman)
Comment 8•10 years ago
|
||
I have only a tenuous grasp of what this is for, so here's my small amount of feedback: * Why not store this in the form history DB? If not there, store it in (encrypted) JSON, and manage the values in-memory. * I suspect that different platforms will have very different modes of interaction between this data and the page -- for example, on most phones it won't be possible to even see all of the fields on-screen. I think exposing an API from the page to access the appropriate form fields, and from storage to access saved values, and then utils as appropriate (which will span two programming languages!), is likely to be least constraining. * Some validation and transformation will need to happen on the Java side (e.g., fixed-width credit card presentation fields, date pickers, etc.). By all means have validation on the JS side, too. * See Bug 939351 for how I really feel. Hope that's of help...
Flags: needinfo?(rnewman)
Comment 9•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #4) > If we're planning to implement this first only on Android, we sure > should have a pref for void requestAutocomplete(); > So you should add > [Pref="your.pref.name"] I don't see a good reason to make this android-only; seems like this patch should live in bug 939351.
Comment 10•10 years ago
|
||
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #9) > (In reply to Olli Pettay [:smaug] from comment #4) > > If we're planning to implement this first only on Android, we sure > > should have a pref for void requestAutocomplete(); > > So you should add > > [Pref="your.pref.name"] > > I don't see a good reason to make this android-only; seems like this patch > should live in bug 939351. I agree that part of part 1 should live in bug 939351. The part that impls the nsIAutofillController as a JS component is Android specific, and should live here.
Comment 11•10 years ago
|
||
Comment on attachment 8342070 [details] [diff] [review] Part 2: WIP requestAutocomplete implementation >diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java > import org.mozilla.gecko.widget.GeckoActionProvider; > import org.mozilla.gecko.widget.ButtonToast; >- > import org.json.JSONArray; Keep 'em separated > import org.json.JSONException; > import org.json.JSONObject; >+ private AutofillHandler mAutofillHandler; Putting this in BrowserApp means Web Apps won't be able to use it. Consider the implications and maybe move to GeckoApp. >+ registerEventListener("Autofill:Prompt"); >+ registerEventListener("Autofill:Response"); Why are these here? Aren't they in AutofillHandler.java? > unregisterEventListener("Settings:Show"); > unregisterEventListener("Updater:Launch"); No unregister ? Not a concern if the registers should be removed anyway. I think you have a good start on the approach. Looks like you could keep moving it forward and see what other issues pop up.
Attachment #8342070 -
Flags: feedback?(mark.finkle) → feedback+
Reporter | ||
Comment 12•10 years ago
|
||
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #9) > (In reply to Olli Pettay [:smaug] from comment #4) > > If we're planning to implement this first only on Android, we sure > > should have a pref for void requestAutocomplete(); > > So you should add > > [Pref="your.pref.name"] > > I don't see a good reason to make this android-only; seems like this patch > should live in bug 939351. Agreed -- I was going to do that after figuring out how the rest of the IDL changes would look (i.e., figuring out comment 2), but I guess I can just move patch 1 to bug 939351 now and do the remainder separately as the implementation gets sorted out. (In reply to Mark Finkle (:mfinkle) from comment #11) > Keep 'em separated Eclipse does that automatically, need to figure out how to turn that off... > > import org.json.JSONException; > > import org.json.JSONObject; > > >+ private AutofillHandler mAutofillHandler; > > Putting this in BrowserApp means Web Apps won't be able to use it. Consider > the implications and maybe move to GeckoApp. Yeah, I didn't think WebApps would be making use of this feature, but I guess it can't hurt to move it just in case. > >+ registerEventListener("Autofill:Prompt"); > >+ registerEventListener("Autofill:Response"); > > Why are these here? Aren't they in AutofillHandler.java? Oops, I used to have these here and left some turds behind.
Comment 13•10 years ago
|
||
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #9) > (In reply to Olli Pettay [:smaug] from comment #4) > > If we're planning to implement this first only on Android, we sure > > should have a pref for void requestAutocomplete(); > > So you should add > > [Pref="your.pref.name"] > > I don't see a good reason to make this android-only; seems like this patch > should live in bug 939351. I said "first". So if we don't have implementation on all the platforms, we should pref it, and once we have implementation everywhere, remove the pref.
Updated•10 years ago
|
relnote-firefox:
? → ---
Reporter | ||
Comment 14•10 years ago
|
||
Here are screenshots of the UI so far. Still working on cleaning up the patches for feedback -- some of them should be ready by tomorrow. Build here: http://people.mozilla.org/~bnicholson/fennec_builds/autocomplete-1.apk requestAutocomplete demo site here: https://googledrive.com/host/0B28BnxIvH5DuWGc3Mm5sZE9DekE/
Reporter | ||
Comment 15•10 years ago
|
||
Reporter | ||
Comment 16•10 years ago
|
||
Reporter | ||
Comment 17•10 years ago
|
||
Reporter | ||
Comment 18•10 years ago
|
||
Updated patch for Android. Still lots of dirty parts (several of them commented), so I'm mostly just looking for higher-level design comments for now. But also happy to hear any other comments if you want to scrutinize it more closely. As I mentioned in bug 939351, I created an etherpad at https://etherpad.mozilla.org/requestautocomplete that I'll start filling in soon.
Attachment #8342063 -
Attachment is obsolete: true
Attachment #8342070 -
Attachment is obsolete: true
Attachment #8401722 -
Flags: feedback?(mark.finkle)
Reporter | ||
Comment 19•10 years ago
|
||
Same story as above.
Attachment #8401723 -
Flags: feedback?(liuche)
Reporter | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 20•10 years ago
|
||
Comment on attachment 8401722 [details] [diff] [review] Part 1: Android requestAutocomplete implementation I need to read up on the requestAutocomplete spec, but I assume it specifies all of the fields you are using in the patch. We aren't making them up. You are adding a handful of Java data classes. Just wondering if we need all of them or not. It's not too bad, but Java tends to breed classes, so I want to be mindful. >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js >+Cu.import("resource://gre/modules/JavaRequest.jsm"); I'd like to think about needing to force importing code. >+let Autofill = { >+ init: function () { >+ JavaRequest.addListener(this, "Autofill:Get"); >+ JavaRequest.addListener(this, "Autofill:Validate"); >+ Services.obs.addObserver(this, "Autofill:Edit", false); >+ }, >+ >+ uninit: function () { >+ JavaRequest.removeListener(this, "Autofill:Get"); >+ JavaRequest.removeListener(this, "Autofill:Validate"); >+ Services.obs.removeObserver(this, "Autofill:Edit"); >+ }, Also wondering if we could move this to an external JS file. Maybe we can't. It looks like you were being thoughtful and only including the minimal amount of code needed to register. We do have a helper for observer service, I wonder if we could make a helper for JavaRequest. Just thinking out loud more than anything here. f+, looks good overall.
Attachment #8401722 -
Flags: feedback?(mark.finkle) → feedback+
Reporter | ||
Updated•10 years ago
|
Attachment #8401723 -
Flags: feedback?(liuche)
Updated•10 years ago
|
Alias: rAc-android
Updated•10 years ago
|
No longer depends on: fx-autofill-backend
Reporter | ||
Comment 21•9 years ago
|
||
This gives us parity with desktop for a minimal rAc dialog. It's not very useful since the user can't actually change the values that get submitted, but this sets up the framework by implementing some essential files: * RequestAutocompleteUI.jsm - Hooks into the existing rAc platform framework and triggers the Java-side rAc handling * AutofillGeckoClient.java - The bridge between Gecko and Java rAc code, responsible for serializing/deserializing data and dispatching autofill events * AutofillDialog.java - Displays the rAc dialog This feature is preffed off by default, so with this patch applied, the "dom.forms.autocomplete.experimental" and "dom.forms.requestAutocomplete" prefs need to be enabled (the build linked below already has them enabled). Build here: https://people.mozilla.org/~bnicholson/fennec_builds/autocomplete-2.apk To test this out, you can go to https://people.mozilla.org/~bnicholson/test/rac-simple.html and click the requestAutocomplete button. For now, the dialog simply shows OK and cancel; if you click OK, hardcoded dummy values will be sent to the page. If you enable these prefs on a desktop build and follow the same steps, you should see the same behavior.
Attachment #8398232 -
Attachment is obsolete: true
Attachment #8398233 -
Attachment is obsolete: true
Attachment #8398235 -
Attachment is obsolete: true
Attachment #8398236 -
Attachment is obsolete: true
Attachment #8401722 -
Attachment is obsolete: true
Attachment #8401723 -
Attachment is obsolete: true
Attachment #8475339 -
Flags: review?(mark.finkle)
Attachment #8475339 -
Flags: review?(liuche)
Reporter | ||
Comment 22•9 years ago
|
||
Forgot to refresh.
Attachment #8475339 -
Attachment is obsolete: true
Attachment #8475339 -
Flags: review?(mark.finkle)
Attachment #8475339 -
Flags: review?(liuche)
Attachment #8475342 -
Flags: review?(mark.finkle)
Attachment #8475342 -
Flags: review?(liuche)
Reporter | ||
Comment 23•9 years ago
|
||
Renaming bug since this is just a base implementation and far from a working rAc. Also marking bug 1043633 as a dependency since this uses the namespaced, promise-based Messaging.jsm.
Depends on: 1043633
Summary: Implement requestAutocomplete for Firefox for Android → Implement base requestAutocomplete UI for Android
Comment 24•9 years ago
|
||
Comment on attachment 8475342 [details] [diff] [review] Implement rAc Android base UI, v2 Review of attachment 8475342 [details] [diff] [review]: ----------------------------------------------------------------- Looks good as a base implementation. I assume that additional non-address sections will be added to AutofillData? It could be nice to add a note in the comments that further fields/sections can be added there. Could you point me to the bug that handles the messages back to Gecko? I don't see a callback in the RequestAutocompleteUI, or what would be calling it with the autofill data. A handful of nits and maybe some naming - all small things. ::: mobile/android/base/autofill/AutofillData.java @@ +8,5 @@ > +/** > + * Data requested by a requestAutocomplete invocation. > + * > + * When a request is handled, the sections, address sections, and fields are populated to match > + * the data being requested. The UI is reponsible for populating the data in each field. When the spelling: responsible @@ +26,5 @@ > + * contactType: "" > + * > + */ > +public class AutofillData implements Iterable<AutofillData.Section> { > + private final ArrayList<Section> sections = new ArrayList<Section>(); Nit: Use List in the type declaration here. @@ +52,5 @@ > + section.addAddressSection(addressSection); > + } > + > + sections.add(section); > + } Will there be handling for individual fields that are not part of an implemented section? @@ +58,5 @@ > + > + /** > + * Data for a section, corresponding to the section-* autocomplete attribute. > + */ > + public static class Section implements Iterable<AddressSection> { How will this change when there are more section types than just AddressSection? ::: mobile/android/base/autofill/AutofillDialog.java @@ +19,5 @@ > + private final Context context; > + private AlertDialog dialog; > + private SubmitListener submitListener; > + > + private static final HashMap<String, String> DUMMY_DB = new HashMap<String, String>(); Nit: Use Map in declaration. @@ +49,5 @@ > + > + private void showDialog(final AutofillData requestData) { > + ThreadUtils.assertOnUiThread(); > + > + // This view is being inflated for a dialog, so no root exists yet. I think it might be more clear to say that this view is being inflated manually to be passed to a Dialog builder, which handles setting the root - up to you. ::: mobile/android/base/autofill/AutofillException.java @@ +1,3 @@ > +package org.mozilla.gecko.autofill; > + > +@SuppressWarnings("serial") I looked at some of our other exceptions, and it looks we just include some serializationUID. I don't think this is an exception that would be serialized or anything, but maybe it's worth doing anyways? ::: mobile/android/base/autofill/AutofillGeckoClient.java @@ +16,5 @@ > +/** > + * Bridge between Gecko and the Java front-end for accessing autofill data. > + */ > +public class AutofillGeckoClient { > + private static AutofillListener autofillListener; Maybe this should be AutofillCallback or AutofillHandler (or AutofillDelegate), because it doesn't do any actual listening, but gets called when the event listener gets a message. @@ +18,5 @@ > + */ > +public class AutofillGeckoClient { > + private static AutofillListener autofillListener; > + > + public interface SubmitListener { Similar, maybe a Handler or Callback? (I don't feel very strongly about this one, though.) @@ +30,5 @@ > + public interface AutofillListener { > + /** > + * Called when the page has fired requestAutocomplete(). > + * > + * The listener *must* respond to the request by calling either Clarify that the *submit* listener must call... because this interface is also called a Listener. @@ +105,5 @@ > + * @param callback Callback from the NativeEventListener used to send a response to Gecko > + */ > + private static void submitAutofill(AutofillData data, EventCallback callback) { > + try { > + JSONArray jsonFields = new JSONArray(); final?
Attachment #8475342 -
Flags: review?(liuche) → review+
Comment 25•9 years ago
|
||
Comment on attachment 8475342 [details] [diff] [review] Implement rAc Android base UI, v2 >diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java Call me crazy, but I don't like using the "dialog" as the main driver. I'd rather use the "Gecko Client" as the driver. I'd be happier never seeing the dialog instantiated unless we really get a callback. Thoughts? >diff --git a/mobile/android/base/resources/layout/autofill_dummy_dialog.xml b/mobile/android/base/resources/layout/autofill_dummy_dialog.xml <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" >+ android:layout_width="fill_parent" >+ android:layout_height="wrap_content" >+ android:orientation="vertical"> >+ >+ <TextView android:layout_width="fill_parent" >+ android:layout_height="wrap_content" >+ android:text="requestAutocomplete demo dialog" /> What's the next step to be able to get a real dialog with some level of user value? Doesn't need to be a full down rAC with credit cards and payments. >diff --git a/mobile/android/modules/RequestAutocompleteUI.jsm b/mobile/android/modules/RequestAutocompleteUI.jsm >+this.RequestAutocompleteUI = function (aAutofillData) { nit: function( >+ show: Task.async(function* () { I hate you
Attachment #8475342 -
Flags: review?(mark.finkle) → feedback+
Comment 26•9 years ago
|
||
Let's try to get back to this bug after sprinting. Product and UX have been asking about "Form Fill Improvements" and this is the heart of it.
Updated•9 years ago
|
tracking-fennec: --- → ?
Updated•9 years ago
|
Priority: -- → P1
Updated•9 years ago
|
tracking-fennec: ? → +
Reporter | ||
Updated•8 years ago
|
Assignee: bnicholson → nobody
Status: ASSIGNED → NEW
Comment 27•7 years ago
|
||
requestAutocomplete was removed from the HTML specification.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
Assignee | ||
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•