Handle filling inexact matches for Form Autofill select elements on "address-level1" (states)

RESOLVED FIXED in Firefox 56

Status

()

Product:
Toolkit
Component:
Form Manager
Type:
enhancement
Status:
RESOLVED FIXED
Opened:
2 years ago
Updated:
2 years ago

People

(Reporter: scottwu, Assigned: scottwu)

Tracking

(Blocks 1 bug)

Version:
unspecified
Target:
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [form autofill:M3])

Attachments

(1 attachment)

Bug 1365544 - Handle filling inexact matches on address-level1 select fields.
59 bytes, text/x-review-board-request
lchang
: review+
Details 
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).
Whiteboard: [form autofill:M3] → [form autofill:M3] ETA:612
> ::: 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 &amp; baz </select>

> $0.text 
"Foo & baz" 
> $0.textContent 
"  Foo  &  baz  "
Whiteboard: [form autofill:M3] ETA:612 → [form autofill:M3]
Summary: Handle filling inexact matches for Form Autofill select elements → Handle filling inexact matches for Form Autofill select elements on "address-level1" (states)
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)
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 `===`?
(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.
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 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+
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.
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
https://hg.mozilla.org/mozilla-central/rev/a3ca1ad0db29
Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
See Also: → 1022925
You need to log in before you can comment on or make changes to this bug.