Closed Bug 1020865 Opened 10 years ago Closed 10 years ago

Implement the dialog displayed upon form.requestAutocomplete()

Categories

(Toolkit :: Form Manager, defect)

Product:
Toolkit
Component:
Form Manager
Type:
defect
Priority:
Not set
Severity:
normal
Points:
8

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla33
Iteration:
33.3

People

(Reporter: MattN, Assigned: Paolo)

References

Details

Attachments

(1 file, 6 obsolete files)

Patch for checkin
(deleted), patch
Details | Diff | Splinter Review
The UI allows the user to choose an existing autofill profile or create a new one.
Flags: firefox-backlog+
Depends on: 1020868
Alias: rAc-desktop-chooseUI
Whiteboard: p=8 → p=8 [qa?]
Marking qa+ because this can be tested manually by enabling the required preferences in about:config.
Whiteboard: p=8 [qa?] → p=8 [qa+]
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Whiteboard: p=8 [qa+] → p=8 s=33.1 [qa+]
Iteration: --- → 33.2
Points: --- → 8
QA Whiteboard: [qa+]
Whiteboard: p=8 s=33.1 [qa+]
Attached patch Initial file layout (obsolete) (deleted) — — Splinter Review
This patch is the beginning of the work here, I'm interested in feedback about the general approach and file layout. This version only displays a demo window, but can already be invoked from a demo site. Since we're placing the implementation in the Toolkit folder, I think the Desktop UI could be the default in the integration module, unless overridden. I've created this minimal test file locally to invoke the UI, though any requestAutocomplete demo site should work once the preferences are toggled: <!DOCTYPE HTML><html><head><meta charset="utf-8"></head><body> <form id="form"> <input type="button" value="rAc" onclick="document.getElementById('form').requestAutocomplete();"> </form> </body></html>
Attachment #8445316 - Flags: feedback?(MattN+bmo)
Comment on attachment 8445316 [details] [diff] [review] Initial file layout Review of attachment 8445316 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/formautofill/FormAutofillIntegration.jsm @@ +19,5 @@ > > XPCOMUtils.defineLazyModuleGetter(this, "Promise", > "resource://gre/modules/Promise.jsm"); > +XPCOMUtils.defineLazyModuleGetter(this, "RequestAutocompleteDesktopUI", > + "resource://gre/modules/RequestAutocompleteDesktopUI.jsm"); I wonder if some of our indirection leading to forking is really necessary. If we used a module name without "desktop" then other platforms could just override the JSM path in the jar and then they may not need to fork this file. ::: toolkit/components/formautofill/desktopui/RequestAutocompleteDesktopUI.jsm @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public I don't think we should mention desktop in the module or file name since ideally this could work for any non-native implementation (maybe with a few ifdefs). If we really just want to make something that only works only on desktop, the code should probably move to browser/. ::: toolkit/components/formautofill/desktopui/requestautocomplete.js @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public The standard directory name would be "content" instead of desktopui. ::: toolkit/components/formautofill/desktopui/requestautocomplete.xhtml @@ +14,5 @@ > + > +<html xmlns="http://www.w3.org/1999/xhtml"> > + <head> > + <title>&requestautocomplete.title.demo;</title> > + <link rel="stylesheet" type="text/css" type="text/css" is the default so it's not required ::: toolkit/locales/en-US/chrome/formautofill/requestautocomplete.dtd @@ +4,5 @@ > + > +<!-- LOCALIZATION NOTE (requestautocomplete.title): > + This is a demo label and does not need to be translated. > + --> > +<!ENTITY requestautocomplete.title.demo "requestAutocomplete demo window"> Perhaps we should just omit the entity and reference rather that using a placeholder? ::: toolkit/themes/shared/requestautocomplete.css @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public Nit: use camelCase for all of the new file names. I wonder if we should create a formautofill directory under shared but we can do that later with an hg move if we need.
Attachment #8445316 - Flags: feedback?(MattN+bmo) → feedback+
(In reply to Matthew N. [:MattN] from comment #3) > I wonder if some of our indirection leading to forking is really necessary. > If we used a module name without "desktop" then other platforms could just > override the JSM path in the jar and then they may not need to fork this > file. I'm not sure there's a lot of value in not forking this file, and I've not seen this technique used elsewhere, but it might be an efficient way of doing the base override once. > ::: > toolkit/components/formautofill/desktopui/RequestAutocompleteDesktopUI.jsm > @@ +1,1 @@ > > +/* This Source Code Form is subject to the terms of the Mozilla Public > > I don't think we should mention desktop in the module or file name since > ideally this could work for any non-native implementation (maybe with a few > ifdefs). If we really just want to make something that only works only on > desktop, the code should probably move to browser/. Android might use this UI temporarily, but in the end the Desktop entry point would need to move to "browser" when we support displaying the UI inside a panel. For now we can keep it here and open in a separate window so that Android can use it to experiment, until they have the native UI ready. > ::: toolkit/components/formautofill/desktopui/requestautocomplete.js > @@ +1,1 @@ > > +/* This Source Code Form is subject to the terms of the Mozilla Public > > The standard directory name would be "content" instead of desktopui. This convention is generally used for separating the files by type, for example "src" for JSMs, "content" for JS and XUL, and "public" for IDL. Now that we have simple and flexible moz.build files, maybe we could start separating files based on how they work together instead. Putting the JSM that handles the UI in a dedicated "ui" folder, near the dialog that it controls, seems clearer than separating it into a "src" folder. I think the name "content" suggests we're using the the old layout instead. What do you think?
Attached patch Updated layout (obsolete) (deleted) — — Splinter Review
In this second round, I renamed "desktopui" to "defaultui".
Attachment #8445316 - Attachment is obsolete: true
Attached patch The first test (obsolete) (deleted) — — Splinter Review
An example of a browser-chrome test for the user interface, still unfinished.
Attachment #8447224 - Flags: feedback?(MattN+bmo)
Comment on attachment 8447224 [details] [diff] [review] The first test Review of attachment 8447224 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/formautofill/defaultui/RequestAutocompleteDefaultUI.jsm @@ +39,5 @@ > + > + // Wrap the callback function so that it survives XPCOM. > + let args = { resolveFn: resolveFn }; > + args.wrappedJSObject = args; > + Do you know of better ways of doing this? This was the simplest I could find in the MDN examples and looking at our mozilla-central code.
Attached patch The first test (obsolete) (deleted) — — Splinter Review
Forgot to "hg add" the test file!
Attachment #8447224 - Attachment is obsolete: true
Attachment #8447224 - Flags: feedback?(MattN+bmo)
Attachment #8447916 - Flags: feedback?(MattN+bmo)
(In reply to :Paolo Amadini from comment #4) > (In reply to Matthew N. [:MattN] from comment #3) > > I wonder if some of our indirection leading to forking is really necessary. > > If we used a module name without "desktop" then other platforms could just > > override the JSM path in the jar and then they may not need to fork this > > file. > > I'm not sure there's a lot of value in not forking this file, and I've not > seen this technique used elsewhere, but it might be an efficient way of > doing the base override once. I believe the technique I described is used for network error pages on all platforms. > > toolkit/components/formautofill/desktopui/RequestAutocompleteDesktopUI.jsm > > @@ +1,1 @@ > > > +/* This Source Code Form is subject to the terms of the Mozilla Public > > > > I don't think we should mention desktop in the module or file name since > > ideally this could work for any non-native implementation (maybe with a few > > ifdefs). If we really just want to make something that only works only on > > desktop, the code should probably move to browser/. > > Android might use this UI temporarily, but in the end the Desktop entry > point would need to move to "browser" when we support displaying the UI > inside a panel. I don't think the panel UI necessarily requires a move to browser since I was thinking that would be implemented as a toolkit widget. > For now we can keep it here and open in a separate window so > that Android can use it to experiment, until they have the native UI ready. Okay, but in the meantime I'd rather keep a generic name until we know how things will flush out. > > ::: toolkit/components/formautofill/desktopui/requestautocomplete.js > > @@ +1,1 @@ > > > +/* This Source Code Form is subject to the terms of the Mozilla Public > > > > The standard directory name would be "content" instead of desktopui. > > This convention is generally used for separating the files by type, for > example "src" for JSMs, "content" for JS and XUL, and "public" for IDL. I normally see it more as front-end (content/) vs. back-end (src/ although we normally flatten that into the parent dir. now). > Now that we have simple and flexible moz.build files, maybe we could start > separating files based on how they work together instead. Putting the JSM > that handles the UI in a dedicated "ui" folder, near the dialog that it > controls, seems clearer than separating it into a "src" folder. I think the > name "content" suggests we're using the the old layout instead. > > What do you think? In general I prefer to follow existing conventions unless there is a very compelling reason not to since it makes it easier for other Mozilla developers to dig into new code. With the above distinction, I would be fine with having the front-end JSM in the content directory.
Comment on attachment 8447916 [details] [diff] [review] The first test Review of attachment 8447916 [details] [diff] [review]: ----------------------------------------------------------------- Test looks good so far. ::: toolkit/components/formautofill/defaultui/RequestAutocompleteDefaultUI.jsm @@ +38,5 @@ > + let uiPromise = new Promise(resolve => resolveFn = resolve); > + > + // Wrap the callback function so that it survives XPCOM. > + let args = { resolveFn: resolveFn }; > + args.wrappedJSObject = args; Can you use window.opener with ww.openWindow? Perhaps not. This isn't so bad. Otherwise we could give this method access to the chrome window and use window.(openDialog|open) which I believe avoids the XPCOM problem.
Attachment #8447916 - Flags: feedback?(MattN+bmo) → feedback+
Attached patch The patch (obsolete) (deleted) — — Splinter Review
This includes the updates to the requestAutocomplete method from bug 1020607, and adds the final mock UI object used by xpcshell and mochitest-chrome tests. The "disabled" test is now replaced by the "cancel" test, since we have not implemented any case where we are expected to return "disabled", and for unexpected exceptions we should return the empty string as a reason, according to the specification. I filed bug 1032762 to take care of the actual preference to block the feature globally, which is different from the existing development preferences, and may depend on the chosen preferences UI.
Attachment #8446548 - Attachment is obsolete: true
Attachment #8447916 - Attachment is obsolete: true
Attachment #8448666 - Flags: review?(MattN+bmo)
Alias: rAc-desktop-chooseUI
Attached patch The current patch (obsolete) (deleted) — — Splinter Review
Fixed one nit, the "form" property was unused. I'm attaching a new patch, since I'll rebase the others on top of this one.
Attachment #8448666 - Attachment is obsolete: true
Attachment #8448666 - Flags: review?(MattN+bmo)
Attachment #8448671 - Flags: review?(MattN+bmo)
Comment on attachment 8448671 [details] [diff] [review] The current patch Review of attachment 8448671 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to :Paolo Amadini from comment #11) > The "disabled" test is now replaced by the "cancel" test, since we have not > implemented any case where we are expected to return "disabled", and for > unexpected exceptions we should return the empty string as a reason, > according to the specification. We were returning "disabled" to deal with the following case: "the user agent does not support this form's fields" since we don't support any fields yet. ::: toolkit/components/formautofill/content/RequestAutocompleteDefaultUI.jsm @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/* > + * Handles the requestAutocomplete user interface on Desktop. s/on Desktop// I still think this should be "RequestAutocompleteUI.jsm" (removing "default"). The same goes for the other files/objects.
Attachment #8448671 - Flags: review?(MattN+bmo) → review+
(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #13) > We were returning "disabled" to deal with the following case: "the user > agent does not support this form's fields" since we don't support any fields > yet. We'll actually support a successful autocomplete with various fields as soon as the next patches land, so this is no longer a valid case, and I believe we can add back a test for this case when we implement the actual restrictions later.
Attached patch Patch for checkin (deleted) — — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=ca7dfd843d21
Attachment #8448671 - Attachment is obsolete: true
Iteration: 33.2 → 33.3
This initially failed on tryserver because of a packaging issue (uppercase/lowercase mismatch) that was easy to solve. However, there is another packaging problem that went unnoticed before, and probably already occurs in Nightly: the nsIFormAutofillContentService interface is not available in packaged builds (./mach package), while it works in a local build (./mach run). It can easily be verified using Components.interfaces in the Browser Console. Everything seems correct to me, the IDL file is declared in toolkit/components/formautofill/moz.build, the IID is unique, and the IDL file looks just like similar files in other Toolkit components. What could be wrong? I don't think the interface needs to be declared in a manifest. XPIDL_MODULE is "toolkit_formautofill", the underscore is also used elsewhere so it shouldn't be a problem.
Flags: needinfo?(gps)
The xpt is not listed in package-manifest.in
Flags: needinfo?(gps)
(In reply to Benjamin Smedberg [:bsmedberg] from comment #17) > The xpt is not listed in package-manifest.in Ah, so there was something more that needed to go into the manifest! The next thing I'm going to say is, obviously, that I look forward to package manifests being generated automatically from moz.build files :-)
(In reply to :Paolo Amadini from comment #18) > The next thing I'm going to say is, obviously, that I look forward to > package manifests being generated automatically from moz.build files :-) Eliminating package manifests (or at least starting to auto-generate part of them) was enabled by moz.build files. It's on the long term goals list. But there is not currently any resourcing available to work on it :/
There is still an issue with the browser-chrome test on Windows 7 for which the size of the window hosting the XHTML document is not computed correctly: http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/05a0342d35ce1fa8dc50f36ba2836c704a283a6b9009841f7885c3939f1761826628ffa2fe13bc1b72f4f031d47294dfbfa4ef27b430157bf00d06a0012def6f The OK and Cancel buttons are invisible. I added a sizeToContent() call but it did not help. I'm trying to do a local Windows 7 build, but it's been some time since the last one and prerequisites have since been updated, so this is taking longer than expected. Suggestions on how to solve the window size issue are welcome!
Once the build issues were solved, it turned out that the problem did not occur locally, even using a similar Windows theme and screen resolution as the test machine, and various system text size settings. It occurred to me in the past that size calculations of windows containing XUL description elements were off by one pixel, so it's possible that something similar is happening with the XHTML document. For the moment, I worked around the issue by shortening the test label. Since this interface will be moved to a panel in the browser UI, this bug will not affect the final version, but we should be aware that it might affect changes to the test interface.
https://hg.mozilla.org/mozilla-central/rev/a599e3c677bf
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Hi Florin, can a QA contact be assigned for verification of this bug.
Flags: needinfo?(florin.mezei)
Flags: needinfo?(florin.mezei)
QA Contact: andrei.vaida
Hi Andrei, following up to see if this bug can be verified by Monday which is the end of the Desktop iteration.
Flags: needinfo?(andrei.vaida)
Andrei, the feature needs both these about:config preferences to be enabled: dom.forms.autocomplete.experimental dom.forms.requestAutocomplete You can then verify that a stub dialog is displayed from a requestAutocomplete test page. The dialog is just a demo, the only thing that should be verified is that the dialog is shown.
(In reply to Marco Mucci [:MarcoM] from comment #26) > Hi Andrei, following up to see if this bug can be verified by Monday which > is the end of the Desktop iteration. Hi Marco, I will follow up with my test results for this fix later today. (In reply to :Paolo Amadini from comment #27) > Andrei, the feature needs both these about:config preferences to be enabled: > > dom.forms.autocomplete.experimental > dom.forms.requestAutocomplete > > You can then verify that a stub dialog is displayed from a > requestAutocomplete test page. The dialog is just a demo, the only thing > that should be verified is that the dialog is shown. Thank you for the tips, Paolo!
Flags: needinfo?(andrei.vaida)
This issue is verified fixed on Nightly 33.0a1 (2014-07-16) using the test case from Comment 2 along with this demo [1], under: Windows 7 64-bit, Mac OS X 10.9.4 and Ubuntu 14.04 LTS 32-bit. However, there are a few minor things I'd like to mention: * Windows - while in full screen, the browser's title bar flickers when the stub dialog is launched * Mac OS X - the stub dialog box is always displayed at the top, left-hand side of the screen * Ubuntu - the stub dialog box is always displayed at the bottom, left-hand side of the screen; in full screen though, it's displayed at the top side of the screen, positioned somewhere at the middle [1] http://goo.gl/WlNFAh
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: