557628 - Implement autocomplete attribute for input and form elements

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

$URL is for forms, this is for input elements: http://dev.w3.org/html5/spec/forms.html#attr-input-autocomplete

Comment 1

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

So what parts of autocomplete aren't implemented currently?
LoginManager supports at least some parts of autocomplete
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/src/nsLoginManager.js#766

Comment 2

[Mounir Lamouri (:mounir)]

- autocomplete IDL attribute is not available in input elements
- autocomplete content attribute is not implemented for form elements
- autocomplete IDL attribute is not available in form elements
- input elements should behave differently depending on the form elements autocomplete attribute value
- specific UI's should be used for some input element states (not exhaustive list: color, date's, range, number ...) but those elements are not yet implemented.

It would be really easy to implement everything except the specific UI's. Maybe I could do that because if we set autocomplete='off' on an input element it will let think we are supporting this attribute but we are not completely.

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Tests v1

I did not test the UI aspects. I don't know how to do that easily.

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

I've added |mozIsAutocompletionEnabled| in nsIDOMNSHTMLInputElement IDL.
This function could be added in nsContentUtils instead.
I choose to expose it to let a js context use it. In addition, in a performance perspective, it is much more straightforward to do this check in nsHTMLInputElement because it prevents us to manipulate too many strings.
I could understand if you me to move it to nsContentUtils or somewhere else.

Comment 5

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

You can't add mozIsAutocompletionEnabled to nsContentUtils because a
caller of it isn't linked to gklayout.

Currently mozIsAutocompletionEnabled is used only by C++ callers, so
I wonder if the method could be [noscript] - at least for now.

Comment 6

[Mounir Lamouri (:mounir)]

This patch is using the macro introduced in the patch from bug 551670.

Comment 7

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

Comment on attachment 438459 [details] [diff] [review]
Patch v1


>+static const nsAttrValue::EnumTable kInputAutocompleteTable[] = {
>+  { "default", NS_FORM_AUTOCOMPLETE_DEFAULT },
>+  { "on",      NS_FORM_AUTOCOMPLETE_ON      },
>+  { "off",     NS_FORM_AUTOCOMPLETE_OFF     },
>+  { 0 }
>+};
>+
>+// Default autocomplete value is 'default'.
>+static const nsAttrValue::EnumTable* kInputDefaultAutocomplete = &kInputAutocompleteTable[0];

Is this really the right thing. I read HTML5 draft so that
if autocomplete isn't set, the idl attribute should return
the value of form elements autocomplete. The draft speaks about
on and off keywords, but default state.
"The default state indicates that the user agent is to use the autocomplete attribute on the element's form owner instead. "

And if .autocomplete always returns either on or off, 
there is no need for MozIsAutocompletionEnable

Comment 8

[Mounir Lamouri (:mounir)]

(In reply to comment #7)
> (From update of attachment 438459 [details] [diff] [review])
> 
> >+static const nsAttrValue::EnumTable kInputAutocompleteTable[] = {
> >+  { "default", NS_FORM_AUTOCOMPLETE_DEFAULT },
> >+  { "on",      NS_FORM_AUTOCOMPLETE_ON      },
> >+  { "off",     NS_FORM_AUTOCOMPLETE_OFF     },
> >+  { 0 }
> >+};
> >+
> >+// Default autocomplete value is 'default'.
> >+static const nsAttrValue::EnumTable* kInputDefaultAutocomplete = &kInputAutocompleteTable[0];
> 
> Is this really the right thing. I read HTML5 draft so that
> if autocomplete isn't set, the idl attribute should return
> the value of form elements autocomplete. The draft speaks about
> on and off keywords, but default state.
> "The default state indicates that the user agent is to use the autocomplete
> attribute on the element's form owner instead. "
> 
> And if .autocomplete always returns either on or off, 
> there is no need for MozIsAutocompletionEnable

For what I understand, the autocomplete attribute for the input element has three states (on, off and default) [1] and there is a resulting autocompletion state which can only be on or off [2].
I found this quite reasonable because if I do |foo.autocomplete='default'| then |foo.autocomplete|, I want to get 'default' not 'on' or 'off'. If I get 'on' or 'off' it would be really hard to know if the element will follow the form element autocomplete attribute value or not.

[1] "The autocomplete  attribute is an enumerated attribute. The attribute has three states. The on  keyword maps to the on state, and the off keyword maps to the off  state. The attribute may also be omitted. The missing value default is the default  state."

[2] "Each input element has a resulting autocompletion state, which is either on or off."

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Part 1: Form autocomplete (v1)

Let's have this done.
form.autocomplete is simple.

input.autocomplete is going to reflect the computed state. We will see if whatwg is fine with that.

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Sorry, looks like I select the wrong file in the file picker...

Comment 11

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Form autocomplete (v1)

Comment 12

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Input autocomplete (v1)

We talked with Jonas about this attribute and we agreed that reflecting the autocompletion state is probably much more useful than reflecting the content attribute. Actually, reflecting the content attribute is pretty useless for autocomplete.
So, we are going to implement that and change if people in whatwg strongly disagree. (Hixie didn't give a real feedback about that proposition).

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Input autocomplete (v2)

Both patches are now following the specifications.

Comment 14

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

Comment on attachment 468082 [details] [diff] [review]
Part 1 - Form autocomplete (v1)


>-[scriptable, uuid(083d2e20-a996-11df-94e2-0800200c9a66)]
>+[scriptable, uuid(653dc482-d6db-4e85-bdd9-151fd110e7b1)]
> interface nsIDOMHTMLFormElement : nsIDOMHTMLElement
> {
>+           attribute DOMString            acceptCharset;
>+           attribute DOMString            action;
>+           attribute DOMString            autocomplete;
>   readonly attribute nsIDOMHTMLCollection elements;
>   readonly attribute long                 length;
>            attribute DOMString            name;
>-           attribute DOMString            acceptCharset;
>-           attribute DOMString            action;
>            attribute DOMString            enctype;
>            attribute DOMString            method;
>            attribute DOMString            target;

Why you move action and acceptCharset?The attributes aren't in
alphabetical order in either case, so just add autocomplete to the end.

Comment 15

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

Comment on attachment 468901 [details] [diff] [review]
Part 2 - Input autocomplete (v2)

So autocomplete handling in input element is very different than in
form element. That doesn't make sense.
https://bug557628.bugzilla.mozilla.org/attachment.cgi?id=468083 would make
more sense.
Why is the HTML5 draft adding specified the way it is?
(I assume this patch implements the latest HTML5 draft)

Comment 16

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

(In reply to comment #15)
> Why is the HTML5 draft adding specified the way it is?
s/adding//

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Input autocomplete (v3)

Comment 18

[Mounir Lamouri (:mounir)]

Comment on attachment 474601 [details] [diff] [review]
Part 2 - Input autocomplete (v3)

This is a patch following a discussion with Jonas:
input.autocomplete doesn't return the state but is limited to only known values (the spec doesn't make it limited to only known values). If @autocomplete is different from "on" or "off", it will return "".

The reason to give up with returning the state is that is not really common in the API and it will having the setter and the getter doing very different things so input.autocomplete = input.autocomplete wouldn't be a no-op.

Jonas told me he will review it but I would be glad to have your feedback on that Olli.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

FWIW, a nice advantage of this solution is that you can get the computed autocomplete value (if you happen to care) using the javascript expression:

inputelem.autocomplete || inputelem.form.autocomplete

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Also, this does seem to match what the spec says. It does limit inputelement.autocomplete to known values.

Comment 21

[Mounir Lamouri (:mounir)]

Oh, I didn't see the change :-/ I should got 100 lashes!

Comment 22

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

Comment on attachment 474601 [details] [diff] [review]
Part 2 - Input autocomplete (v3)

Yeah, this is good.

Comment 23

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/46c16b4a5995
http://hg.mozilla.org/mozilla-central/rev/7a99955956fd