Closed
Bug 1401411
Opened 7 years ago
Closed 7 years ago
Autofill is too aggressive at saving profiles compared to Chrome
Categories
(Toolkit :: Form Manager, defect, P3)
Toolkit
Form Manager
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | wontfix |
firefox57 | --- | fixed |
firefox58 | --- | fixed |
People
(Reporter: MattN, Assigned: selee)
References
()
Details
(Whiteboard: [form autofill:MVP])
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
lchang
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details |
e.g. Chrome doesn't seem to save just a first name + last name + organization. They also don't seem to save a (first name, last name, city, state, zip, country code) combo. The main issue is that users are getting too many partial addresses it seems.
Two of my profiles just have an email address (each a different one) + zip code since I needed to enter them as a confirmation on a website. I no longer have access to this website but this case was also below the threshold of 3.
I think we need to do somethings to be less aggressive. e.g. don't count first + last name as two fields for the threshold. Any name fields should count as 1 IMO.
Reporter | ||
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
status-firefox-esr52:
--- → unaffected
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → selee
Status: NEW → ASSIGNED
Comment 1•7 years ago
|
||
After discussed with devs, I would also suggestion first name, middle name and last name count as 1.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.
https://reviewboard.mozilla.org/r/183108/#review188316
::: browser/extensions/formautofill/FormAutofillHandler.jsm:587
(Diff revision 1)
> this._normalizeAddress(data.address);
>
> + let getAddressFieldCount = (record) => {
> + let hasName = 0;
> + let length = 0;
> + for (let key of Object.keys(record)) {
AFAIK, there is no parent or inhertance of `record`, and I don't expect that we would like to look up the key in the prototype chain. This is why `for ... of Object.keys(...)` here rather than `for ... in`.
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.
https://reviewboard.mozilla.org/r/183108/#review189058
::: browser/extensions/formautofill/FormAutofillHandler.jsm:584
(Diff revision 1)
> });
> });
>
> this._normalizeAddress(data.address);
>
> + let getAddressFieldCount = (record) => {
I'm thinking to put this function outside `createRecords` because `createRecords` is big enough and this function may grow over time.
::: browser/extensions/formautofill/FormAutofillHandler.jsm:587
(Diff revision 1)
> this._normalizeAddress(data.address);
>
> + let getAddressFieldCount = (record) => {
> + let hasName = 0;
> + let length = 0;
> + for (let key of Object.keys(record)) {
I'm fine with that.
::: browser/extensions/formautofill/test/unit/test_createRecords.js:34
(Diff revision 1)
> - "family-name": "Doe",
> + "organization": "Mozilla",
> country: "United States",
> },
> expectedRecord: {
> address: {
> "given-name": "John",
> - "family-name": "Doe",
> + organization: "Mozilla",
nit: We should choose one style from `"organization"` or `organization` (w/ or w/o quotes).
Attachment #8911708 -
Flags: review?(lchang)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.
https://reviewboard.mozilla.org/r/183108/#review189058
> I'm thinking to put this function outside `createRecords` because `createRecords` is big enough and this function may grow over time.
The latest patch is with this change and moving the length check in the new function `_isAddressRecordCreatable`. I think the actual purpose of this function should not only count the length but also check it.
`_isCreditCardRecordCreatable` is the same purpose but for credit card records.
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.
https://reviewboard.mozilla.org/r/183108/#review189058
> The latest patch is with this change and moving the length check in the new function `_isAddressRecordCreatable`. I think the actual purpose of this function should not only count the length but also check it.
> `_isCreditCardRecordCreatable` is the same purpose but for credit card records.
hmm, the debug log looks like should change with `_isAddressRecordCreatable` and `_isCreditCardRecordCreatable` functions.
Assignee | ||
Comment 8•7 years ago
|
||
Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.
Remove the flag since I need to add more tests for credit card cases.
Attachment #8911708 -
Flags: review?(lchang)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
Hey Luke, The latest patch is with some credit card cases now. Please review it again. Thanks. :P
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.
https://reviewboard.mozilla.org/r/183108/#review189526
Looks good. Thanks.
Attachment #8911708 -
Flags: review?(lchang) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 12•7 years ago
|
||
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/130a44290c28
All name related fields should be counted as 1 field only when creating the records. r=lchang
Keywords: checkin-needed
Comment 13•7 years ago
|
||
Backed out for eslint failure at browser/extensions/formautofill/test/unit/test_createRecords.js:207:8 | Missing trailing comma:
https://hg.mozilla.org/integration/autoland/rev/27bd931e843d46869717e4160704a49e5c13a74e
Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=130a44290c28b51b919c3f0d01ababad2a594c1d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=133996953&repo=autoland
> TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/extensions/formautofill/test/unit/test_createRecords.js:207:8 | Missing trailing comma. (comma-dangle)
Flags: needinfo?(lchang)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
Hey Sebastian, the latest patch is with eslint fixes, please help to land it again. Thank you.
Flags: needinfo?(selee)
Keywords: checkin-needed
Comment 17•7 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f3faf87ed901
All name related fields should be counted as 1 field only when creating the records. r=lchang
Keywords: checkin-needed
Comment 18•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee | ||
Comment 19•7 years ago
|
||
Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.
Approval Request Comment
[Feature/Bug causing the regression]: New feature.
[User impact if declined]:
Autofill feature is too aggressive at saving the new profile from user's input.
Actually, we should focus on the address related profile.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]:
[Why is the change risky/not risky?]:
This is a minor change since it's about the criteria of saving address profiles.
[String changes made/needed]: No.
Attachment #8911708 -
Flags: approval-mozilla-beta?
Comment 20•7 years ago
|
||
Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.
Improve a new feature, taking it.
Should be in 57b5
Attachment #8911708 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 21•7 years ago
|
||
bugherder uplift |
Flags: in-testsuite+
Comment 22•7 years ago
|
||
(In reply to Sean Lee [:seanlee][:weilonge] from comment #19)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.
Setting qe-verify- based on Sean's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•