Closed Bug 1300993 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Form Manager, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla53
Iteration:
53.4 - Jan 9
Tracking Status
firefox53 --- fixed

People

(Reporter: MattN, Assigned: lchang)

References

Details

(Whiteboard: [form autofill:M1])

Attachments

(1 file, 2 obsolete files)

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.
Whiteboard: [form autofill:M1] → [form autofill:M1][form autofill]
Whiteboard: [form autofill:M1][form autofill] → [form autofill:M1]
Assignee: nobody → schung
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.
Flags: needinfo?(MattN+bmo)
Unassigned myself since it's would be more clear after bug 1304634 landed.
Assignee: schung → nobody
Flags: needinfo?(MattN+bmo)
Assignee: nobody → lchang
Status: NEW → ASSIGNED
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.
Attachment #8815208 - Flags: feedback?(MattN+bmo)
Attachment #8815208 - Flags: feedback?(MattN+bmo)
Target Milestone: --- → mozilla54
Iteration: --- → 53.4 - Jan 9
Blocks: 1325538
(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?
(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.
(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.
Blocks: 1325724
(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.
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).
Steve, Thanks. It works now.
Attachment #8822354 - Attachment is obsolete: true
Attachment #8822354 - Flags: review?(MattN+bmo)
Attachment #8823496 - Attachment is obsolete: true
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.
Attachment #8815208 - Flags: review?(MattN+bmo) → review+
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.
Tests are passed.
Keywords: checkin-needed
Target Milestone: mozilla54 → ---
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/824ca0e106e0
Connect satchel autocomplete to form autofill results instead of form history when appropriate. r=MattN
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/824ca0e106e0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1330567
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.