Closed Bug 1405997 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Form Manager, defect, P3)

Product:
Toolkit
Component:
Form Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- fix-optional
firefox58 --- fixed

People

(Reporter: selee, Assigned: selee)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [form autofill:MVP])

Attachments

(1 file)

Bug 1405997 - Fill a non-autocomplete tel field with tel-national value.
59 bytes, text/x-review-board-request
lchang
: review+
Details 
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 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?
Attachment #8915696 - Flags: review?(lchang)
Hey Luke, Thanks for the review. Could you review the latest patch again?
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).
```
Attachment #8915696 - Flags: review?(lchang) → review+
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.
Blocks: 1407545
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 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
Hey Luke, Thank you for the review. All issues that you addressed are fixed.
Keywords: checkin-needed
status-firefox57: --- → fix-optional
Priority: -- → P3
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d305adca613e
Fill a non-autocomplete tel field with tel-national value. r=lchang
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d305adca613e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: