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)
Toolkit
Form Manager
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.
Assignee | ||
Comment 1•7 years 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•7 years ago
|
Assignee: nobody → ralin
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years 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 6•7 years ago
|
||
mozreview-review |
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•7 years 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 | ||
Comment 8•7 years ago
|
||
> This part is for the missing method from Bug 1340468. correction: s/Bug 1340468/Bug 1326138
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
mozreview-review |
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 11•7 years ago
|
||
mozreview-review |
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 13•7 years ago
|
||
mozreview-review |
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•7 years 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•7 years 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•7 years 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) |
Comment 20•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 21•7 years ago
|
||
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)
Comment 22•7 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Comment 25•7 years ago
|
||
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
Comment 26•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 28•7 years ago
|
||
(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
Comment 29•7 years ago
|
||
pushed: https://hg.mozilla.org/integration/autoland/rev/dbe1a6c3780a10e6a49ca8d855fc5e9a01903e9a
Keywords: checkin-needed
Comment 30•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dbe1a6c3780a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 31•7 years ago
|
||
Backout by ryanvm@gmail.com: https://hg.mozilla.org/mozilla-central/rev/a66571f6f5b0 Backed out changeset c7fc679c1c9e for eslint failure
You need to log in
before you can comment on or make changes to this bug.
Description
•