Closed
Bug 1365544
Opened 8 years ago
Closed 8 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•8 years ago
|
Whiteboard: [form autofill:M3] → [form autofill:M3] ETA:612
Comment 2•8 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•8 years ago
|
Whiteboard: [form autofill:M3] ETA:612 → [form autofill:M3]
Updated•8 years ago
|
Blocks: fx-form-autofill
Assignee | ||
Updated•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
||
Try results look okay: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfe2939ce4f1&selectedJob=109125207
Keywords: checkin-needed
Comment 13•8 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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 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
•