Closed Bug 557628 Opened 10 years ago Closed 9 years ago

Implement autocomplete attribute for input and form elements

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b7

People

(Reporter: mounir, Assigned: mounir)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, html5)

Attachments

(2 files, 6 obsolete files)

$URL is for forms, this is for input elements: http://dev.w3.org/html5/spec/forms.html#attr-input-autocomplete
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
- 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.
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Keywords: dev-doc-needed
Attached patch Tests v1 (obsolete) — Splinter Review
I did not test the UI aspects. I don't know how to do that easily.
Attachment #438458 - Flags: review?(Olli.Pettay)
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #438459 - Flags: review?(Olli.Pettay)
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.
This patch is using the macro introduced in the patch from bug 551670.
Depends on: 551670
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
Attachment #438459 - Flags: review?(Olli.Pettay) → review-
Attachment #438458 - Flags: review?(Olli.Pettay) → review-
(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."
Attached patch Part 1: Form autocomplete (v1) (obsolete) — Splinter Review
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.
Attachment #438458 - Attachment is obsolete: true
Attachment #438459 - Attachment is obsolete: true
Attachment #467983 - Flags: review?(Olli.Pettay)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Sorry, looks like I select the wrong file in the file picker...
Attachment #467983 - Attachment is obsolete: true
Attachment #468075 - Flags: review?(Olli.Pettay)
Attachment #467983 - Flags: review?(Olli.Pettay)
Attachment #468075 - Attachment is obsolete: true
Attachment #468082 - Flags: review?(Olli.Pettay)
Attachment #468075 - Flags: review?(Olli.Pettay)
Attached patch Part 2 - Input autocomplete (v1) (obsolete) — Splinter Review
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).
Attachment #468083 - Flags: review?(Olli.Pettay)
Blocks: 589486
Blocks: 589487
Attached patch Part 2 - Input autocomplete (v2) (obsolete) — Splinter Review
Both patches are now following the specifications.
Attachment #468083 - Attachment is obsolete: true
Attachment #468901 - Flags: review?(Olli.Pettay)
Attachment #468083 - Flags: review?(Olli.Pettay)
Attachment #468082 - Attachment description: Patch 1 - Form autocomplete (v1) → Part 1 - Form autocomplete (v1)
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.
Attachment #468082 - Flags: review?(Olli.Pettay) → review+
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)
(In reply to comment #15)
> Why is the HTML5 draft adding specified the way it is?
s/adding//
Attachment #474601 - Attachment description: Part 2 → Part 2 - Input autocomplete (v3)
Attachment #474601 - Attachment is patch: true
Attachment #474601 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #474601 - Flags: review?(jonas)
Attachment #474601 - Flags: feedback?(Olli.Pettay)
Attachment #468901 - Attachment is obsolete: true
Attachment #468901 - Flags: review?(Olli.Pettay)
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
Also, this does seem to match what the spec says. It does limit inputelement.autocomplete to known values.
Oh, I didn't see the change :-/ I should got 100 lashes!
Comment on attachment 474601 [details] [diff] [review]
Part 2 - Input autocomplete (v3)

Yeah, this is good.
Attachment #474601 - Flags: feedback?(Olli.Pettay) → feedback+
Pushed:
http://hg.mozilla.org/mozilla-central/rev/46c16b4a5995
http://hg.mozilla.org/mozilla-central/rev/7a99955956fd
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.