Fill the selected autofill profile when an autocomplete entry is chosen

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
Form Manager
P3
enhancement
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: MattN, Assigned: seanlee)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
mozilla54
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [form autofill:M1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 2 obsolete attachments)

When an autocomplete entry is selected, send a message to the child to fill the relevant section of the form using the API in bug 1300988.
It may also be useful to listen for the DOMAutocomplete event.

Comment 2

10 months ago
Just a quick note about what I realize for filling the selected result from: It seems like AutoCompletePopup will send FormAutoComplete:HandleEnter message while item selected[1], and browser-content will receive this message for calling autocomplete controller's handleEnter API[2]. Since we will not leverage the controller to fill the profile, we might require more discussion for possible solution here.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/AutoCompletePopup.jsm#279
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/content/browser-content.js#1470

Comment 3

10 months ago
(In reply to Matthew N. [:MattN] from comment #1)
> It may also be useful to listen for the DOMAutocomplete event.

Seems like the loginmanager fill the password field based on the DOMAutocomplete event[1], but I'm not sure whether we should implement another profile insertion API in content.js.
 
[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#80
You need to use the perma link!

comment 2:
[1] https://dxr.mozilla.org/mozilla-central/rev/47f42f21541b9b98ad7db82edb996b29065debd0/toolkit/components/satchel/AutoCompletePopup.jsm#279
[2] https://dxr.mozilla.org/mozilla-central/rev/47f42f21541b9b98ad7db82edb996b29065debd0/toolkit/content/browser-content.js#1498-1505

comment 3:

[1] https://dxr.mozilla.org/mozilla-central/rev/47f42f21541b9b98ad7db82edb996b29065debd0/browser/base/content/content.js#80
... and this is where the value is actually being placed into the input box and the DOMAutocomplete event is dispatched.

https://dxr.mozilla.org/mozilla-central/rev/47f42f21541b9b98ad7db82edb996b29065debd0/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1414-1563

I can see a few interesting call here:

* Maybe we could use |popup->GetOverrideValue(value);| to switch the guid passed from the popup to the actual value?
* Maybe we could use the two observer triggered in the function instead of the event?

I will try to dig deeper...
The overrideValue is a rabbit hole which goes back to the parent process, so it would not help us unless we change the result object to take another |overrideValues| array. This is too big of a change that I think we should avoid.

I think we should do the following:

1. have the |values| in the result array to be the actual value to put into the input, and have nsAutoCompleteController::EnterMatch() to put the selected on into the text field.
2. The DOMAutocomplete event will get triggered at the end of that function, after the text has been put into the field.
3. Upon receiving the DOMAutocomplete event, we ask for AutoCompletePopup.selectedIndex from chrome or content process to tell which profile is selected.
4. Get the profile and set the value of other text fields.

(3) is a bit tricky. AutoCompletePopup in content is just a dummy of the parent object; it lives in browser-content.js so I don't think we could get a reference of it. This means we would need the parent script to do (3) upon receiving the message passed from child, and send back the profile needed for (4) upon receiving the message.

I will need the skeleton of search-result-returning function to start implement this.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #6)
> I will need the skeleton of search-result-returning function to start
> implement this.

... and that's bug 1304634
Depends on: 1304634

Updated

8 months ago
Blocks: 1325538
Comment hidden (mozreview-request)
Comment on attachment 8822357 [details]
Bug 1300989 - Part 1: Add guid interface, f?=MattN

Hi Matt,
It's the patch is about guid interface mentioned in bug 1300992 comment 2. I think it might be more suitable for this bug(or you prefer to move this patch into another new bug).
Attachment #8822357 - Flags: feedback?(MattN+bmo)
And in this patch you could use this.openedPopup.view.getGuidAt(selectedIndex) while receiving handleEnter event in AutoCompletePopup.jsm.
(Assignee)

Comment 11

8 months ago
Hi MattN,

I am going to work on this bug. In the current design, FormAutoCompleteResult[1] is used to provide the result for Profile AUtoComplete Popup. I would suggest to create ProfileAutoCompletePopup for handling profile case only. Here are some reasons:
1. AFAIK, FormAutoCompleteResult is for form history. An isolated design can avoid the mixed codes of FormHistory and Profile in FormAutoCompleteResult.
2. Reducing the risk of affecting FormHistory feature.
3. Put all Profile relative codes into the same directory.

May I have your suggestions? Thanks.

[1] http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/toolkit/components/satchel/nsFormAutoCompleteResult.jsm#12
(Assignee)

Updated

8 months ago
Assignee: nobody → selee
(Assignee)

Updated

8 months ago
Depends on: 1328778
(Assignee)

Comment 12

8 months ago
For comment 11, bug 1328778 is for discussing the implementation of AutoCompleteResult for Profile.
Comment on attachment 8822357 [details]
Bug 1300989 - Part 1: Add guid interface, f?=MattN

https://reviewboard.mozilla.org/r/101304/#review102806

Without seeing an example of where we would need to call getGuidAt I'm thinking that my proposal to have getLabelAt or getCommentAt contain the whole serialized profile should remove the need for this new API which I'm hesistant to add.
Comment on attachment 8822357 [details]
Bug 1300989 - Part 1: Add guid interface, f?=MattN

(In reply to Steve Chung [:steveck] from comment #10)
> And in this patch you could use
> this.openedPopup.view.getGuidAt(selectedIndex) while receiving handleEnter
> event in AutoCompletePopup.jsm.

With my proposal you could get the whole profile object (containing a guid property) with getLabelAt and/or getCommentAt which should remove the need for a separate API.
Attachment #8822357 - Flags: feedback?(MattN+bmo)
(Assignee)

Comment 15

8 months ago
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #14)
> Comment on attachment 8822357 [details]
> Bug 1300989 - Part 1: Add guid interface, f?=MattN
> 
> (In reply to Steve Chung [:steveck] from comment #10)
> > And in this patch you could use
> > this.openedPopup.view.getGuidAt(selectedIndex) while receiving handleEnter
> > event in AutoCompletePopup.jsm.
> 
> With my proposal you could get the whole profile object (containing a guid
> property) with getLabelAt and/or getCommentAt which should remove the need
> for a separate API.

If getLabelAt and/or getCommentAt will return the whole profile, does it mean we have to redesign the richlistitem binding for profile items like the discussion at bug 1326138 comment 3?

Another question is how do we know the user's selection after user hits enter key or mouse-down? Use "selectedIndex" at [1], then file another event (with results[selectedIndex]) and handle it in FormFill extension?

[1] http://searchfox.org/mozilla-central/rev/0f254a30d684796bcc8b6e2a102a0095d25842bb/toolkit/content/browser-content.js#1496
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 16

8 months ago
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #13)
> Comment on attachment 8822357 [details]
> Bug 1300989 - Part 1: Add guid interface, f?=MattN
> 
> https://reviewboard.mozilla.org/r/101304/#review102806
> 
> Without seeing an example of where we would need to call getGuidAt I'm
> thinking that my proposal to have getLabelAt or getCommentAt contain the
> whole serialized profile should remove the need for this new API which I'm
> hesistant to add.

May I know why you are hesitant to add the new API?
IMHO, getGuidAt or getValueObjectAt is more meaningful to provide a detail information (like profile).
(Assignee)

Comment 17

8 months ago
I summarize this bug into couple steps after reading the bug comments:
1. Provide the primary and secondary information for user to choose the profiles.
  - timing to transform the profile into the human-readable string.
  - Two possible solutions here for discussing getValueAt/getLabelAt/getCommentAt in ProfileAutoCompleteResult:
    A. Don't modify *.idl and use the current APIs.
       - Need to extend richlistitem binding for satisfying the customized usage of getLabelAt/getCommentAt.
       - pros: Don't need to change the original autocomplete codes like controller, etc.
       - pros: we can have more flexibility to adjust UI.
       - risk: so there would be more works than extending autocomplete-richlistitem.
    B. Add getValueObjectAt/getGuidAt support.
       - pros: we can reuse and extend autocomplete-richlistitem.
       - cons: more works to add *.idl and *.cpp codes.
       - risk: if extending autocomplete-richlistitem can fulfill the current spec.

2. Handle the selection and pass it to FormAutofillContent
  - AFAIU, [1] is a place to file a custom event to notify FormAutofillContent to handle the selection.
  - The event should contain:
    - style (e.g. "autofill-profile") for FormAutofillContent to judge if it wants to handle this selection.
    - GUID - for querying the full profile of a selected item if needed.
  - Two options here for the event data format:
    - A. Full profile - for filling fields.
    - B. GUID + style

3. Handle the selected profile for filling form in FormAutofillContent.
  - FormAutofillContent can query the full profile from GUID or use the profile in event directly. (depends on the event data format in 2.)
  - Only handle the interested style selection. This can avoid the confusion when there are many event listener.

4. Send DOMAutoComplete event to fulfill the documentation [2] when filling fields automatically.
  - We can have another bug to discuss this. However, I think the timing might be important. (e.g. event race condition)

Some additional thoughts based on above items:
* For the item [1.A.] above, the APIs in ProfileAutoCompleteResult can be in such usage:
  [getValueAt] - return the primary value for the original form history code path to fill the focused field. There would be a timing issue if it returns an empty string here then file DOMAutoComplete event. AFAIK, LoginManager will use DOMAutoComplete event to search matching logins with input.value and fill the other field, e.g. password.
  [getLabelAt] - return the JSON serialized string with the primary and secondary information. In this way, item-binding don't need to know the schema of profile.
  [getCommentAt] - return the JSON serialized string with the full profile or GUID only. This is for [1] to file a custom event including the selected profile.

I know these information might be too much, but I wish this can clarify my thoughts and speed up the implementation discussion.

Thank you for reading these.

[1] http://searchfox.org/mozilla-central/rev/e3e8571c5378ac92663d4f583ccc4ad0a3019716/toolkit/content/browser-content.js#1465
[2] https://developer.mozilla.org/en-US/docs/Web/Events/DOMAutoComplete
[3] http://searchfox.org/mozilla-central/rev/e3e8571c5378ac92663d4f583ccc4ad0a3019716/browser/base/content/content.js#79
Iteration: --- → 53.5 - Jan 23
(Assignee)

Comment 18

7 months ago
After discussing this proposal, we had the agreements below:
* Adopt the option A for (item 1) and implement the new binding based on richlistitem in bug 1326138.
* Listen "FormAutoComplete:HandleEnter" message in FormAutofillContent instead of creating a custom event for the user selection.
The message data includes whole profile and style. If we can get a reference to the popup (and it's attributes e.g. id) it's another option for recognizing the interested profile.
* Handle DOMAutoComplete and input field events in another bug.
* Checking the form filling behavior won’t have a side-effect between Profile Form Autofill and LoginManagement.
Process the primary and secondary information in ProfileAutoCompleteResult.

I am going to implement this feature based on the above items.

Thanks a lot for the team to have this great discussion.
Flags: needinfo?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

7 months ago
Comment on attachment 8826030 [details]
Bug 1300989 - Part 1: Implement the profile filling feature when a user selects one.;

Hey MattN,

Could you give this patch which contains the following changes a feedback:
1. Implement `ProfileAutoCompleteResult._generateLabels` function and store the result into `ProfileAutoCompleteResult._popupLabels`.
2. `ProfileAutoCompleteResult.getValueAt` returns the primary value for field filling.
3. `ProfileAutoCompleteResult.getLabelAt` returns the primary and secondary labels for UI binding.
4. Pass the selected profile to `FormAutofillContent` by "FormAutoComplete:HandleEnter" message.

Something should be done in the follow-up bugs:
1. Refine the name label relative rules. This patch only provides a simple rule to get the full name. We can refine it in M2.
2. Fill the other fields based on the selected profile.
3. The verification of label generating rules should be included in ProfileAutoCompleteResult xpc-shell test 

Thanks.
Attachment #8826030 - Flags: feedback?(MattN+bmo)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8826030 - Flags: feedback?(MattN+bmo)
Comment on attachment 8826030 [details]
Bug 1300989 - Part 1: Implement the profile filling feature when a user selects one.;

https://reviewboard.mozilla.org/r/104082/#review106212

These are my comments for now but there are still a few details to look into next time but it's late.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:10
(Diff revision 4)
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["FormAutofillUtils"];
> +
> +this.FormAutofillUtils = {
> +  generateFullName(firstName, lastName, middleName) {

FWIW I would also be fine with this in the storage JSM instead.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:78
(Diff revision 4)
>        throw Components.Exception("Index out of range.", Cr.NS_ERROR_ILLEGAL_VALUE);
>      }
>    },
>  
> +  _getSecondaryLabel(focusedFieldName, existingFieldNames, profile) {
> +    /* TODO: Since "name" is a special case here, so the seconday "name" label

typo: secondary

::: browser/extensions/formautofill/content/FormAutofillContent.js:243
(Diff revision 4)
>        guid: "test-guid-2",
>        organization: "Mozilla",
>        streetAddress: "331 E. Evelyn Avenue",
>        tel: "1-650-903-0800",
>      }];
> -    let result = new ProfileAutoCompleteResult(searchString, fieldName, profiles, {});
> +    let existingFieldNames = ["organization", "tel", "streetAddress"];

What does this variable mean?

::: browser/extensions/formautofill/content/FormAutofillContent.js:246
(Diff revision 4)
> +                                               existingFieldNames,
> +                                               profiles, {});

Nit: Stick with one argument per line if you start that.

::: browser/extensions/formautofill/content/FormAutofillContent.js:323
(Diff revision 4)
>          break;
>      }
>    },
>  
> +  receiveMessage(message) {
> +    // TODO: Handle the form autofilling part.

…only if `isPopupSelection`

::: toolkit/components/satchel/AutoCompletePopup.jsm:279
(Diff revision 4)
>     * Despite its name, handleEnter is what is called when the
>     * user clicks on one of the items in the popup.
>     */
>    handleEnter(aIsPopupSelection) {
>      if (this.openedPopup) {
> +      let popup = this.openedPopup;

This variable doesn't seem necessary and isn't used in two places

::: toolkit/components/satchel/AutoCompletePopup.jsm:283
(Diff revision 4)
>          isPopupSelection: aIsPopupSelection,
> +        value: popup.view.getValueAt(selectedIndex),
> +        label: popup.view.getLabelAt(selectedIndex),
> +        comment: popup.view.getCommentAt(selectedIndex),
> +        style: popup.view.getStyleAt(selectedIndex),

We should only return these new fields if `aIsPopupSelection` I think. I believe selectedIndex can be -1 otherwise. I hope an existing mochitest would have caught this.

https://dxr.mozilla.org/mozilla-central/rev/6a23526fe5168087d7e4132c0705aefcaed5f571/toolkit/components/autocomplete/nsIAutoCompleteController.idl#64-70
Attachment #8826030 - Flags: feedback?(MattN+bmo) → feedback+
Attachment #8822357 - Attachment is obsolete: true
(Assignee)

Comment 25

7 months ago
mozreview-review-reply
Comment on attachment 8826030 [details]
Bug 1300989 - Part 1: Implement the profile filling feature when a user selects one.;

https://reviewboard.mozilla.org/r/104082/#review106212

> FWIW I would also be fine with this in the storage JSM instead.

`generateFullName` placed in FormAutofillUtils can be accessed by ProfileStorage as well. I would like to leave this function in FormAutofillUtils.

> What does this variable mean?

That's for composing primary and secondary labels. Steve mentioned this in https://bugzilla.mozilla.org/show_bug.cgi?id=1300992#c15

`let existingFieldNames = ["organization", "tel", "streetAddress"];` // this means there are only "organization", "tel", "streetAddress" existing in a form.

Based on the spec https://mozilla.invisionapp.com/share/TM8GRODVH#/screens/185446489, the 1st item is "streetAddress", the second one is "organization". That's why the "name" will be ignored even its priority is greater than "organization".

However, we are still discussing how to compose this array. The example is an array of field name strings, but it's probably useful to contain other information in an object array like this:
let existingFields = [{
                        name: "organization",
                        ...
                      }, ...];
                      
But we don't have an idea if it's worth yet.

> This variable doesn't seem necessary and isn't used in two places

It's used at popup.view.getValueAt(selectedIndex) and following lines.
(Assignee)

Comment 26

7 months ago
(In reply to Sean Lee [:seanlee][:weilonge] from comment #25)
> Comment on attachment 8826030 [details]
> Bug 1300989 - Implement the profile filling feature when a user selects one.
> 
> https://reviewboard.mozilla.org/r/104082/#review106212
> 
> > FWIW I would also be fine with this in the storage JSM instead.
> 
> `generateFullName` placed in FormAutofillUtils can be accessed by
> ProfileStorage as well. I would like to leave this function in
> FormAutofillUtils.
May I know your reason to put `generateFullName` in ProfileStorage? If it's placed in ProfileStorage, that means we have to discuss the place of relative transforms as well. (e.g., country code/name, street address concat or truncat, etc.)

This will affect how to write the xpc-shell test as well.
Comment hidden (mozreview-request)
(Assignee)

Comment 28

7 months ago
mozreview-review
Comment on attachment 8826030 [details]
Bug 1300989 - Part 1: Implement the profile filling feature when a user selects one.;

https://reviewboard.mozilla.org/r/104082/#review107448

::: toolkit/components/satchel/AutoCompletePopup.jsm:292
(Diff revision 5)
> +        messageData.value = popup.view.getValueAt(selectedIndex);
> +        messageData.label = popup.view.getLabelAt(selectedIndex);
> +        messageData.comment = popup.view.getCommentAt(selectedIndex);
> +        messageData.style = popup.view.getStyleAt(selectedIndex);
> +*/
> +        messageData.fullResult = this.openedPopup.view.results[selectedIndex];

Since popup.view.getCommentAt [1] returns the `label` instead of `comment`, There are two possibile solutions in my mind:
1. Use view.results to get the full result object of the selected one. This is what this patch provides exactly.

2. We still can get label via getCommentAt. How do you think if we use `comment` to contain the full profile? So getCommentAt here will return the object:
{
  primary: "...",
  secondary: "...",
  fullProfile: {
    guid: "...",
    ...
    
  },
}

May I have your suggestion here?

(The above comment is still there for comparison, and it will be removed in the following patch.)

[1] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/toolkit/components/satchel/AutoCompletePopup.jsm#46
(Assignee)

Updated

7 months ago
Attachment #8826030 - Flags: feedback?(MattN+bmo)
(Reporter)

Comment 29

7 months ago
mozreview-review-reply
Comment on attachment 8826030 [details]
Bug 1300989 - Part 1: Implement the profile filling feature when a user selects one.;

https://reviewboard.mozilla.org/r/104082/#review107448

> Since popup.view.getCommentAt [1] returns the `label` instead of `comment`, There are two possibile solutions in my mind:
> 1. Use view.results to get the full result object of the selected one. This is what this patch provides exactly.
> 
> 2. We still can get label via getCommentAt. How do you think if we use `comment` to contain the full profile? So getCommentAt here will return the object:
> {
>   primary: "...",
>   secondary: "...",
>   fullProfile: {
>     guid: "...",
>     ...
>     
>   },
> }
> 
> May I have your suggestion here?
> 
> (The above comment is still there for comparison, and it will be removed in the following patch.)
> 
> [1] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/toolkit/components/satchel/AutoCompletePopup.jsm#46

It think the best thing would be to stop using the awesomebar binding for form history. Ray is trying to make the form history one not extend the awesomebar one and if that works we should port the solution to form history. 

Until we know if that will work I think we move the bad naming mismatch (label vs. comment) from AutoCompletePopup.jsm to the two consumers that are using using the awesomebar binding[1][2]. Just swap the getLabelAt and getCommentAt function names to preserve blames for the implementations. Is there any downside with that?

What do you think?

[1] https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/toolkit/components/passwordmgr/LoginManagerContent.jsm#1332-1367
[2] nsFormAutoComplete.js or is it nsFormAutoCompleteResult.jsm? I can't tell the difference from quick inspection and one doesn't have in-file documentation.
(Assignee)

Updated

7 months ago
Depends on: 1336370
(Assignee)

Comment 30

7 months ago
I've filed a new bug 1336370 to discuss how to fix the issue (mentioned in comment 29).
Comment hidden (mozreview-request)
(Assignee)

Comment 32

7 months ago
After having an offline discussion with MattN, the issue in bug 1336370 is resolved with a workaround to return "comment" in getLabelAt, so it won't block this bug now.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 35

7 months ago
Hey MattN,

The latest patch includes "autocomplete-will-enter-text" solution with sendSyncMessage("FormAutoComplete:GetSelectedIndex", {}), so the "Enter" key issue fixed. We don't need to change AutoCompletePopup.jsm anymore. However, we can discuss if it's worth to fix getLabelAt and getCommentAt issue (bug 1336370).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8834772 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Depends on: 1338458
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 41

6 months ago
mozreview-review-reply
Comment on attachment 8826030 [details]
Bug 1300989 - Part 1: Implement the profile filling feature when a user selects one.;

https://reviewboard.mozilla.org/r/104082/#review107448

> It think the best thing would be to stop using the awesomebar binding for form history. Ray is trying to make the form history one not extend the awesomebar one and if that works we should port the solution to form history. 
> 
> Until we know if that will work I think we move the bad naming mismatch (label vs. comment) from AutoCompletePopup.jsm to the two consumers that are using using the awesomebar binding[1][2]. Just swap the getLabelAt and getCommentAt function names to preserve blames for the implementations. Is there any downside with that?
> 
> What do you think?
> 
> [1] https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/toolkit/components/passwordmgr/LoginManagerContent.jsm#1332-1367
> [2] nsFormAutoComplete.js or is it nsFormAutoCompleteResult.jsm? I can't tell the difference from quick inspection and one doesn't have in-file documentation.

"autocomplete-will-enter-text" and "FormAutoComplete:GetSelectedIndex" solution are applied in the latest patch. Since we don't need to touch AutoCompletePopup.jsm, it's not an issue now.
Comment on attachment 8835924 [details]
Bug 1300989 - Part 2: Use WeakMap to manage the form details.;

https://reviewboard.mozilla.org/r/111476/#review113086
Attachment #8835924 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8826030 [details]
Bug 1300989 - Part 1: Implement the profile filling feature when a user selects one.;

https://reviewboard.mozilla.org/r/104082/#review111896

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:18
(Diff revision 9)
> +XPCOMUtils.defineLazyModuleGetter(this, "FormAutofillUtils",
> +                                  "resource://formautofill/FormAutofillUtils.jsm");
> +
>  this.ProfileAutoCompleteResult = function(searchString,
> -                                           fieldName,
> +                                          focusedFieldName,
> +                                          existingFieldNames,

How about `adjacentFieldNames`, `otherFieldNames` or `allFieldNames`? I guess it depends on whether this includes the field name of `focusedFieldName`.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:62
(Diff revision 9)
> +  _existingFieldNames: null,
>  
>    // The matching profiles contains the information for filling forms.
>    _matchingProfiles: null,
>  
> +  _popupLabels: null,

Can you add a comment here too?

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:91
(Diff revision 10)
> +      "street-address",  // Street address
> +      "name",           // Full name if needed
> +      "address-level2",  // City/Town
> +      "organization",   // Company or organization name

Nit: re-align the comments

::: browser/extensions/formautofill/content/FormAutofillContent.js:17
(Diff revision 10)
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");

Nit: sort alphabetically

::: browser/extensions/formautofill/content/FormAutofillContent.js:245
(Diff revision 10)
>        if (this.forceStop) {
>          return;
>        }

Do we need to null the cached results in this case?

::: browser/extensions/formautofill/content/FormAutofillContent.js:386
(Diff revision 10)
>     */
>    getInputDetails(element) {
>      for (let formDetails of this._formsDetails) {
>        for (let detail of formDetails) {
>          if (element == detail.element) {
> -          return this._serializeInfo(detail);
> +          return detail;

I think this is going to cause errors trying to send elements across the process boundary, at least that's why this was added initially.

::: browser/extensions/formautofill/content/FormAutofillContent.js:412
(Diff revision 10)
>      }
>      return null;
>    },
>  
> +  setProfileAutoCompleteResult(result) {
> +    this._profileAutoCompleteResult = result;

This should be defined at the top of the object literal.

::: browser/extensions/formautofill/content/FormAutofillContent.js:412
(Diff revision 10)
>      }
>      return null;
>    },
>  
> +  setProfileAutoCompleteResult(result) {
> +    this._profileAutoCompleteResult = result;

This should have a name to indicate that it's the last/cached result.
e.g. `this._lastAutoCompleteResult`

::: browser/extensions/formautofill/content/FormAutofillContent.js:417
(Diff revision 10)
> +      case "autocomplete-will-enter-text": {
> +        let selectedIndex = sendSyncMessage("FormAutoComplete:GetSelectedIndex", {});
> +        if (selectedIndex.length == 1 && Number.isInteger(selectedIndex[0])) {
> +          selectedIndex = selectedIndex[0];
> +        } else {
> +          selectedIndex = -1;

This implementation is large enough that it should be in its own method.

::: browser/extensions/formautofill/content/FormAutofillContent.js:418
(Diff revision 10)
> +  },
> +
> +  observe(subject, topic, data) {
> +    switch (topic) {
> +      case "autocomplete-will-enter-text": {
> +        let selectedIndex = sendSyncMessage("FormAutoComplete:GetSelectedIndex", {});

Nit: rename this since it's not the selectedIndex yet, it contains it.

::: browser/extensions/formautofill/content/FormAutofillContent.js:419
(Diff revision 10)
> +        if (selectedIndex.length == 1 && Number.isInteger(selectedIndex[0])) {
> +          selectedIndex = selectedIndex[0];
> +        } else {
> +          selectedIndex = -1;
> +        }

Rewrite this to return in the unwanted cases instead. 
```js
if (selectedIndexResult.length != 1 || !Number.isInteger(selectedIndexResult[0])) {
  throw new Error("Invalid autocomplete selectedIndex");
  return;
}
let selectedIndex = selectedIndexResult[0];
```

::: browser/extensions/formautofill/content/FormAutofillContent.js:433
(Diff revision 10)
> +        let formDetails = this.getFormDetails(formFillController.focusedInput);
> +        for (let inputInfo of formDetails) {
> +          if (inputInfo.element === formFillController.focusedInput) {

I think we should be checking if the observer notification is relevant to the frames content script as the first thing before we do too much work like sending a sync message. I think with your patch every tab will send a sync message instead of just the relevant tab's.

::: browser/extensions/formautofill/content/FormAutofillContent.js:435
(Diff revision 10)
> +          if (inputInfo.element === formFillController.focusedInput) {
> +            continue;

This seems wrong to me. I think it's the opppsite.

::: browser/extensions/formautofill/content/FormAutofillContent.js:440
(Diff revision 10)
> +          if (inputInfo.element === formFillController.focusedInput) {
> +            continue;
> +          }
> +          let value = profile[inputInfo.fieldName];
> +          if (value) {
> +            inputInfo.element.value = value;

This is incorrect. We need to use setUserInput to fire proper events. We should have a test for the events.
Attachment #8826030 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 47

6 months ago
mozreview-review-reply
Comment on attachment 8826030 [details]
Bug 1300989 - Part 1: Implement the profile filling feature when a user selects one.;

https://reviewboard.mozilla.org/r/104082/#review111896

> Do we need to null the cached results in this case?

I add a comment here to make sure that this should be null while the cached code implementing.

> I think this is going to cause errors trying to send elements across the process boundary, at least that's why this was added initially.

Another function `getSerializedInputDetails` is added for returning serialized input details.

> I think we should be checking if the observer notification is relevant to the frames content script as the first thing before we do too much work like sending a sync message. I think with your patch every tab will send a sync message instead of just the relevant tab's.

The null checking of formDetails is moved to the top of `_fillInFields` function, and it can prevent sending a sync message if the focused input is not in this frame.

> This seems wrong to me. I think it's the opppsite.

Since the focused one will be handled in [1], it can be skipped to prevent filling value twice.

[1] http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1551

> This is incorrect. We need to use setUserInput to fire proper events. We should have a test for the events.

Does this test implement in mochitest rather than xpc-shell test?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 51

6 months ago
Push another changes for adding the xpc-shell test.
(Assignee)

Updated

6 months ago
Depends on: 1333682
Comment on attachment 8826030 [details]
Bug 1300989 - Part 1: Implement the profile filling feature when a user selects one.;

https://reviewboard.mozilla.org/r/104082/#review113306

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:53
(Diff revision 12)
>    // The autocomplete attribute of the focused input field
> -  _fieldName: "",
> +  _focusedFieldName: "",
> +
> +  // The autocomplete attributes of all input fields in the form.
> +  _allFieldNames: null,

Technically these two comments shouldn't mention "attributes" since the field name may come from heuristics, not from the attribute.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:62
(Diff revision 12)
> +  // The array contains the object of primary and secondary labels for each
> +  // profiles.
> +  _popupLabels: null,

Nit: The prefix: "The array contains the " is redundant and can be removed.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:79
(Diff revision 12)
>      if (index < 0 || index >= this._matchingProfiles.length) {
>        throw Components.Exception("Index out of range.", Cr.NS_ERROR_ILLEGAL_VALUE);
>      }
>    },
>  
> +  _getSecondaryLabel(focusedFieldName, allFieldNames, profile) {

Can you add a JSDoc comment to this method please?

::: browser/extensions/formautofill/content/FormAutofillContent.js:246
(Diff revision 12)
> +        // TODO: we should clear the cache of inputDetails and formsDetails
> +        // while implementing the cache codes.
>          return;

I was talking about the `_lastAutoCompleteResult` cache… don't we need to clear it here?

::: browser/extensions/formautofill/content/FormAutofillContent.js:300
(Diff revision 12)
>    getInputDetails() {
>      // TODO: Maybe we'll need to wait for cache ready if detail is empty.
> -    return FormAutofillContent.getInputDetails(formFillController.focusedInput);
> +    return FormAutofillContent.getSerializedInputDetails(formFillController.focusedInput);
>    },

I honestly still find it confusing with two `getInputDetails` and `getFormDetails` methods. I think we should talk about removing the two here.

::: browser/extensions/formautofill/content/FormAutofillContent.js:302
(Diff revision 12)
>     * @returns {Object}
>     *          Target input's information that cached in FormAutofillContent.
>     */
>    getInputDetails() {
>      // TODO: Maybe we'll need to wait for cache ready if detail is empty.
> -    return FormAutofillContent.getInputDetails(formFillController.focusedInput);
> +    return FormAutofillContent.getSerializedInputDetails(formFillController.focusedInput);

Did you consider adding a second optional argument to getInputDetails to decide whether to serialize?

::: browser/extensions/formautofill/content/FormAutofillContent.js:316
(Diff revision 12)
> +  getAllFieldNames() {
> +    return FormAutofillContent.getAllFieldNames(formFillController.focusedInput);
> +  },

Do we really need this stub?

::: browser/extensions/formautofill/content/FormAutofillContent.js:368
(Diff revision 12)
>          ProfileAutocomplete.ensureUnregistered();
>        }
>      });
>      sendAsyncMessage("FormAutofill:getEnabledStatus");
> +
> +    Services.obs.addObserver(this, "autocomplete-will-enter-text", false);

Why not put this on the `ProfileAutocomplete` object?
`_fillInFields` could be renamed to `_fillFromAutocompleteRow` or something which ends up using FormAutofillHandler.autofillFormFields

::: browser/extensions/formautofill/content/FormAutofillContent.js:454
(Diff revision 12)
> +  _fillInFields() {
> +    let formDetails = this.getFormDetails(formFillController.focusedInput);
> +    if (!formDetails) {

To make this more unit-testable I think it should take an argument of form details and the focused input.

::: browser/extensions/formautofill/content/FormAutofillContent.js:481
(Diff revision 12)
> +      let value = profile[inputInfo.fieldName];
> +      if (value) {
> +        inputInfo.element.setUserInput(value);

This is actually already implemented as `FormAutofillHandler.autofillFormFields` and I think the handler is a better place for filling.
Attachment #8826030 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 56

6 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65c9e514c27107208d95211c73c574a58d0a8452
Comment on attachment 8826030 [details]
Bug 1300989 - Part 1: Implement the profile filling feature when a user selects one.;

https://reviewboard.mozilla.org/r/104082/#review113688

::: browser/extensions/formautofill/FormAutofillContent.jsm:146
(Diff revision 13)
>  let ProfileAutocomplete = {
> +  _lastAutoCompleteResult: null,
>    _registered: false,

You should define that you're implementing the nsIObserver interface with `QueryInterface:…,`

::: browser/extensions/formautofill/FormAutofillContent.jsm:170
(Diff revision 13)
>      this._registered = false;
> +

Nit: also clear `_lastAutoCompleteResult`

::: browser/extensions/formautofill/FormAutofillContent.jsm:181
(Diff revision 13)
> +      case "autocomplete-will-enter-text": {
> +        if (!formFillController.focusedInput) {
> +          break;

I thought you added a comment here to explain when it can be null.

::: browser/extensions/formautofill/FormAutofillContent.jsm:192
(Diff revision 13)
> +    return contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                         .getInterface(Ci.nsIDocShell)
> +                         .QueryInterface(Ci.nsIInterfaceRequestor)
> +                         .getInterface(Ci.nsIContentFrameMessageManager);

Nit: align the periods.

::: browser/extensions/formautofill/FormAutofillContent.jsm
(Diff revision 13)
> -  _serializeInfo(detail) {
> -    let info = Object.assign({}, detail);

Did you fix the CPOW issue with GetProfiles?
Attachment #8826030 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8835924 [details]
Bug 1300989 - Part 2: Use WeakMap to manage the form details.;

https://reviewboard.mozilla.org/r/111476/#review113690

::: browser/extensions/formautofill/FormAutofillContent.jsm:242
(Diff revision 4)
>   * Handles content's interactions for the process.
>   *
>   * NOTE: Declares it by "var" to make it accessible in unit tests.
>   */
>  var FormAutofillContent = {
> +  _formsDetails: new WeakMap(),

There should be a JSDoc comment here explaining they keys and values.
Comment on attachment 8835925 [details]
Bug 1300989 - Part 3: xpc-shell test.;

https://reviewboard.mozilla.org/r/111478/#review113692

Please file a follow-up to work on soon to test actual filling including the DOM events from setUserInput I mentioned before. We should test the events on the focused input and the others.
Attachment #8835925 - Flags: review?(MattN+bmo) → review+
I will address the comments and land it now.

Comment 61

6 months ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d561f7514acd
Part 1: Fill the autofill profile that the user selects from autocomplete; r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/251bfde7c7b5
Part 2: Use a WeakMap to store form autofill details.; r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a7e67d37100
Part 3: Update xpc-shell tests; r=MattN

Comment 62

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d561f7514acd
https://hg.mozilla.org/mozilla-central/rev/251bfde7c7b5
https://hg.mozilla.org/mozilla-central/rev/7a7e67d37100
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Updated

6 months ago
Blocks: 1339721
(Assignee)

Updated

6 months ago
Blocks: 1339727
(Assignee)

Updated

6 months ago
Blocks: 1339740
(Assignee)

Updated

6 months ago
Blocks: 1340104

Updated

2 months ago
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.