Closed
Bug 1375382
Opened 7 years ago
Closed 7 years ago
[Form Autofill] Handle filling in country field select element
Categories
(Toolkit :: Form Manager, enhancement)
Toolkit
Form Manager
Tracking
()
RESOLVED
FIXED
mozilla57
People
(Reporter: scottwu, Assigned: lchang)
References
(Blocks 1 open bug)
Details
(Whiteboard: [form autofill:M3])
Attachments
(1 file)
Form autofill should try to fill country field select element. FormAutofillHandler will first look for the country key (ex. US) on both the option values and texts of a country select element. If there's no exact match, we should try to identify them.
Assignee | ||
Updated•7 years ago
|
Whiteboard: [form autofill:MVP] → [form autofill:M4]
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → lchang
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8891179 [details] Bug 1375382 - [Form Autofill] Handle filling in country field select element. https://reviewboard.mozilla.org/r/162384/#review169160 ::: browser/extensions/formautofill/FormAutofillUtils.jsm:289 (Diff revision 3) > + break; > - } > + } > > - if (fieldName === "country") { > - // TODO: Support matching countries (Bug 1375382) > + let matchingOption; > + alternativeCountryNames.some(name => { > + let reName = new RegExp("\\b" + this.escapeRegExp(name) + "\\b", "i"); `reName` should be kept in a cache since `name` is a const value in a const array. This makes `RegExp("\\b" + this.escapeRegExp(name) + "\\b", "i")` can not be precompiled. ::: browser/extensions/formautofill/ProfileStorage.jsm:1325 (Diff revision 3) > + if (!address.country) { > + for (let country in FormAutofillUtils.ALTERNATIVE_COUNTRY_NAMES) { > + let found = FormAutofillUtils.ALTERNATIVE_COUNTRY_NAMES[country].find(name => { > + let reName = new RegExp("\\b" + FormAutofillUtils.escapeRegExp(name) + "\\b", "i"); > + return reName.test(countryName); > + }); In this snippet, there are three cross-JSM calls in the `for` loop. I suggest to write a helper in `FormAutofillUtils` and use it here. ```JS // This should be in FormAutofillUtils. findCountryCode(countryName) { for (let country in this.ALTERNATIVE_COUNTRY_NAMES) { let found = this.ALTERNATIVE_COUNTRY_NAMES[country].find(name => { let reName = this.getCountryRegexp(name); return reName.test(countryName); }); if (found) { return country; } } return null; }, ``` You seem want to handle more matching cases, so we should have a test for the above cases in `test_transformFields.js` as well.
Attachment #8891179 -
Flags: review?(selee)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
Updated accordingly. Thanks.
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8891179 [details] Bug 1375382 - [Form Autofill] Handle filling in country field select element. https://reviewboard.mozilla.org/r/162384/#review171040 ::: browser/extensions/formautofill/FormAutofillUtils.jsm:235 (Diff revision 4) > + let dataset = this.getCountryAddressData(country); > + this._collator[country] = new Intl.Collator(dataset.lang, {sensitivity: "base", ignorePunctuation: true}); This assumes that there is one language per country which isn't correct so that should be noted here. You can get other languages by splitting the `languages` property on "~" but that still isn't ideal as I think only one gets actually used. It seems like the locale of the page should be taken into account to do this properly. I'm fine with this for the MVP if there's a comment and a bug. Maybe we should have a whiteboard for V2 stuff involving rollout in more locales: "[form autofill:V2]"? ::: browser/extensions/formautofill/FormAutofillUtils.jsm:305 (Diff revision 4) > + case "address-level1": > - if (!Array.isArray(dataset.sub_keys)) { > + if (!Array.isArray(dataset.sub_keys)) { If you're switching to use `case` then you should add {} braces so that variables have block scope
Whiteboard: [form autofill:M4] → [form autofill:M3]
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8891179 [details] Bug 1375382 - [Form Autofill] Handle filling in country field select element. https://reviewboard.mozilla.org/r/162384/#review171990 The patch looks good to me. I will review it again once the issues that MattN addressed are fixed. Thanks.
Attachment #8891179 -
Flags: review?(selee)
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8891179 [details] Bug 1375382 - [Form Autofill] Handle filling in country field select element. https://reviewboard.mozilla.org/r/162384/#review171040 > This assumes that there is one language per country which isn't correct so that should be noted here. You can get other languages by splitting the `languages` property on "~" but that still isn't ideal as I think only one gets actually used. It seems like the locale of the page should be taken into account to do this properly. > > I'm fine with this for the MVP if there's a comment and a bug. Maybe we should have a whiteboard for V2 stuff involving rollout in more locales: "[form autofill:V2]"? Thanks for pointing it out. I should have noticed that. I'll add a comment for now and file a bug to follow up how we can better deal with the languages.
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8891179 [details] Bug 1375382 - [Form Autofill] Handle filling in country field select element. https://reviewboard.mozilla.org/r/162384/#review172112 LGTM! Thank you.
Attachment #8891179 -
Flags: review?(selee) → review+
Comment 12•7 years ago
|
||
Pushed by lchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/40da3955d4a1 [Form Autofill] Handle filling in country field select element. r=seanlee
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/40da3955d4a1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 14•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/64e427f215f7
status-firefox56:
--- → fixed
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•