Closed Bug 1380910 Opened 2 years ago Closed 2 years ago

[Form Autofill] Consider showing the intersection of results' categories and form's categories instead of the all categories in also fill note

Categories

(Toolkit :: Form Manager, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- verified
firefox57 --- fixed

People

(Reporter: ralin, Assigned: ralin)

References

Details

(Whiteboard: [form autofill:M4])

Attachments

(1 file)

Right now, when none of the profiles are selected we show all categories of  the fields in the focused form. Maybe the notes should respect the results from search instead of the form on the page, because the results are truly what we're going to auto-filling for users.

More precisely example:

If user has two categories of fields in the storage, like
[Field Name]              [Category]
- name:  Tim Berners-Lee  (name)
- email: timbl@w3.org     (email)

When user click on name field in the form which contains the following fields:
- name.                   (name)
- organization            (company)
- email                   (email)
- tel                     (phone)


Expected content of the notes:
Also fill email

Actual content:
Also fill company, email, phone
(It is confusing and a bit misleading since user doesn't have "tel" and "organization" in his profiles....)
s/If user has two categories of fields in the storage/If user has two categories of data in his only one profile/
Yeah, I agree with the "Expected content"
Sounds reasonable to me.
BTW, the string should be "Autofill email"
(In reply to Juwei Huang[:juwei] from comment #3)
> Sounds reasonable to me.
Thanks :D

> BTW, the string should be "Autofill email"
My fault didn't use the latest specced string in the description. The string will be updated in bug 1378072 soon.
Assignee: nobody → ralin
Status: NEW → ASSIGNED
Summary: [Form Autofill] Consider showing the union of results' categories instead the categories of all fields in also fill note → [Form Autofill] Consider showing the intersection of results' categories and form's categories instead of the all categories in also fill note
Comment on attachment 8896101 [details]
Bug 1380910 - Enhance form autofill note by showing the categories intersection of result fields and form fields instead of their union.

https://reviewboard.mozilla.org/r/167384/#review173332

LGTM! Thank you.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:29
(Diff revision 1)
>      // The user's query string
>      this.searchString = searchString;
>      // The field name of the focused input.
>      this._focusedFieldName = focusedFieldName;
>      // All field names in the form which contains the focused input.
>      this._allFieldNames = allFieldNames;

This `_allFieldNames` is based on a `form`, and you provide `fillableFieldNames` is based on all `matchingProfiles`.

For the original `_allFieldNames`, I don't think this would be useful if we have `fillableFieldNames`.

I just want to avoid the similiar variable in the same class, and the definition of `fillableFieldNames` looks more sense to me than the original `_allFieldNames`.
```JS
    const fillableFieldNames = [...matchingProfiles.reduce((fieldSet, curProfile) => {
      for (let field of Object.keys(curProfile)) {
        fieldSet.add(field);
      }

      return fieldSet;
    }, new Set())].filter(field => allFieldNames.includes(field));

    this._allFieldNames = fillableFieldNames;
```

How do you think moving `fillableFieldNames` related code to replace `this._allFieldNames` in the constructor like above?

::: browser/extensions/formautofill/content/formautofill.xml:179
(Diff revision 1)
> +           *
> +           * There're three different states of warning message:
> +           * 1. None of addresses were selected: We show all the categories intersection of fields in the
> +           *    form and fields in the results.
> +           * 2. An address was selested: Show the additional categories that will also be filled.
> +           * 3. An address was selected, but the focused category is the same as the only all categories: Only show

For "the same as the only all categories", does this mean "the same as the only one category"?
Attachment #8896101 - Flags: review?(selee) → review+
Comment on attachment 8896101 [details]
Bug 1380910 - Enhance form autofill note by showing the categories intersection of result fields and form fields instead of their union.

https://reviewboard.mozilla.org/r/167384/#review173332

Thanks for review :D

Since bug 1371149 will be landing soon, I'd like to rebase on that first and then fix the issues in order not to increase the chance of merging conflict.

> This `_allFieldNames` is based on a `form`, and you provide `fillableFieldNames` is based on all `matchingProfiles`.
> 
> For the original `_allFieldNames`, I don't think this would be useful if we have `fillableFieldNames`.
> 
> I just want to avoid the similiar variable in the same class, and the definition of `fillableFieldNames` looks more sense to me than the original `_allFieldNames`.
> ```JS
>     const fillableFieldNames = [...matchingProfiles.reduce((fieldSet, curProfile) => {
>       for (let field of Object.keys(curProfile)) {
>         fieldSet.add(field);
>       }
> 
>       return fieldSet;
>     }, new Set())].filter(field => allFieldNames.includes(field));
> 
>     this._allFieldNames = fillableFieldNames;
> ```
> 
> How do you think moving `fillableFieldNames` related code to replace `this._allFieldNames` in the constructor like above?

I agree. ProfileAutoCompleteResult doesn't care about the fields other than fillable ones at least for now, so I think it's fine  "allFieldNames" can fully replaced with "fillableFieldNames", maybe can reduce some unnecessary iterations as well :P

> For "the same as the only all categories", does this mean "the same as the only one category"?

yeah, it should be what you said, thanks.
Comment on attachment 8896101 [details]
Bug 1380910 - Enhance form autofill note by showing the categories intersection of result fields and form fields instead of their union.

https://reviewboard.mozilla.org/r/167384/#review173840

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:45
(Diff revision 3)
>      this.defaultIndex = 0;
>      // The reason the search failed
>      this.errorDescription = "";
>      // The value used to determine whether the form is secure or not.
>      this._isSecure = isSecure;
> +    // All fillable field names in the form which contains the focused input.

nit: All fillable field names in the form including the field name of the currently-focused input.

::: browser/extensions/formautofill/content/formautofill.xml:203
(Diff revision 3)
>              let orderedCategoryList = ["address", "name", "organization", "tel", "email"];
>              let showCategories = hasExtraCategories ?
>                orderedCategoryList.filter(category => categories.includes(category) && category != this._focusedCategory) :
>                [this._focusedCategory];
> +
> +            let sep = this._stringBundle.GetStringFromName("fieldNameSeparator");

nit: s/sep/separator/
Attachment #8896101 - Flags: review?(lchang) → review+
Thanks
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/323abbc4f721
Enhance form autofill note by showing the categories intersection of result fields and form fields instead of their union. r=lchang,seanlee
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/323abbc4f721
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Verified using latest 56 beta 6 that this is fixed across platforms (Windows 10 64bit, macOS 10.12.6 and Ubuntu 16.04 64bit).
You need to log in before you can comment on or make changes to this bug.