Closed
Bug 1385216
Opened 7 years ago
Closed 7 years ago
[Form Autofill] Address lines are detected as a profile update and trigger profile update
Categories
(Toolkit :: Form Manager, defect)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox54 | --- | disabled |
firefox55 | --- | disabled |
firefox56 | --- | verified |
firefox57 | --- | fixed |
People
(Reporter: aflorinescu, Assigned: lchang)
References
Details
(Whiteboard: [form autofill])
Attachments
(1 file)
[Description:] Somehow related to bug 1379138, the scenario here varies a bit by just autofilling a form with a multi-line address, afterwhich the autofill engine shall detect it as a change and offer to update/create new profile. [Steps:] 1. Create a multi address line profile. 2. Fill in a form (e.g. https://luke-chang.github.io/autofill-demo/basic.html) using the multi line saved profile. 3. Press Submit. 4. Chose any of Update profile [Actual Result:] The profile used for the form is updated by removing the multi line address and saving the address in one line. [Expected Result:] The autofill engine shouldn't identify the filled in profile as different from the existent profile in storage and therefore not trigger any profile update.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → lchang
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8891894 [details] Bug 1385216 - [Form Autofill] Avoid triggering update on fields that aren't changed after autofilling or contain concatenated street address. https://reviewboard.mozilla.org/r/162912/#review169156 ::: browser/extensions/formautofill/FormAutofillHandler.jsm:391 (Diff revision 1) > + * Contains two properties: > + * - address: The new address that convert from details with > + * trimmed result. > + * - untouchedFields: Fields that aren't touched after autofilling. > */ > createProfile() { You'll need to rebase since createProfile support both address and credit card now(sorry about that). Instead of returning the records and untouchedFields separately, maybe we can return address/cc object like following structure: address: { guid: record: untouchedFields: }, // or null if invalid creditCard: { // Same here }
Attachment #8891894 -
Flags: review?(schung)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8891894 [details] Bug 1385216 - [Form Autofill] Avoid triggering update on fields that aren't changed after autofilling or contain concatenated street address. https://reviewboard.mozilla.org/r/162912/#review169156 > You'll need to rebase since createProfile support both address and credit card now(sorry about that). > Instead of returning the records and untouchedFields separately, maybe we can return address/cc object like following structure: > address: { > guid: > record: > untouchedFields: > }, // or null if invalid > creditCard: { > // Same here > } Rebased. Thanks.
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8891894 [details] Bug 1385216 - [Form Autofill] Avoid triggering update on fields that aren't changed after autofilling or contain concatenated street address. https://reviewboard.mozilla.org/r/162912/#review170240 ::: browser/extensions/formautofill/FormAutofillParent.jsm:290 (Diff revision 2) > _onFormSubmit(data, target) { > let {address} = data; > > if (address.guid) { > + // Avoid updating the fields that users don't modify. > + if (address.untouchedFields) { It seems unnecessary because `untouchedFields` is empty array by default. You can either remove this checking or `address.untouchedFields.length > 0` if you want to save more time.
Attachment #8891894 -
Flags: review?(schung)
Comment 6•7 years ago
|
||
Err.. Sorry I cleared the review accidentally. Overall looks fine for me BTW.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8891894 [details] Bug 1385216 - [Form Autofill] Avoid triggering update on fields that aren't changed after autofilling or contain concatenated street address. https://reviewboard.mozilla.org/r/162912/#review170240 > It seems unnecessary because `untouchedFields` is empty array by default. You can either remove this checking or `address.untouchedFields.length > 0` if you want to save more time. Good catch. After looking into it, I noticed that `untouchedFields.length == 0` would never happens because `guid` wouldn't be set if all fields are changed. I'll simply remove the check. Thanks.
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8891894 [details] Bug 1385216 - [Form Autofill] Avoid triggering update on fields that aren't changed after autofilling or contain concatenated street address. https://reviewboard.mozilla.org/r/162912/#review170636 ::: browser/extensions/formautofill/ProfileStorage.jsm:1386 (Diff revision 3) > + // match each other. > + if (field == "street-address" && > + FormAutofillUtils.toOneLineAddress(addressFound[field]) == FormAutofillUtils.toOneLineAddress(addressToMerge[field])) { > + // Intend to keep the multi-line version or the existing one if both > + // are the single-line version. > + if (addressFound[field].includes("\n") || !addressToMerge[field].includes("\n")) { I'm not sure why we need to check if both are single line here, because it should not enter this block(addressToMerge[field] should equal to addressFound[field] at the very begining). Is there any case that need to reset the merge target with both sigle line addresses?
Attachment #8891894 -
Flags: review?(schung)
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8891894 [details] Bug 1385216 - [Form Autofill] Avoid triggering update on fields that aren't changed after autofilling or contain concatenated street address. https://reviewboard.mozilla.org/r/162912/#review170638 Looks good to me except for the minor concern in profile storage, thanks.
Attachment #8891894 -
Flags: review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8891894 [details] Bug 1385216 - [Form Autofill] Avoid triggering update on fields that aren't changed after autofilling or contain concatenated street address. https://reviewboard.mozilla.org/r/162912/#review170636 > I'm not sure why we need to check if both are single line here, because it should not enter this block(addressToMerge[field] should equal to addressFound[field] at the very begining). Is there any case that need to reset the merge target with both sigle line addresses? You're right. Thanks for pointing it out.
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8891894 [details] Bug 1385216 - [Form Autofill] Avoid triggering update on fields that aren't changed after autofilling or contain concatenated street address. https://reviewboard.mozilla.org/r/162912/#review171568 LGTM! Thank you.
Attachment #8891894 -
Flags: review?(selee) → review+
Comment 15•7 years ago
|
||
Pushed by lchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c7ca671926a6 [Form Autofill] Avoid triggering update on fields that aren't changed after autofilling or contain concatenated street address. r=seanlee,steveck
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c7ca671926a6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 17•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/8cc5993a9104
Flags: in-testsuite+
Comment 18•7 years ago
|
||
Verified using latest 56 beta 6 that this is fixed across platforms (Windows 10 64bit, macOS 10.12.6 and Ubuntu 16.04 64bit).
You need to log in
before you can comment on or make changes to this bug.
Description
•