1020602 - Send serialized input elements from form to requestAutocomplete UI component

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Brian Nicholson (:bnicholson)]

When requestAutocomplete is initiated, we need to create a list of all pending autofill elements in that form and send them to the UI component.

The test for this bug should use a mock UI to verify that the correct elements are submitted, so marking bug 1020495 as a blocker.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Send serialized elements to requestAutocomplete UI

Minimal implementation that 1) pulls input/textarea/select elements from the form, and 2) passes the data to the UI in the format described here: https://etherpad.mozilla.org/requestautocomplete.

Comment 2

[:Paolo Amadini]

Comment on attachment 8445593 [details] [diff] [review]
Send serialized elements to requestAutocomplete UI

This looks good, it mainly needs tests, but the next step for now can be the patch for populating the fields, to get a minimal "working" UI in place.

After thinking about the format, we currently have:

  fields[section][addressType][fieldName] = null;

But maybe explicitly naming the collections would be clearer:

  autofillData.sections[section].addressTypes[addressType].fields[fieldName]

This would also allow us to associate other data to the intermediate levels of the structure.

In any case, this isn't a change we necessarily need to do now, we can always fine-tune the format in a follow-up.

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8445593 [details] [diff] [review]
Send serialized elements to requestAutocomplete UI

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

::: toolkit/components/formautofill/FormAutofillContentService.js
@@ +50,5 @@
> +  _getAutofillFields: function (aForm) {
> +    let fields = {};
> +
> +    for (let el of aForm.elements) {
> +      if (!this._isMutableAutofillCandidate(el)) {

I think this check needs to be split into two:
* One that returns from the whole method to indicate that there is a field we don't support yet (e.g. <select> or <textarea>) but should autofill so we can dispatch "disabled".
* Another that excludes elements we don't need to support e.g. radio/checkbox. Once we change el.getAttribute("autocomplete") below to use the API from bug 1020496 we will get that part for free since they won't have the API.

@@ +105,5 @@
> +    };
> +  },
> +
> +  _isMutableAutofillCandidate: function (aEl) {
> +    // TODO: Check whether elements are readonly/disabled/etc.

Some fields which are not mutable should still be sent to the UI so this function should change names and I think we should eventually just include properties from the element so the UI knows not to fill them rather than skip them. e.g. input type=hidden in https://www.w3.org/Bugs/Public/show_bug.cgi?id=25471

@@ +106,5 @@
> +  },
> +
> +  _isMutableAutofillCandidate: function (aEl) {
> +    // TODO: Check whether elements are readonly/disabled/etc.
> +    return (aEl.nodeName === "INPUT" || aEl.nodeName === "TEXTAREA" || aEl.nodeName === "SELECT");

Since the <select> patch bounced in bug 1020697 and <textarea> hasn't been implemented yet, we shouldn't include them here yet so we can dispatch "disabled" if we encountered them. See above.

You can use input.mozIsTextField(true) to filter out radio/checkbox but it also filters out type=hidden. Exposing nsIFormControl::IsTextControl would be better since that includes textarea but hidden would still need to be special-cased. I just realized that number is missing too and possibly others so maybe we should have a separate helper perhaps on nsIFormControl (eventually) like isAutocompletableControl.

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Send serialized elements to requestAutocomplete UI, v2

Removed the _isMutableAutofillCandidate() helper since it was incomplete and ambiguous, and referenced bug numbers in the TODO comments.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8446087 [details] [diff] [review]
Send serialized elements to requestAutocomplete UI, v2

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

::: toolkit/components/formautofill/FormAutofillContentService.js
@@ +52,5 @@
> +
> +    for (let el of aForm.elements) {
> +      // TODO: Use @autocomplete attributes directly from the element (bug 1030295).
> +      // TODO: Filter unsupported <input> types (bug 1030301).
> +      let attr = el.getAttribute("autocomplete");

Sorry, I overlooked this: Make this el.autocomplete so you get the validation checks and santization from the DOM code.

@@ +88,5 @@
> +    let match;
> +    let token;
> +
> +    while (match = regex.exec(attr)) {
> +      token = match[0];

You don't need this regex/normalization stuff when you use the normalized IDL attribute:

let tokens = aAttr.split(" ");
…
fieldName: tokens.length ? tokens[tokens.length - 1] : ""

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: Send serialized elements to requestAutocomplete UI, v3

(In reply to Matthew N. [:MattN] from comment #5)
> Sorry, I overlooked this: Make this el.autocomplete so you get the
> validation checks and santization from the DOM code.

Oops, forgot that those @autocomplete changes landed. Patch updated.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8446119 [details] [diff] [review]
Send serialized elements to requestAutocomplete UI, v3

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

Thanks Brian.

::: toolkit/components/formautofill/FormAutofillContentService.js
@@ +75,5 @@
> +        fields[section][addressType] = {};
> +      }
> +
> +      // TODO: Add a property to indicate that fields are immutable (bug 1020701).
> +      fields[section][addressType][fieldName] = null;

Perhaps we should already add a TODO with a new bug to handle arrays of contact fields (e.g. "tel") instead of overwriting when there is more than one fieldName with different contactTypes.

@@ +93,5 @@
> +
> +    return {
> +      section: "",
> +      addressType: "",
> +      fieldName: fieldName

You're going to want to include contactType here eventually as it will be needed as a property on fields[section][addressType][fieldName] eventually.

Comment 8

[:Paolo Amadini]

Attachment: Send serialized elements to requestAutocomplete UI, v3, rebased

Rebased on top of bug 1020865.

Comment 9

[:Paolo Amadini]

I'm currently thinking about the processing model for the autocomplete fields, that is how the set of form fields and their autocomplete tokens should be grouped and presented in the user interface.

In doing this, I'll revisit the serializable format, in general using a model that is more similar to what I suggested in comment 2, but with levels that are closer to what the user interface needs, which I'm still trying to determine. I think that most of the organization of the fields and structure checking are independent from the user interface, and may be done before it is invoked.

Comment 10

[:Paolo Amadini]

Attachment: Updated format

For the moment, I just updated the format to use arrays, and added the round-trip field identifier. I kept the custom section level, though it might turn out not be so useful.

The "hint set" and "scope" mentioned in the specification don't seem to apply well to the requestAutocomplete UI, they seem to be designed primarily for the Form Autofill feature. There is no useful grouping we could make from that information at present.

Comment 11

[:Paolo Amadini]

Attachment: Adapted patch

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8450319 [details] [diff] [review]
Adapted patch

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

::: toolkit/components/formautofill/FormAutofillContentService.js
@@ +141,5 @@
> +    let tokens = autocomplete.split(" ");
> +    return {
> +      customSectionName: tokens.find(t => t.startsWith("section-")) || "",
> +      addressType: tokens.find(t => t == "billing" || t == "shipping") || "",
> +      contactType: tokens.find(t => t == "home" || t == "work") || "",

IIUC, this is wrong since the tokens have to be in a specific order. We can just wait for bug 1020496 at this point.

::: toolkit/components/formautofill/FormAutofillIntegration.jsm
@@ +44,5 @@
> +   *            name: User-specified section name, or empty string.
> +   *            addressSections: [{
> +   *              addressType: "shipping", "billing", or empty string.
> +   *              fields: [{
> +   *                localId: Number used to associate request and response.

I didn't think we'd need an internal ID. Can you explain why we do?

Comment 13

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #12)
> IIUC, this is wrong since the tokens have to be in a specific order. We can
> just wait for bug 1020496 at this point.

Well, when extracting the order won't matter because the keywords don't overlap. This is just temporary code, however, and I'm fine with waiting on bug 1020496 if we can land it soon. We should, however, consider keeping this stub in case bug 1020496 is delayed. There's bug 1030295 on file to remove it.

> ::: toolkit/components/formautofill/FormAutofillIntegration.jsm
> @@ +44,5 @@
> > +   *            name: User-specified section name, or empty string.
> > +   *            addressSections: [{
> > +   *              addressType: "shipping", "billing", or empty string.
> > +   *              fields: [{
> > +   *                localId: Number used to associate request and response.
> 
> I didn't think we'd need an internal ID. Can you explain why we do?

This is used to associate the field in the returned data structure in the other direction (see the patch in bug 1020607).

While an association can, in theory, be done by considering the order of the items in the data trees (second email field of "billing" address type of first section), it just means the ID still exists but is, in fact, the position in arrays. This forces the returned data structures to use the same trees or similar positional reference as a form of ID, when there would be no need for that at all. This means less flexibility in changing the data structures later.

We can't use object identity or direct element references like we do in other places because the data is transmitted cross-process. For example, Downloads.jsm uses object identity but the B2G wrapper API has still to generate locally valid synthetic identifiers for cross-process communication.

Comment 14

[:Paolo Amadini]

Attachment: Rebased patch

Matt, this patch is rebased to use the getAutocompleteInfo method. It works! :-)

To rationalize the patch queue and clear the space for landing, I've taken this bug and filed bug 1036481 for the associated test cases you were working on.

Comment 15

[Brian Nicholson (:bnicholson)]

Comment on attachment 8453146 [details] [diff] [review]
Rebased patch

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

::: toolkit/components/formautofill/FormAutofillContentService.js
@@ +66,5 @@
> +                ? new this.window.Event("autocomplete", { bubbles: true })
> +                : new this.window.AutocompleteErrorEvent("autocompleteerror",
> +                                                         { bubbles: true,
> +                                                           reason: reason });
> +    yield this.waitForTick();

Interesting how you did this, but I wonder if we could make it even simpler. Promises are already async, so couldn't you just do something like 'yield Promise.resolve();' instead?

Honestly, I don't really see much value in making this event dispatch async since several of the prior steps are async already, but I guess that's what the spec says to do.

@@ +82,5 @@
> +
> +    // We will assign a local ID to each individual field.  This is required
> +    // because we serialize the data, potentially send it to another process,
> +    // and receive a response later.
> +    let localId = 1;

We should be able to get of these IDs altogether without relying on order (as discussed on IRC).

@@ +86,5 @@
> +    let localId = 1;
> +
> +    for (let element of this.form.elements) {
> +      // Query the interface and exclude elements that cannot be autocompleted.
> +      if (!(element instanceof Ci.nsIDOMHTMLInputElement)) {

I wonder if we should just omit this check until we implement bug 1030301? I guess we can keep this and change it later, but I thought one of our goals was to implement only the bare minimum for each bug to keep patches smaller and more testable.

@@ +90,5 @@
> +      if (!(element instanceof Ci.nsIDOMHTMLInputElement)) {
> +        continue;
> +      }
> +
> +      // Exclude elements to whick no autocomplete field has been assigned.

nit: s/whick/which/

@@ +103,5 @@
> +      if (!section) {
> +        section = {
> +          name: info.customSectionName,
> +          addressSections: [],
> +        };

It seems more convoluted to me to do:

data.sections[section].addressSections[addressSection].fields[field]

instead of just:

data[section][addressSection][field]

The former format does allow us to add intermediate data as you mentioned in comment 2, but is there anything in particular you're anticipating? You said in that comment that we could switch to this format in a follow-up, if necessary, so I'm curious why you wanted to make that change now.

Comment 16

[:Paolo Amadini]

(In reply to Brian Nicholson (:bnicholson) from comment #15)
> Promises are already async, so couldn't you just do something like 'yield
> Promise.resolve();' instead?

The promise resolution loop handles all the currently resolved promises in a single event loop tick. This means that, in promises, the "then" function always returns before its callbacks are invoked, but there is no guarantee that the callbacks will be executed in the next tick.

> Honestly, I don't really see much value in making this event dispatch async
> since several of the prior steps are async already, but I guess that's what
> the spec says to do.

I figured out we needed to wait an entire tick because of the spec, but if the requirement is only to terminate the currently executing JavaScript code before firing the event, then Tasks will do that for us.

> @@ +82,5 @@
> > +
> > +    // We will assign a local ID to each individual field.  This is required
> > +    // because we serialize the data, potentially send it to another process,
> > +    // and receive a response later.
> > +    let localId = 1;
> 
> We should be able to get of these IDs altogether without relying on order
> (as discussed on IRC).

A summary of it is that we can use a business key initially made of { section, addressType, contactType, fieldName } and not support different values for fields with the same autocomplete attribute - we should return "disabled" if this is the case (because then we don't know which field to use for metadata like validation).

There's not an outstanding advantage of one solution over the other, so we can reintroduce IDs if we see it is necessary or simplifies the code later.

> @@ +86,5 @@
> > +    let localId = 1;
> > +
> > +    for (let element of this.form.elements) {
> > +      // Query the interface and exclude elements that cannot be autocompleted.
> > +      if (!(element instanceof Ci.nsIDOMHTMLInputElement)) {
> 
> I wonder if we should just omit this check until we implement bug 1030301? I
> guess we can keep this and change it later, but I thought one of our goals
> was to implement only the bare minimum for each bug to keep patches smaller
> and more testable.

I'm not sure what you're asking. We need to QueryInterface to nsIDOMHTMLInputElement in order to access getAutocompleteInfo. There's no trace of bug 1030301 here.

> It seems more convoluted to me to do:
> 
> data.sections[section].addressSections[addressSection].fields[field]
> 
> instead of just:
> 
> data[section][addressSection][field]
> 
> The former format does allow us to add intermediate data as you mentioned in
> comment 2, but is there anything in particular you're anticipating? You said
> in that comment that we could switch to this format in a follow-up, if
> necessary, so I'm curious why you wanted to make that change now.

I believe this format makes the code easier to read and understand. I did the change since I was already writing the tests for this and tests use the format internally.

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8453146 [details] [diff] [review]
Rebased patch

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

I don't have anything to add on top of what Brian already said so I guess just make those changes and it'll likely be a straightforward r+ but I'd like to see it to make sure I understand the changes correctly.

Comment 18

[:Paolo Amadini]

Attachment: Without local identifier

In this patch, the only change is the removal of the ID.

Comment 19

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/4b08f0ea3bb6

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4b08f0ea3bb6