Last Comment Bug 557628 - Implement autocomplete attribute for input and form elements
: Implement autocomplete attribute for input and form elements
Status: RESOLVED FIXED
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla2.0b7
Assigned To: Mounir Lamouri (:mounir)
:
:
Mentors:
http://dev.w3.org/html5/spec/forms.ht...
Depends on: 551670
Blocks: html5forms 589487 559749 589486
  Show dependency treegraph
 
Reported: 2010-04-06 13:32 PDT by Mounir Lamouri (:mounir)
Modified: 2010-09-27 14:12 PDT (History)
9 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Tests v1 (7.19 KB, patch)
2010-04-12 04:56 PDT, Mounir Lamouri (:mounir)
bugs: review-
Details | Diff | Splinter Review
Patch v1 (21.07 KB, patch)
2010-04-12 05:02 PDT, Mounir Lamouri (:mounir)
bugs: review-
Details | Diff | Splinter Review
Part 1: Form autocomplete (v1) (145 bytes, patch)
2010-08-20 18:51 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.1 (7.21 KB, patch)
2010-08-21 14:36 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 1 - Form autocomplete (v1) (7.21 KB, patch)
2010-08-21 16:10 PDT, Mounir Lamouri (:mounir)
bugs: review+
jonas: approval2.0+
Details | Diff | Splinter Review
Part 2 - Input autocomplete (v1) (6.50 KB, patch)
2010-08-21 16:13 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 2 - Input autocomplete (v2) (6.01 KB, patch)
2010-08-24 19:11 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 2 - Input autocomplete (v3) (7.97 KB, patch)
2010-09-12 20:02 PDT, Mounir Lamouri (:mounir)
jonas: review+
bugs: feedback+
jonas: approval2.0+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2010-04-06 13:32:56 PDT
$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] 2010-04-08 04:38:32 PDT
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) 2010-04-08 05:03:53 PDT
- 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) 2010-04-12 04:56:37 PDT
Created attachment 438458 [details] [diff] [review]
Tests v1

I did not test the UI aspects. I don't know how to do that easily.
Comment 4 Mounir Lamouri (:mounir) 2010-04-12 05:02:07 PDT
Created attachment 438459 [details] [diff] [review]
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] 2010-04-12 05:10:28 PDT
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) 2010-04-12 08:32:07 PDT
This patch is using the macro introduced in the patch from bug 551670.
Comment 7 Olli Pettay [:smaug] 2010-04-18 14:26:08 PDT
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) 2010-04-18 16:08:42 PDT
(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) 2010-08-20 18:51:15 PDT
Created attachment 467983 [details] [diff] [review]
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) 2010-08-21 14:36:20 PDT
Created attachment 468075 [details] [diff] [review]
Patch v1.1

Sorry, looks like I select the wrong file in the file picker...
Comment 11 Mounir Lamouri (:mounir) 2010-08-21 16:10:18 PDT
Created attachment 468082 [details] [diff] [review]
Part 1 - Form autocomplete (v1)
Comment 12 Mounir Lamouri (:mounir) 2010-08-21 16:13:09 PDT
Created attachment 468083 [details] [diff] [review]
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) 2010-08-24 19:11:24 PDT
Created attachment 468901 [details] [diff] [review]
Part 2 - Input autocomplete (v2)

Both patches are now following the specifications.
Comment 14 Olli Pettay [:smaug] 2010-09-06 07:59:47 PDT
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] 2010-09-06 08:09:35 PDT
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] 2010-09-06 08:12:37 PDT
(In reply to comment #15)
> Why is the HTML5 draft adding specified the way it is?
s/adding//
Comment 17 Mounir Lamouri (:mounir) 2010-09-12 20:02:03 PDT
Created attachment 474601 [details] [diff] [review]
Part 2 - Input autocomplete (v3)
Comment 18 Mounir Lamouri (:mounir) 2010-09-12 20:05:39 PDT
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 2010-09-12 22:18:08 PDT
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 2010-09-12 23:21:31 PDT
Also, this does seem to match what the spec says. It does limit inputelement.autocomplete to known values.
Comment 21 Mounir Lamouri (:mounir) 2010-09-12 23:25:18 PDT
Oh, I didn't see the change :-/ I should got 100 lashes!
Comment 22 Olli Pettay [:smaug] 2010-09-14 10:35:48 PDT
Comment on attachment 474601 [details] [diff] [review]
Part 2 - Input autocomplete (v3)

Yeah, this is good.

Note You need to log in before you can comment on or make changes to this bug.