1405997 - [Form Autofill] Fill a single `tel` field with `tel-national` number

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P3)

Description

[Sean Lee [:seanlee][:weilonge]]

When there is only one `tel` field recognized in a set of telephone fields, the `tel` field should be filled with `tel-national` number instead of `tel`.

Comment 1

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 1405997 - Fill a non-autocomplete tel field with tel-national value.

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

Comment 2

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8915696 [details]
Bug 1405997 - Fill a non-autocomplete tel field with tel-national value.

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

Comment 3

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8915696 [details]
Bug 1405997 - Fill a non-autocomplete tel field with tel-national value.

https://reviewboard.mozilla.org/r/186906/#review192620

Overall looks good with a few comments below.

BTW, you miss a test case: `Always use "tel-national" if "pattern" and "maxlength" aren't specified on a field without "autocomplete" attribute`

::: browser/extensions/formautofill/FormAutofillHandler.jsm:239
(Diff revision 2)
>          }
>        }
>      }
>    },
>  
> +  _telTransformer(profile) {

I would recommend to leave some comments explaining what we intend to do here.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:262
(Diff revision 2)
> +        return;
> +      }
> +    }
> +
> +    if (detail._reason != "autocomplete") {
> +      profile.tel = profile["tel-national"];

Add a comment to explain it's a workaround because "tel-national" field is used more commonly.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:265
(Diff revision 2)
> +
> +    if (detail._reason != "autocomplete") {
> +      profile.tel = profile["tel-national"];
> +    } else {
> +      if (element.pattern) {
> +        let pattern = new RegExp(element.pattern, "u");

nit: It seems able to reuse the previous `pattern`, doesn't it?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:268
(Diff revision 2)
> +    } else {
> +      if (element.pattern) {
> +        let pattern = new RegExp(element.pattern, "u");
> +        if (pattern.test(profile["tel-national"])) {
> +          profile.tel = profile["tel-national"];
> +          return;

The `return` looks redundant. Is it intentional?

Comment 4

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8915696 [details]
Bug 1405997 - Fill a non-autocomplete tel field with tel-national value.

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

Comment 5

[Sean Lee [:seanlee][:weilonge]]

Hey Luke, Thanks for the review. Could you review the latest patch again?

Comment 6

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8915696 [details]
Bug 1405997 - Fill a non-autocomplete tel field with tel-national value.

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

Comment 7

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8915696 [details]
Bug 1405997 - Fill a non-autocomplete tel field with tel-national value.

https://reviewboard.mozilla.org/r/186906/#review192654

Thanks.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:240
(Diff revision 3)
> +   * `tel` value in a profile needs to be converted to `tel-national` when `tel`
> +   *  value can not be filled into a `tel` field.
> +   * @param {Object} profile
> +   *        A profile to be converted and filled in.

nit: "Replace `tel` with `tel-national` if `tel` violates the input element's restriction."

Also, I would prefer to delete "and filled in" because it sounds like we're going to fill the profile in somewhere within this function (but we are not).

::: browser/extensions/formautofill/FormAutofillHandler.jsm:274
(Diff revision 3)
> +      // `tel-national` is used widely than `tel`, so the filling value of `tel`
> +      // is chagned to `tel-national` for a field without `autocomplete` attr.

Please also mention that we should improve it once more countries are supported.

BTW, I prefer to comment like below.

```
Since we only target people living in US and using en-US websites in MVP, it makes more sense to fill `tel-national` instead of `tel` if the field is identified by heuristics and no other clues to determine which one is better.

TODO: We should improve this once more countries are supported (Bug XXXXXXX).
```

Comment 8

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8915696 [details]
Bug 1405997 - Fill a non-autocomplete tel field with tel-national value.

https://reviewboard.mozilla.org/r/186906/#review192660

::: browser/extensions/formautofill/FormAutofillHandler.jsm:246
(Diff revision 4)
> +   *  value can not be filled into a `tel` field.
> +   * @param {Object} profile
> +   *        A profile to be converted and filled in.
> +   */
> +  _telTransformer(profile) {
> +    if (!profile["tel-national"]) {

You should test the existence of `profile.tel` as well.

Comment 9

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8915696 [details]
Bug 1405997 - Fill a non-autocomplete tel field with tel-national value.

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

Comment 10

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8915696 [details]
Bug 1405997 - Fill a non-autocomplete tel field with tel-national value.

https://reviewboard.mozilla.org/r/186906/#review192654

> Please also mention that we should improve it once more countries are supported.
> 
> BTW, I prefer to comment like below.
> 
> ```
> Since we only target people living in US and using en-US websites in MVP, it makes more sense to fill `tel-national` instead of `tel` if the field is identified by heuristics and no other clues to determine which one is better.
> 
> TODO: We should improve this once more countries are supported (Bug XXXXXXX).
> ```

bug 1407545 is filed for the TODO here.

Comment 11

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8915696 [details]
Bug 1405997 - Fill a non-autocomplete tel field with tel-national value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186906/diff/5-6/

Comment 12

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8915696 [details]
Bug 1405997 - Fill a non-autocomplete tel field with tel-national value.

https://reviewboard.mozilla.org/r/186906/#review193486

::: browser/extensions/formautofill/FormAutofillHandler.jsm:260
(Diff revision 6)
> +    let element = detail.elementWeakRef.get();
> +    let _pattern;
> +    let testPattern = str => {
> +      if (!_pattern) {
> +        // The pattern has to match the entire value.
> +        _pattern = new RegExp("^(?:" + element.pattern + ")$", "u");

Since we need a full match here, this solution is to match the full string.
Please see the reference design from DOM code:
http://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/dom/base/nsContentUtils.cpp#7169-7171

Comment 13

[Sean Lee [:seanlee][:weilonge]]

Hey Luke, Thank you for the review. All issues that you addressed are fixed.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d305adca613e
Fill a non-autocomplete tel field with tel-national value. r=lchang

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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