Implement autocomplete attribute for input and form elements

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Blocks: 2 bugs, {dev-doc-complete, html5})

Trunk
mozilla2.0b7
dev-doc-complete, html5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

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

Comment 1

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

Comment 2

7 years ago
- 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)

Updated

7 years ago
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Keywords: dev-doc-needed
(Assignee)

Comment 3

7 years ago
Created attachment 438458 [details] [diff] [review]
Tests v1

I did not test the UI aspects. I don't know how to do that easily.
Attachment #438458 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 4

7 years ago
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.
Attachment #438459 - Flags: review?(Olli.Pettay)

Comment 5

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

Comment 6

7 years ago
This patch is using the macro introduced in the patch from bug 551670.
Depends on: 551670

Updated

7 years ago
Blocks: 559749

Comment 7

7 years ago
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-

Updated

7 years ago
Attachment #438458 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 8

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

Comment 9

7 years ago
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.
Attachment #438458 - Attachment is obsolete: true
Attachment #438459 - Attachment is obsolete: true
Attachment #467983 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 10

7 years ago
Created attachment 468075 [details] [diff] [review]
Patch v1.1

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

Comment 11

7 years ago
Created attachment 468082 [details] [diff] [review]
Part 1 - Form autocomplete (v1)
Attachment #468075 - Attachment is obsolete: true
Attachment #468082 - Flags: review?(Olli.Pettay)
Attachment #468075 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 12

7 years ago
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).
Attachment #468083 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

7 years ago
Blocks: 589486
(Assignee)

Updated

7 years ago
Blocks: 589487
(Assignee)

Comment 13

7 years ago
Created attachment 468901 [details] [diff] [review]
Part 2 - Input autocomplete (v2)

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

Updated

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

Comment 17

7 years ago
Created attachment 474601 [details] [diff] [review]
Part 2 - Input autocomplete (v3)
(Assignee)

Updated

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

Comment 18

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

Updated

7 years ago
Attachment #468901 - Attachment is obsolete: true
Attachment #468901 - Flags: review?(Olli.Pettay)
Attachment #474601 - Flags: review?(jonas)
Attachment #474601 - Flags: review+
Attachment #474601 - Flags: approval2.0+
Attachment #468082 - Flags: approval2.0+
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.
(Assignee)

Comment 21

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

Comment 23

7 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/46c16b4a5995
http://hg.mozilla.org/mozilla-central/rev/7a99955956fd
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7

Updated

7 years ago
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.