Closed Bug 1340468 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Form Manager, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: lchang, Assigned: ralin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M2])

Attachments

(1 file)

When implementing previewing feature, the formautofill add-on needs to know which profile is being hovered.
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: nobody → ralin
Status: NEW → ASSIGNED
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 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)
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?
Depends on: 1338482
> This part is for the missing method from Bug 1340468.

correction: s/Bug 1340468/Bug 1326138
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 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.
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.
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/
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 on attachment 8840798 [details]
Bug 1340468 - Notify formautofill add-on of which item is being selected.

https://reviewboard.mozilla.org/r/115234/#review127480
Attachment #8840798 - Flags: review?(MattN+bmo) → review+
Thank you MattN :)

Luke and I just had a discussion about simulating preview by background color and placeholder here.
> .....
> fieldDetail.element.style.backgroundColor = value ? "rgba(36,138,235,.1)" : "transparent";
> fieldDetail.element.placeholder = value;
> .....

Since we think it's not necessary to do this now, so would you mind that I comment out these codes? (for who want to try mock preview, they can remove comments by themselves...)

I think we can wait for DOM and highlight parts implemented, and integrate with real API in another bug then.
Flags: needinfo?(MattN+bmo)
Blocks: 1300996
(In reply to Ray Lin[:ralin] from comment #21)
> Thank you MattN :)
> 
> Luke and I just had a discussion about simulating preview by background
> color and placeholder here.
> > .....
> > fieldDetail.element.style.backgroundColor = value ? "rgba(36,138,235,.1)" : "transparent";
> > fieldDetail.element.placeholder = value;
> > .....
> 
> Since we think it's not necessary to do this now, so would you mind that I
> comment out these codes? (for who want to try mock preview, they can remove
> comments by themselves...)
> 
> I think we can wait for DOM and highlight parts implemented, and integrate
> with real API in another bug then.

That's fine with me. I would also be fine with deleting those lines before landing.
Flags: needinfo?(MattN+bmo)
Thanks.
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c7fc679c1c9e
Notify formautofill add-on of which item is being selected. r=MattN
Keywords: checkin-needed
sorry had to back this out for eslint failure like https://treeherder.mozilla.org/logviewer.html#?job_id=87508121&repo=autoland&lineNumber=4405
Flags: needinfo?(ralin)
(In reply to Iris Hsiao [:ihsiao] from comment #26)
> sorry had to back this out for eslint failure like
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=87508121&repo=autoland&lineNumber=4405

Thank you Iris,

patch updated, try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46be92c7b2ce4399c3eb9af7998e87533cb864b7&selectedJob=87517388

eslint looks good now.
Flags: needinfo?(ralin)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dbe1a6c3780a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/a66571f6f5b0
Backed out changeset c7fc679c1c9e for eslint failure
Depends on: 1358041
You need to log in before you can comment on or make changes to this bug.