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

VERIFIED FIXED in Firefox 56

Status

()

defect
--
major
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: adrian_sv, Assigned: lchang)

Tracking

(Blocks 1 bug)

56 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 disabled, firefox55 disabled, firefox56 verified)

Details

(Whiteboard: [form autofill:MVP])

Attachments

(1 attachment)

[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]
Reporter

Comment 1

2 years ago
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

Updated

2 years ago
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 6

2 years ago
mozreview-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

::: 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 7

2 years ago
mozreview-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/#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+
Assignee

Comment 8

2 years ago
mozreview-review-reply
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!
Comment hidden (mozreview-request)

Comment 10

2 years ago
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

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0400aea3c9ce
Status: ASSIGNED → RESOLVED
Last Resolved: 2 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.