Closed
Bug 1365544
Opened 7 years ago
Closed 7 years ago
Handle filling inexact matches for Form Autofill select elements on "address-level1" (states)
Categories
(Toolkit :: Form Manager, enhancement)
Toolkit
Form Manager
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: scottwu, Assigned: scottwu)
References
(Blocks 1 open bug)
Details
(Whiteboard: [form autofill:M3])
Attachments
(1 file)
Bug 1364823 handles filling select element with profile data. It checks if values from profile match select option text/value. However we should handle inexact matches, particularly for address-level1 (States).
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Whiteboard: [form autofill:M3] → [form autofill:M3] ETA:612
Comment 2•7 years ago
|
||
> ::: browser/extensions/formautofill/FormAutofillHandler.jsm:108 > (Diff revision 5) > > + } > > + case "SELECT": { > > + // Do not change value if the option is already selected. > > + // Use case for multiple select is not considered here. > > + let selected = element.options[element.selectedIndex]; > > + if (value === selected.textContent || value === selected.value) { If we keep a check for the label, we should use .text instead of .textContent since .text is normalized: <select><option value="bar"> Foo & baz </select> > $0.text "Foo & baz" > $0.textContent " Foo & baz "
Updated•7 years ago
|
Whiteboard: [form autofill:M3] ETA:612 → [form autofill:M3]
Updated•7 years ago
|
Blocks: fx-form-autofill
Assignee | ||
Updated•7 years ago
|
Summary: Handle filling inexact matches for Form Autofill select elements → Handle filling inexact matches for Form Autofill select elements on "address-level1" (states)
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8870648 [details] Bug 1365544 - Handle filling inexact matches on address-level1 select fields. https://reviewboard.mozilla.org/r/142118/#review154880 There's a case that the matching string is only a substring in the value/label of <option>. We might need to deal with it in this bug. BTW, remember to add unit tests and mochitests to verify it. ::: browser/extensions/formautofill/FormAutofillHandler.jsm:222 (Diff revision 2) > + if (element instanceof Ci.nsIDOMHTMLSelectElement) { > + let option = FormAutofillUtils.findSelectOption(element, profile, fieldDetail.fieldName); > + if (!option) { > + continue; > + } > + element.previewValue = option.text; nit: Please comment on this that we set preview state on it even it's currently selected. ::: browser/extensions/formautofill/FormAutofillUtils.jsm:207 (Diff revision 2) > + if (dataset === undefined) { > + return; > + } > + > + if (fieldName === "address-level1") { > + let keys = dataset["sub_keys"].split("~"); Does it make sense to cache `sub_keys` and `sub_names`? For example: ```js if (!Array.isArray(dataset["sub_keys"]) { dataset["sub_keys"] = dataset["sub_keys"].split("~"); } let keys = dataset["sub_keys"]; ``` ::: browser/extensions/formautofill/FormAutofillUtils.jsm:220 (Diff revision 2) > + if (this.strCompare(identifiedValue, optionValue) || > + this.strCompare(identifiedValue, optionText)) { Since both `identifiedValue` and `optionValue` come from `sub_keys`, can we simply compare them by `==`?
Attachment #8870648 -
Flags: review?(lchang)
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8870648 [details] Bug 1365544 - Handle filling inexact matches on address-level1 select fields. https://reviewboard.mozilla.org/r/142118/#review154880 > nit: Please comment on this that we set preview state on it even it's currently selected. Got it. > Does it make sense to cache `sub_keys` and `sub_names`? For example: > > ```js > if (!Array.isArray(dataset["sub_keys"]) { > dataset["sub_keys"] = dataset["sub_keys"].split("~"); > } > let keys = dataset["sub_keys"]; > ``` Yes I could do that to save on split calls > Since both `identifiedValue` and `optionValue` come from `sub_keys`, can we simply compare them by `==`? You're right! I do have a question about using `==` or `===`. Is `==` preferred over `===`?
Comment 6•7 years ago
|
||
(In reply to Scott Wu [:scottwu] from comment #5) > > Since both `identifiedValue` and `optionValue` come from `sub_keys`, can we simply compare them by `==`? > > You're right! I do have a question about using `==` or `===`. Is `==` > preferred over `===`? I have no preference here actually.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
Addressed issues, added test cases, and fixed eslint errors. I found that using String.prototype.localeCompare has performance issues. MDN[1] suggests using Intl.Collator, and it has indeed solved the problem. I've also added fallbacks to US dataset and locale, so that we can still try to match address-level1 even if user doesn't fill the country field in profile. A regex pattern is added to match state keys (ex. "CA") against option values (ex. "US-CA"). [1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/localeCompare#Performance
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8870648 [details] Bug 1365544 - Handle filling inexact matches on address-level1 select fields. https://reviewboard.mozilla.org/r/142118/#review156184 ::: browser/extensions/formautofill/FormAutofillNameUtils.jsm:209 (Diff revision 3) > let sandbox = {}; > - let scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"] > + FormAutofillUtils.loadDataFromScript(NAME_REFERENCES, sandbox); nit: let sandbox = FormAutofillUtils.loadDataFromScript(NAME_REFERENCES); ::: browser/extensions/formautofill/FormAutofillUtils.jsm:228 (Diff revision 3) > + if (identifiedValue === optionValue || identifiedValue === optionText || > + pattern.test(option.value)) { nit: Put them in one line if they are not exceed 140 characters ::: browser/extensions/formautofill/FormAutofillUtils.jsm:236 (Diff revision 3) > + } > + } > + } > + > + if (fieldName === "country") { > + // TODO: Support matching countries Please file a bug and attach the bug number here.
Attachment #8870648 -
Flags: review?(lchang) → review+
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8870648 [details] Bug 1365544 - Handle filling inexact matches on address-level1 select fields. https://reviewboard.mozilla.org/r/142118/#review156184 Thanks Luke. I've fixed the issues, and also added a check to return early on empty value. Will run tests on try before checking it in.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
Try results look okay: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfe2939ce4f1&selectedJob=109125207
Keywords: checkin-needed
Comment 13•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a3ca1ad0db29 Handle filling inexact matches on address-level1 select fields. r=lchang
Keywords: checkin-needed
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a3ca1ad0db29
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•