Closed
Bug 1378668
Opened 7 years ago
Closed 7 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)
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.
Updated•7 years ago
|
Whiteboard: [form autofill:MVP]
Reporter | ||
Comment 1•7 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
status-firefox54:
--- → disabled
status-firefox55:
--- → disabled
status-firefox56:
--- → affected
status-firefox-esr52:
--- → unaffected
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → lchang
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
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
Assignee | ||
Comment 3•7 years ago
|
||
Also, I think we should update the saved fields as well when a profile is merged or reconciled.
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
(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•7 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•7 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•7 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•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0400aea3c9ce
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Reporter | ||
Comment 12•7 years ago
|
||
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.
Description
•