Closed Bug 1358943 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Form Manager, enhancement, P3)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: lchang, Assigned: lchang)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M2])

Attachments

(1 file)

Bug 1358943 - [Form Autofill] Support "address-line*" fields.
59 bytes, text/x-review-board-request
MattN
: review+
Details 
Create "address-line1", "address-line2" and "address-line3" fields based on "street-address" when loading data from the storage and vice versa.
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+
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.
(In reply to Luke Chang [:lchang] from comment #4)
> Filed bug 1334045.

Sorry. Should be bug 1360114.
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/20109083820b
[Form Autofill] Support "address-line*" fields. r=MattN
https://hg.mozilla.org/mozilla-central/rev/20109083820b
Status: ASSIGNED → RESOLVED
Closed: 7 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: