Closed Bug 1378668 Opened 3 years ago Closed 3 years ago

[Form Autofill] - On sequentially filled in autofill addresses, not all fields work as autofill triggers (new profile/first address)

Categories

(Toolkit :: Form Manager, defect)

56 Branch
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- disabled
firefox55 --- disabled
firefox56 --- verified

People

(Reporter: aflorinescu, Assigned: lchang)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:MVP])

Attachments

(1 file)

[Environment:]
Windows 10x64, Ubuntu 16.04, Mac Osx 10.12

Nightly 56.0a1 20170705100248

[Description:]
First creating a new autofill profile, and filling the preferences fields sequentially will only allow the first saved fields to trigger autofill.

[Preconditions:]
Create a new profile, filling only the following fields: Name, Company.

[Steps:]
1. Open Firefox.
2. Navigate to https://luke-chang.github.io/autofill-demo/basic.html
3. Double click on Name andon Organization.
4. Double click on all other fields.
5. Open Preferences/Security/Saved profiles.
6. Fill in all the other fields.
7. Return to the form or open any other form.
8. Double click on Name and Organization.
9. Double click on all other fields.

[Actual Result:]
Steps 3 - Name and Organization double click trigger the form autofill.
Steps 4 - All other fields do not trigger autofill as expected - no values saved in the profile.
Step 8 - Name and Organization double click trigger the form autofill.
Step 9 - All other fields do not trigger autofill.

[Expected Result:]
Step 9 - All fields trigger autofill.

[Note:]
1. After browser restart the autofill trigger works for all fields - as far as I can tell, this scenario only works on a new profile or on a profile that first creates/saves a new profile.
2. Also, creating a secondary profile will allow all fields to trigger autofill.
3. Setting severity to minor since it's an edge case, although I think the cause should be investigated in order to make sure there isn't another scenario more widely spread that is impacted by this bug.
Whiteboard: [form autofill:MVP]
Raising the severity for this bug since there are quite a few variations of this bug which users will encounter at the start when we launch the feature and it's a given we're going to hit this a bit, since our users won't have saved address profiles.
Adding to that the fact that most of our users will have the form history, the scenarios become quite complex: I've noticed issues with profile updates (incomplete 1st profile), issues where form history triggers instead of form autofill(incomplete 1st profile), but I'm speculating that fixing this issue will automatically fix those scenarios. I'm basing this assumption on the fact that after browser restart or after a complete profile is saved, the issues are not reproducible anymore.
Severity: minor → major
Assignee: nobody → lchang
Status: NEW → ASSIGNED
It's caused because we currently update the saved fields only upon adding and removing a profile. Not sure why we explicitly excluded the updating scenario in bug 1325724 [1]. Ni Steve as he may know the detail.

[1] https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/browser/extensions/formautofill/FormAutofillParent.jsm#120-123
Flags: needinfo?(schung)
See Also: → 1325724
Also, I think we should update the saved fields as well when a profile is merged or reconciled.
(In reply to Luke Chang [:lchang] from comment #2)
> It's caused because we currently update the saved fields only upon adding
> and removing a profile. Not sure why we explicitly excluded the updating
> scenario in bug 1325724 [1]. Ni Steve as he may know the detail.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 7d2e89fb92331d7e4296391213c1e63db628e046/browser/extensions/formautofill/
> FormAutofillParent.jsm#120-123

I might be confused by updating "record" or "field" at that time... You're right that we should update filed name for all the events except "notifyUsed" since it only update record's  metadata.
Flags: needinfo?(schung)
Comment on attachment 8890312 [details]
Bug 1378668 - [Form Autofill] Update the saved field names when a profile is updated, merged or reconciled.

https://reviewboard.mozilla.org/r/161440/#review167206

::: browser/extensions/formautofill/test/unit/test_activeStatus.js:56
(Diff revision 1)
>    formAutofillParent._computeStatus.returns(!formAutofillParent._active);
>    formAutofillParent._onStatusChanged.reset();
>    formAutofillParent.observe(null, "formautofill-storage-changed", "add");
>    do_check_eq(formAutofillParent._onStatusChanged.called, true);
>  
> +  // profile updated => Need to trigger _onStatusChanged

nit: Since we'll need to verify 6 events and 5 of them should have the same verification process, maybe a loop like:
```
["add", "remove", ...].forEach(event => {
  ...
do_check_eq(formAutofillParent._onStatusChanged.called, true, "Need to trigger _onStatusChanged because of profile " + event);
});
```

::: browser/extensions/formautofill/test/unit/test_savedFieldNames.js:31
(Diff revision 1)
>  
>    // profile added => Need to trigger updateValidFields
>    formAutofillParent.observe(null, "formautofill-storage-changed", "add");
>    do_check_eq(formAutofillParent._updateSavedFieldNames.called, true);
>  
> +  // profile updated => Need to trigger updateValidFields

nit: Same here.
Comment on attachment 8890312 [details]
Bug 1378668 - [Form Autofill] Update the saved field names when a profile is updated, merged or reconciled.

https://reviewboard.mozilla.org/r/161440/#review167210

Now I remeber it's because we only handle storage is empty or not for the enabling status in the past, but right now we change the status by checking the number of saved fileds(so we don't have to get storage again). The patch looks fine and I only have few nits about the test, thanks!
Attachment #8890312 - Flags: review?(schung) → review+
Comment on attachment 8890312 [details]
Bug 1378668 - [Form Autofill] Update the saved field names when a profile is updated, merged or reconciled.

https://reviewboard.mozilla.org/r/161440/#review167206

> nit: Since we'll need to verify 6 events and 5 of them should have the same verification process, maybe a loop like:
> ```
> ["add", "remove", ...].forEach(event => {
>   ...
> do_check_eq(formAutofillParent._onStatusChanged.called, true, "Need to trigger _onStatusChanged because of profile " + event);
> });
> ```

Good point!
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0400aea3c9ce
[Form Autofill] Update the saved field names when a profile is updated, merged or reconciled. r=steveck
https://hg.mozilla.org/mozilla-central/rev/0400aea3c9ce
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Verified as fixed on 56.0a1 20170728100358 Windows 10x64/ Mac OSX 10.12
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.