1020440 - Create DOM interfaces and stub for the requestAutocomplete method and events

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Paolo Amadini]

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

How is this different to Bug 939351?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Nm, just saw the comments in Bug 939351.

Comment 3

[:Paolo Amadini]

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.

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Create DOM interfaces and stub for requestAutocomplete

Just the interfaces and stub. Still needs a test, which I'll post separately.

Comment 5

[Matthew N. [:MattN]]

(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".

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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+

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: Create DOM interfaces and stub for requestAutocomplete, v2

Updated to address review comments.

Comment 8

[Matthew N. [:MattN]]

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.

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: Create DOM interfaces and stub for requestAutocomplete, v3

Updated again with files moved to toolkit/components/formautofill. Also renamed pref to "dom.forms.requestAutocomplete".

Comment 10

[Matthew N. [:MattN]]

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

Comment 11

[:Paolo Amadini]

(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 :-)

Comment 12

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/568758b30cb2

Comment 13

[Brian Nicholson (:bnicholson)]

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.

Comment 14

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #13)
> bug 1020440.

Clipboard fail. I meant bug 1020616.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/568758b30cb2

Comment 16

[Tracy Walker [:tracy]]

What is the user facing impact of this?

Comment 17

[:Paolo Amadini]

(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).