1030301 - Filter unsupported <input> types that use @autocomplete

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Brian Nicholson (:bnicholson)]

With bug 1020496, all <input> types will have an @autocomplete attribute -- even those that aren't supported for rAc (radio, checkbox, etc.). We should filter out unsupported types.

(Quoting Matthew N. [:MattN] from https://bugzilla.mozilla.org/show_bug.cgi?id=1020602#c3)
> You can use input.mozIsTextField(true) to filter out radio/checkbox but it
> also filters out type=hidden. Exposing nsIFormControl::IsTextControl would
> be better since that includes textarea but hidden would still need to be
> special-cased. I just realized that number is missing too and possibly
> others so maybe we should have a separate helper perhaps on nsIFormControl
> (eventually) like isAutocompletableControl.

Comment 1

[[ex-Mozilla] Agi Sferro | :agi]

Would returning a blank string for non-supported input types work?

so e.g.

<input type="checkbox" autocomplete="tel">

would have @autocomplete == ""

Comment 2

[Brian Nicholson (:bnicholson)]

When I filed this bug, I think I meant to say getAutocompleteInfo() instead of @autocomplete since this is going to be used by us to filter out invalid inputs. After talking to MattN, though, he thinks both GetAutocomplete and GetAutocompleteInfo should be changed here: https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLInputElement.cpp?mark=1547#1529. This would keep @autocomplete and getAutocompleteInfo consistent.

Rather than returning a blank string, could we return null? smaug, what do you think?

Comment 3

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

The spec doesn't let us return null.
1. If the element has no autocomplete attribute, then jump to the step labeled default.
...
25. Default: Let the element's IDL-exposed autofill value be the empty string, and its autofill hint set and autofill scope be empty.

And the interface has 
attribute DOMString autocomplete;
not
attribute DOMString? autocomplete;

Comment 4

[Brian Nicholson (:bnicholson)]

Thanks. So how about this:
* For @autocomplete, return an empty string as suggested in comment 1.
* For getAutocompleteInfo() (a ChromeOnly helper function), return null.

Comment 5

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

ChromeOnly stuff can return null. Just document it clearly in the interface.

Comment 6

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Filter out non-autocomplete-enabled types for <input>

This patch implements the changes as per comment 4. I'm following the specs for @type=hidden [1] but it looks like they are not in agreement with our tests [2].

I'm not sure what's the process here, should we change the module to DOM: Core & HTML? 

Olli do you want to review this?

Thanks!

[1]: http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#hidden-state-%28type=hidden%29 
[2]: https://tbpl.mozilla.org/?tree=Try&rev=1d6ee00f0199

Comment 7

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

Ugh the spec is pretty much unreadable :(

Comment 8

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

Comment on attachment 8481997 [details] [diff] [review]
Filter out non-autocomplete-enabled types for <input>

Sounds like we need to fix some tests then.
But please check what other browsers do with type="hidden"

Comment 9

[[ex-Mozilla] Agi Sferro | :agi]

WebKit/Chrome doesn't hide the value of @autocomplete for any @type, but does block @type="hidden" input controls from being autocompleted with rAc

Comment 10

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Filter out non-autocomplete-enabled types for <input>

OK I removed the test for @type="hidden" that was failing.

Matt, is there a reason why you included "hidden" among autocompletable controls? Are you ok with removing it? Thanks!

Comment 11

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Filter out non-autocomplete-enabled types for <input> v2

Oh well. The spec changed a few days ago and now type="hidden" is allowed among controls that admits a @autocomplete attribute. Since allowing @autocomplete on type="hidden" is what we were doing before, I guess it makes sense to not change this behavior.

Olli, this patch is essentially the one you reviewed in Comment 8, except that now type="hidden" is autocompletable (hence we don't need to change tests). Asking for review just to be sure this is what we want to check-in. Thanks!

Try: https://tbpl.mozilla.org/?tree=Try&rev=66ac5b2a3c28
(note to sheriffs: the failure in Android 4.0 rc1 is not caused by my patch, e.g. https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=de47d4022beb )

Comment 12

[[ex-Mozilla] Agi Sferro | :agi]

Thanks!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3beca2d04181

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3beca2d04181