The default bug view has changed. See this FAQ.

Notify formautofill add-on of which item is being hovered in the suggestion dropdown

ASSIGNED
Assigned to

Status

()

Toolkit
Form Manager
P3
enhancement
ASSIGNED
a month ago
12 days ago

People

(Reporter: lchang, Assigned: ralin)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [form autofill:M2])

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

a month ago
When implementing previewing feature, the formautofill add-on needs to know which profile is being hovered.
(Assignee)

Comment 1

a month ago
discussed with Matt in person, we could override the property "selected"[0] and then dispatch a custom event once this property is set to true. In item binding, we have a reference to XULDocument, which might be the place where to expose the event.


[0] https://dxr.mozilla.org/mozilla-central/rev/d0462b0948e0b1147dcce615bddcc46379bdadb2/toolkit/content/widgets/listbox.xml#930-939
(Assignee)

Updated

a month ago
Assignee: nobody → ralin
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

25 days ago
Comment on attachment 8840798 [details]
Bug 1340468 - Notify formautofill add-on of which item is being selected.

Hi MattN,

Based on the discussion we had this week, I made some refinements. Could you give me feedback about it? Thanks.
Attachment #8840798 - Flags: feedback?(MattN+bmo)
Comment hidden (mozreview-request)
Comment on attachment 8840798 [details]
Bug 1340468 - Notify formautofill add-on of which item is being selected.

https://reviewboard.mozilla.org/r/115234/#review120276

I think it would be worth talking through some of the ideas to provide a better understanding.

::: browser/extensions/formautofill/FormAutofillContent.jsm:250
(Diff revision 3)
> +      throw new Error("Invalid autocomplete selectedIndex");
> +    }
> +
> +    let selectedIndex = selectedIndexResult[0];
> +    if (selectedIndex == -1) {
> +      // Clear exisiting preview feilds

Typo: "existing"

::: browser/extensions/formautofill/FormAutofillContent.jsm:251
(Diff revision 3)
> +    }
> +
> +    let selectedIndex = selectedIndexResult[0];
> +    if (selectedIndex == -1) {
> +      // Clear exisiting preview feilds
> +      [...doc.getElementsByTagName("form")]

We can't assume a <form> is used. Why do we have to do this for all forms? (I see now that this method isn't only to preview for a row but also used when the popup closes. I think that's not clear from the method name. Perhaps the code to clear previews shoudl be its own method and that's what popupClosed would use?). Why not only the one we're part of by using the info the handler has about this form?

::: browser/extensions/formautofill/content/FormAutofillFrameScript.js:59
(Diff revision 3)
> +      case "FormAutoComplete:SetSelectedIndex":
> +        FormAutofillContent._previewProfile(content.document);

Are you calling this for SetSeletedIndex because it's also going to be doing the styling for fields we succesfully filled? Wouldn't that already be handled via FormAutofill:PreviewProfile when the row is highlighted?

::: browser/extensions/formautofill/content/formautofill.xml:41
(Diff revision 3)
>              this, "anonid", "profile-label"
>            );
>            this._comment = document.getAnonymousElementByAttribute(
>              this, "anonid", "profile-comment"
>            );
> +          this.mm = document.ownerGlobal.gBrowser.selectedBrowser.messageManager;

This seems a bit fragile. Somewhat less fragile is to import AutoCompletePopup.jsm and use its sendMessageToBrowser method.

::: browser/extensions/formautofill/content/formautofill.xml:69
(Diff revision 3)
> +      <method name="_onChanged">
> +        <body></body>
> +      </method>

Can you add a comment about what this is for? I don't see you shadowing a method on richlistbox or listbox so I'm not sure what this is about.
Attachment #8840798 - Flags: review?(MattN+bmo)
(Assignee)

Comment 7

19 days ago
mozreview-review-reply
Comment on attachment 8840798 [details]
Bug 1340468 - Notify formautofill add-on of which item is being selected.

https://reviewboard.mozilla.org/r/115234/#review120276

Thanks for the reviewing, I should have wrote more about the ideas before asking for review. It seems that preview part is relatively simpler than clear existing preview.


There's three cases need to clear the preview
1. popup closed:
  We lost focusedInput at this moment, so it hard to tell which <form> was already styled. I think iterating all <form>s is necessary and acceptable in this case.
 
2. user clicked on one item:
  Similar to 1. But as you mentioned in the other comment, we cannot assume a <form> is used, so we might accidentally clear background color of <form> which has been successfully filled. I've discussed with Steve offline, he said a property will be added to form handler in bug 1338482 which can indicate whether form is used(filled) of not. I'll mark bug 1338482 as depends, and I think it'll be fine here then.

3. keyboard navigate to index -1:
  This is the trickiest one. Item indexed -1 is a keyboard accessible only(I guess) virtual item instead of a real binding, so there's no explicit place for it to notify as we do in @selected setter. Based on the fact that the first or the last item's @selected would be set to false while navigating to -1, I move out the code sending FormAutofill:PreviewProfile from only in the condition when @selected is set to true to the end of @selected setter (see lint 55 in browser/extensions/formautofill/content/formautofill.xml)

> We can't assume a <form> is used. Why do we have to do this for all forms? (I see now that this method isn't only to preview for a row but also used when the popup closes. I think that's not clear from the method name. Perhaps the code to clear previews shoudl be its own method and that's what popupClosed would use?). Why not only the one we're part of by using the info the handler has about this form?

Good idea to separate into two methods.

> Are you calling this for SetSeletedIndex because it's also going to be doing the styling for fields we succesfully filled? Wouldn't that already be handled via FormAutofill:PreviewProfile when the row is highlighted?

ah! This is no longer needed here. I initially intended to catch all possible change, because there are several ways to set "selectedIndex" of richlistbox.

> This seems a bit fragile. Somewhat less fragile is to import AutoCompletePopup.jsm and use its sendMessageToBrowser method.

Got it, it would be good to reuse sendMessageToBrowser.

> Can you add a comment about what this is for? I don't see you shadowing a method on richlistbox or listbox so I'm not sure what this is about.

This part is for the missing method from Bug 1340468. (https://dxr.mozilla.org/mozilla-central/rev/58753259bfeb3b818eac7870871b0aae1f8de64a/toolkit/content/widgets/autocomplete.xml#2047-2058)

Not sure if it's fine to have a quick fix here? or in another bug?
(Assignee)

Updated

19 days ago
Depends on: 1338482
(Assignee)

Comment 8

19 days ago
> This part is for the missing method from Bug 1340468.

correction: s/Bug 1340468/Bug 1326138
Comment hidden (mozreview-request)
Comment on attachment 8840798 [details]
Bug 1340468 - Notify formautofill add-on of which item is being selected.

https://reviewboard.mozilla.org/r/115234/#review122350

::: browser/extensions/formautofill/FormAutofillContent.jsm:247
(Diff revision 4)
> +    [...doc.getElementsByTagName("form")]
> +      .map(form => FormAutofillContent._formsDetails.get(form))
> +      .filter(fh => !!fh)
> +      .forEach(fh => fh.clearPreviewedFormFields());

Can we factor out the code to find "FormLikes" https://dxr.mozilla.org/mozilla-central/rev/6d38ad302429c98115c354d643e81987ecec5d3c/browser/extensions/formautofill/FormAutofillContent.jsm#324-336 and use it instead of getElementsByTagName
Comment on attachment 8840798 [details]
Bug 1340468 - Notify formautofill add-on of which item is being selected.

https://reviewboard.mozilla.org/r/115234/#review122710

Clearing review since we discussed the main issues live.
Attachment #8840798 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment on attachment 8840798 [details]
Bug 1340468 - Notify formautofill add-on of which item is being selected.

https://reviewboard.mozilla.org/r/115234/#review122770

::: browser/extensions/formautofill/FormAutofillContent.jsm:253
(Diff revision 5)
>      formHandler.autofillFormFields(profile, focusedInput);
>    },
> +
> +  _clearProfilePreview(doc) {
> +    // Clear existing preview feilds
> +    [...doc.getElementsByTagName("form")]

This should still change to use the code (factored out) from identifyFormFields.
(Assignee)

Comment 14

13 days ago
mozreview-review-reply
Comment on attachment 8840798 [details]
Bug 1340468 - Notify formautofill add-on of which item is being selected.

https://reviewboard.mozilla.org/r/115234/#review122770

> This should still change to use the code (factored out) from identifyFormFields.

oops...I think mozreivew didn't push the new one, it should have been removed :D

checking now.
Comment hidden (mozreview-request)
(Assignee)

Comment 16

13 days ago
Sorry for messing up the diff, I have no idea but the diff seems somehow mixed with others.

You might need to see whole diff instead :(
https://reviewboard.mozilla.org/r/115234/diff/6/
Comment hidden (mozreview-request)
(Assignee)

Comment 18

12 days ago
Update the status of these three cases:

1. popup closed by clicking outside popup: 
I cache last focused input in the new patch, so no need to iterate through all <form> now.
 
2. popup closed after auto-filling: 
We considered that we can fill profile and clear preview at the same time, but I found FormAutoComplete:PopupClosed is still received by framescript after auto-filling. So I think it's fine to leave autofillFormFields() as-is, and let "clearPreviewedFormFields()" handle the preview-clearing work.

3. keyboard navigate to index -1:
solved as we send FormAutofill:PreviewProfile in both "true"/"false" case of selectedIndex's setter. 


I'll rebase this patch since Bug 1338482 has just landed.
Comment hidden (mozreview-request)
You need to log in before you can comment on or make changes to this bug.