[Form Autofill] Support "address-line*" fields

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Form Manager
P3
normal
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: lchang, Assigned: lchang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [form autofill:M2])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

8 months ago
Create "address-line1", "address-line2" and "address-line3" fields based on "street-address" when loading data from the storage and vice versa.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8860795 [details]
Bug 1358943 - [Form Autofill] Support "address-line*" fields.

https://reviewboard.mozilla.org/r/132756/#review137018

::: browser/extensions/formautofill/ProfileStorage.jsm:27
(Diff revision 2)
> + *       // extended fields
> + *       address-line1,
> + *       address-line2,
> + *       address-line3,

The comment at the top says that this is describing what's stored on disk but these won't be so I guess either remove these or change the comment.

::: browser/extensions/formautofill/ProfileStorage.jsm:27
(Diff revision 2)
>   *       postal-code,
>   *       country,          // ISO 3166
>   *       tel,
>   *       email,
>   *
> + *       // extended fields

Nit: I think "computed"/"calculated" fields is more clear. You may also want to note they're not saved.

::: browser/extensions/formautofill/ProfileStorage.jsm:299
(Diff revision 2)
> -      if (!searchString) {
> -        return !!name;
> -      }
> +    }
> +  },
>  
> -      return name.toLowerCase().startsWith(lcSearchString);
> +  _shrinkFields(profile) {

How about calling this `_normalizeAddress` first until we have other things to normalize.

::: browser/extensions/formautofill/ProfileStorage.jsm:311
(Diff revision 2)
> +        profile["address-line1"] = profile["street-address"];
> +        delete profile["street-address"];
> +      }
> +
> +      // Remove "address-line*" attributes but keep the values.
> +      let addressLine = [1, 2, 3].map(i => {

Nit: addressLines

::: browser/extensions/formautofill/test/unit/test_transformFields.js:53
(Diff revision 2)
> +      "street-address": "line1\nline2\nline3\nline4",
> +      "address-line1": "line1",
> +      "address-line2": "line2",
> +      "address-line3": "line3",

Can you file a bug to prevent the dataloss by concatenating with a locale-specific character in the future. I don't think it needs to be in the MVP though since this is an edgecase.
Attachment #8860795 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 4

8 months ago
mozreview-review-reply
Comment on attachment 8860795 [details]
Bug 1358943 - [Form Autofill] Support "address-line*" fields.

https://reviewboard.mozilla.org/r/132756/#review137018

Thanks.

> Can you file a bug to prevent the dataloss by concatenating with a locale-specific character in the future. I don't think it needs to be in the MVP though since this is an edgecase.

Filed bug 1334045.
(Assignee)

Comment 5

8 months ago
(In reply to Luke Chang [:lchang] from comment #4)
> Filed bug 1334045.

Sorry. Should be bug 1360114.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

8 months ago
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/20109083820b
[Form Autofill] Support "address-line*" fields. r=MattN

Comment 11

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/20109083820b
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.