1300993 - Connect satchel autocomplete to form autofill results instead of form history when appropriate

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P3)

Description

[Matthew N. [:MattN]]

If there are greater than N fields in a form and perhaps only if there are saved profiles, form autofill results should be shown instead of form history ones.

Bug 239380 / bug 380963 provide some options. An alternative is to pile on to how Password Manager works and add a method similar to markAsLoginManagerField but for form autofill.

Comment 1

[Steve Chung [:steveck]]

Hi Matt,
It seems like you would prefer to implement MarkAsAutofillField in bug 1304634. Is that means we'll also need to load corresponding module in nsContextMenu.js(or somewhere else?) for handling the MarkAsAutofillField? Not sure if it's appropriate to implement this as the first step.

Comment 2

[Steve Chung [:steveck]]

Unassigned myself since it's would be more clear after bug 1304634 landed.

Comment 3

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1300993 - Connect satchel autocomplete to form autofill results instead of form history when appropriate.

Review commit: https://reviewboard.mozilla.org/r/96238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96238/

Comment 4

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8815208 [details]
Bug 1300993 - Connect satchel autocomplete to form autofill results instead of form history when appropriate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96238/diff/1-2/

Comment 5

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8815208 [details]
Bug 1300993 - Connect satchel autocomplete to form autofill results instead of form history when appropriate.

Hi Matt,

Since Steve and I found that "StartSearch" should be placed in content process, it might be easier to land bug 1304634 if we have the skeleton in this bug first.

Therefore, I added the schema of "markAsAutofillField" here. I also added an API, getFocusedInput, in FromFillController so that we can get the focused input while processing "StartSearch" in the following bugs.

Not sure if I'm in the right path. Your feedback will be very helpful. Thanks.

Comment 6

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8815208 [details]
Bug 1300993 - Connect satchel autocomplete to form autofill results instead of form history when appropriate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96238/diff/2-3/

Comment 7

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8815208 [details]
Bug 1300993 - Connect satchel autocomplete to form autofill results instead of form history when appropriate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96238/diff/3-4/

Comment 8

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8815208 [details]
Bug 1300993 - Connect satchel autocomplete to form autofill results instead of form history when appropriate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96238/diff/4-5/

Comment 9

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8815208 [details]
Bug 1300993 - Connect satchel autocomplete to form autofill results instead of form history when appropriate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96238/diff/5-6/

Comment 10

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8815208 [details]
Bug 1300993 - Connect satchel autocomplete to form autofill results instead of form history when appropriate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96238/diff/6-7/

Comment 11

[Luke Chang [:lchang] (inactive)]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #0)
> If there are greater than N fields in a form and perhaps only if there are
> saved profiles, form autofill results should be shown instead of form
> history ones.

Hi Matt,

I set N to 3 in this patch but am not sure if it's reasonable. The "markAsAutofillField" is still called even there's no saved profile because I think a field should be always marked and profiles can be taken into account later during searching. What do you think?

Comment 12

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8815208 [details]
Bug 1300993 - Connect satchel autocomplete to form autofill results instead of form history when appropriate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96238/diff/7-8/

Comment 13

[Matthew N. [:MattN]]

(In reply to Luke Chang [:lchang] from comment #11)
> (In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from
> comment #0)
> > If there are greater than N fields in a form and perhaps only if there are
> > saved profiles, form autofill results should be shown instead of form
> > history ones.
> 
> Hi Matt,
> 
> I set N to 3 in this patch but am not sure if it's reasonable. The
> "markAsAutofillField" is still called even there's no saved profile because
> I think a field should be always marked and profiles can be taken into
> account later during searching. What do you think?

IIUC, the problem with that approach is that it means that then autofill code will then need to know how to query form history in the cases where there are no saved profiles.

Comment 14

[Luke Chang [:lchang] (inactive)]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #13)
> IIUC, the problem with that approach is that it means that then autofill
> code will then need to know how to query form history in the cases where
> there are no saved profiles.

As offline discussion, we'll deal with it in the follow-up bug of how to fallback to form-history if there's no profile saved.

Comment 15

[Luke Chang [:lchang] (inactive)]

(In reply to Luke Chang [:lchang] from comment #14)
> As offline discussion, we'll deal with it in the follow-up bug of how to
> fallback to form-history if there's no profile saved.

Bug was filed.

Bug 1325724 - Fallback to form history if there is no form autofill profile saved.

Comment 16

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8815208 [details]
Bug 1300993 - Connect satchel autocomplete to form autofill results instead of form history when appropriate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96238/diff/8-9/

Comment 17

[Steve Chung [:steveck]]

Hi Luke,
Since I disabled the ensureRigister in bug 1304634, you might need to add this in content initialization. BTW you'll need to remove the loadFormAutofillContent while adding task in xpcshell head.js(it seems a unnecessary call and it'll lead the duplicated factory registration).

Comment 18

[Luke Chang [:lchang] (inactive)]

Steve, Thanks. It works now.

Comment 19

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8815208 [details]
Bug 1300993 - Connect satchel autocomplete to form autofill results instead of form history when appropriate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96238/diff/9-10/

Comment 20

[Steve Chung [:steveck]]

Attachment: Bug 1300993 - Connect satchel autocomplete to form autofill results instead of form history when appropriate.

Review commit: https://reviewboard.mozilla.org/r/101298/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101298/

Comment 21

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 1300993 - Add ProfileAutoCompleteResult.

Review commit: https://reviewboard.mozilla.org/r/102024/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102024/

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8815208 [details]
Bug 1300993 - Connect satchel autocomplete to form autofill results instead of form history when appropriate.

https://reviewboard.mozilla.org/r/96238/#review101448

r=me with the duplicate form issue resolved.

::: browser/extensions/formautofill/content/FormAutofillContent.js:178
(Diff revision 8)
>  };
> +
> +/**
> + * Handles content's interactions.
> + */
> +var FormAutofillContent = {

Nit: does `let` work instead?

::: browser/extensions/formautofill/content/FormAutofillContent.js:307
(Diff revision 10)
> +        if (!forms.some(form => form.rootElement === formLike.rootElement)) {
> +          forms.push(formLike);
> +        }

Maybe add a comment about what this is doing so other people understand?

::: browser/extensions/formautofill/content/FormAutofillContent.js:313
(Diff revision 10)
> +    for (let form of doc.forms) {
> +      let formLike = FormLikeFactory.createFromForm(form);
> +      forms.push(formLike);
> +    }

Right now this is redundant as we would have found any relevant forms with <input> using the previous loop. We should either remove this or use forms.some here too I think.

Eventually when we support more than just <input> the `getElementsByTagName("input")` can change to something like `querySelectorAll("input, select, textarea")`.

::: browser/extensions/formautofill/test/unit/head.js:42
(Diff revision 10)
>  // with a file that is still pending deletion highly unlikely.
>  let gFileCounter = Math.floor(Math.random() * 1000000);
>  
>  function loadFormAutofillContent() {
> -  let facGlobal = {};
> +  let facGlobal = {
> +    addEventListener: function() {},

I guess this is fine for now.

Comment 23

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8815208 [details]
Bug 1300993 - Connect satchel autocomplete to form autofill results instead of form history when appropriate.

https://reviewboard.mozilla.org/r/96238/#review101448

I'll address it. Thanks for the review.

> Nit: does `let` work instead?

Seems that using `let` causes the object not to be exposed via `loadSubScriptWithOptions`. I'll add some comments here.

Comment 24

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8815208 [details]
Bug 1300993 - Connect satchel autocomplete to form autofill results instead of form history when appropriate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96238/diff/10-11/

Comment 25

[Luke Chang [:lchang] (inactive)]

Tests are passed.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/824ca0e106e0
Connect satchel autocomplete to form autofill results instead of form history when appropriate. r=MattN

Comment 27

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/824ca0e106e0