Closed
Bug 1380910
Opened 7 years ago
Closed 7 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)
Toolkit
Form Manager
Tracking
()
RESOLVED
FIXED
mozilla57
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....)
Assignee | ||
Comment 1•7 years ago
|
||
s/If user has two categories of fields in the storage/If user has two categories of data in his only one profile/
Comment 2•7 years ago
|
||
Yeah, I agree with the "Expected content"
Comment 3•7 years ago
|
||
Sounds reasonable to me. BTW, the string should be "Autofill email"
Assignee | ||
Comment 4•7 years ago
|
||
(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 | ||
Updated•7 years ago
|
Assignee: nobody → ralin
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
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 6•7 years ago
|
||
mozreview-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 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+
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
mozreview-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/#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+
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
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
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/323abbc4f721
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 15•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/786925d985fb
status-firefox56:
--- → fixed
Flags: in-testsuite+
Comment 16•7 years ago
|
||
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.
Description
•