Create DOM interfaces and stub for the requestAutocomplete method and events

VERIFIED FIXED in mozilla32

Status

()

defect
VERIFIED FIXED
5 years ago
4 months ago

People

(Reporter: Paolo, Assigned: bnicholson)

Tracking

(Blocks 1 bug, {dev-doc-needed})

Trunk
mozilla32
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: p=2 s=33.1 [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

The actual implementation of the requestAutocomplete feature will reside in the Toolkit folder, near to the existing Form Manager code. This bug is about creating the DOM-accessible interfaces and a related test.

We'll figure out the exact folder structure later (bug 1020428), for the moment we just need to add a JavaScript component in the Form Manager that contains a stub implementation.

The DOM method on the Form object will call into the JavaScript component through a simple internal XPCOM interface. The stub implementation there can just raise the DOM event that tells that the feature is disabled.

We'll implement the test suite for rAc in the Toolkit folder as mochitest-chrome, as a start we can add a test there that calls the requestAutocomplete method on a form and waits for the resulting "disabled" DOM event.

The feature needs to be enabled by an about:config preference, that will be enabled by the test suite as needed. We may need a very basic documentation of the method's existence and indicate which preference is needed to activate the feature.
How is this different to Bug 939351?
Nm, just saw the comments in Bug 939351.
By the way, I've seen two preference names around in different patches:
* dom.requestAutocomplete.enabled
* dom.forms.autocomplete.experimental

Are they different on purpose to distinguish the "autocomplete" attribute from the "requestAutocomplete" method? My take is that we could use one of them for all our experimentation needs, but correct me if I'm wrong.
Flags: needinfo?(MattN+bmo)
Flags: firefox-backlog+
Just the interfaces and stub. Still needs a test, which I'll post separately.
Assignee: nobody → bnicholson
Status: NEW → ASSIGNED
Attachment #8434335 - Flags: review?(bugs)
(In reply to :Paolo Amadini from comment #3)
> By the way, I've seen two preference names around in different patches:
> * dom.requestAutocomplete.enabled
> * dom.forms.autocomplete.experimental
> 
> Are they different on purpose to distinguish the "autocomplete" attribute
> from the "requestAutocomplete" method?

Yes, we want to use the new @autocomplete values for form and password manager possibly before rAc uses them. I'd be fine with renaming them to be in the same "namespace".
Flags: needinfo?(MattN+bmo)
Comment on attachment 8434335 [details] [diff] [review]
Create DOM interfaces and stub for requestAutocomplete

>+HTMLFormElement::RequestAutocomplete()
>+{
>+  nsCOMPtr<nsIAutofillController> controller(do_GetService("@mozilla.org/autofill-controller;1"));
>+  if (controller) {
>+    controller->RequestAutocomplete(this);
Given the current spec, I think you actually need to pass window object here too,
and then in the nsIAutofillController implementation use that to create the event.
The issue is that .defaultView returns null for data documents, 
so the js code would throw an exception for
document.implementation.createHTMLDocument().createElement("form").requestAutocomplete();

The right window object would be
bool dummy;
nsCOMPtr<nsIDOMWindow> win = do_QueryInterface(OwnerDoc()->GetScriptHandlingObject(dummy));
and then check if win is null (and if so, return early I guess).

>+[scriptable, uuid(cbf47f9d-a13d-4fad-8221-8964086b1b8a)]
>+interface nsIAutofillController : nsISupports
>+{
>+  void requestAutocomplete(in nsIDOMHTMLFormElement form);
So this would take another param, in nsIDOMWindow window, and 
js implementation of this interface would use that window to create events.

With those changes, r+
Attachment #8434335 - Flags: review?(bugs) → review+
Updated to address review comments.
Attachment #8434335 - Attachment is obsolete: true
Attachment #8435347 - Flags: review?(bugs)
Comment on attachment 8435347 [details] [diff] [review]
Create DOM interfaces and stub for requestAutocomplete, v2

Review of attachment 8435347 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/HTMLFormElement.webidl
@@ +45,5 @@
>    void submit();
>    void reset();
>    boolean checkValidity();
> +
> +  [Pref="dom.requestAutocomplete.enabled"]

As I mentioned in person, switch this to dom.forms.requestAutocomplete to match the style of the other form perfs and add it to all.js with the value false.
Updated again with files moved to toolkit/components/formautofill. Also renamed pref to "dom.forms.requestAutocomplete".
Attachment #8435347 - Attachment is obsolete: true
Attachment #8435347 - Flags: review?(bugs)
Attachment #8435399 - Flags: review?(bugs)
Comment on attachment 8435399 [details] [diff] [review]
Create DOM interfaces and stub for requestAutocomplete, v3

Review of attachment 8435399 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the package-manifest.in changes.

::: browser/installer/package-manifest.in
@@ +453,5 @@
>  @BINPATH@/components/nsFormAutoComplete.js
>  @BINPATH@/components/nsFormHistory.js
>  @BINPATH@/components/FormHistoryStartup.js
>  @BINPATH@/components/nsInputListAutoComplete.js
> +@BINPATH@/components/formautofill.manifest

Aren't you missing AutofillController.js?

::: mobile/android/installer/package-manifest.in
@@ +379,5 @@
>  @BINPATH@/components/nsFormHistory.js
>  @BINPATH@/components/FormHistoryStartup.js
>  @BINPATH@/components/nsInputListAutoComplete.js
>  @BINPATH@/components/AutofillController.js
> +@BINPATH@/components/formautofill.manifest

ditto
Attachment #8435399 - Flags: review+
(In reply to Olli Pettay [:smaug] from comment #6)
> The issue is that .defaultView returns null for data documents, 
> so the js code would throw an exception for
> document.implementation.createHTMLDocument().createElement("form").
> requestAutocomplete();

We'll need to add a test for this straight away :-)
Attachment #8435399 - Flags: review?(bugs) → review+
Note that we can't add any useful tests yet since calling requestAutocomplete will always result in a disabled event. Testing for these special cases (like comment 11) can be addressed elsewhere, probably bug 1020440.
(In reply to Brian Nicholson (:bnicholson) from comment #13)
> bug 1020440.

Clipboard fail. I meant bug 1020616.
https://hg.mozilla.org/mozilla-central/rev/568758b30cb2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Blocks: 1023484
Whiteboard: p=2 → p=2 s=33.1 [qa?]
What is the user facing impact of this?
(In reply to [:tracy] Tracy Walker - QA Mentor from comment #16)
> What is the user facing impact of this?

No user-facing impact from this specific bug.

There is a developer-facing side, that is the new requestAutocomplete method on the Form object, but this is still unimplemented (it generates the "disabled" event).
Whiteboard: p=2 s=33.1 [qa?] → p=2 s=33.1 [qa-]
Status: RESOLVED → VERIFIED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.