Create DOM interfaces and stub for the requestAutocomplete method and events

VERIFIED FIXED in mozilla32

Status

()

VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: Paolo, Assigned: bnicholson)

Tracking

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

Trunk
mozilla32
dev-doc-needed
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)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 3

5 years ago
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)
(Reporter)

Updated

5 years ago
Flags: firefox-backlog+
(Assignee)

Comment 4

5 years ago
Created attachment 8434335 [details] [diff] [review]
Create DOM interfaces and stub for requestAutocomplete

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+
(Assignee)

Comment 7

5 years ago
Created attachment 8435347 [details] [diff] [review]
Create DOM interfaces and stub for requestAutocomplete, v2

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

Comment 9

5 years ago
Created attachment 8435399 [details] [diff] [review]
Create DOM interfaces and stub for requestAutocomplete, v3

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+
(Reporter)

Comment 11

5 years ago
(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+
(Assignee)

Comment 13

5 years ago
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.
(Assignee)

Comment 14

5 years ago
(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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(Reporter)

Updated

5 years ago
Blocks: 1023484

Updated

5 years ago
Whiteboard: p=2 → p=2 s=33.1 [qa?]
What is the user facing impact of this?
(Reporter)

Comment 17

5 years ago
(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-]

Updated

5 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.