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

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Scott Wu [:scottwu]]

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 1

[Scott Wu [:scottwu]]

Attachment: Bug 1365544 - Handle filling inexact matches on address-level1 select fields.

Review commit: https://reviewboard.mozilla.org/r/142118/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142118/

Comment 2

[Matthew N. [:MattN]]

> ::: 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  "

Comment 3

[Scott Wu [:scottwu]]

Comment on attachment 8870648 [details]
Bug 1365544 - Handle filling inexact matches on address-level1 select fields.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142118/diff/1-2/

Comment 4

[Luke Chang [:lchang] (inactive)]

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 `==`?

Comment 5

[Scott Wu [:scottwu]]

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

[Luke Chang [:lchang] (inactive)]

(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 7

[Scott Wu [:scottwu]]

Comment on attachment 8870648 [details]
Bug 1365544 - Handle filling inexact matches on address-level1 select fields.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142118/diff/2-3/

Comment 8

[Scott Wu [:scottwu]]

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

[Luke Chang [:lchang] (inactive)]

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.

Comment 10

[Scott Wu [:scottwu]]

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 11

[Scott Wu [:scottwu]]

Comment on attachment 8870648 [details]
Bug 1365544 - Handle filling inexact matches on address-level1 select fields.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142118/diff/3-4/

Comment 12

[Scott Wu [:scottwu]]

Try results look okay: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfe2939ce4f1&selectedJob=109125207

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3ca1ad0db29
Handle filling inexact matches on address-level1 select fields. r=lchang

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a3ca1ad0db29