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)

56 Branch
defect
Not set
normal

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: nobody → lchang
Status: NEW → ASSIGNED
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 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 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)
Err.. Sorry I cleared the review accidentally. Overall looks fine for me BTW.
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 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 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 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 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+
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
https://hg.mozilla.org/mozilla-central/rev/c7ca671926a6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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.