1020697 - Implement @autocomplete for <select>

Status: RESOLVED FIXED

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

Description

[Matthew N. [:MattN]]

The @autocomplete attribute can be specified on <select> so requestAutocomplete knows what option to select for the user.

Comment 1

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

Attachment: autocompleteselect.patch

Comment 2

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

Sorry pressed enter too fast. I moved the caching logic to nsContentUtils::SerializeAutocompleteAttribute to avoid duplication. But I can look for alternatives if you think it's not a good idea. 

Try: https://tbpl.mozilla.org/?tree=Try&rev=1a05d6c5d036

Comment 3

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

Comment on attachment 8440449 [details] [diff] [review]
autocompleteselect.patch

>+  static AutocompleteAttrState
>+  SerializeAutocompleteAttribute(const nsAttrValue* aAttr,
>+                                 nsAString& aResult,
>+                                 AutocompleteAttrState
>+                                   aCachedState = eAutocompleteAttrState_Unknown);
                                   AutocompleteAttrState aCachedState =
                                     eAutocompleteAttrState_Unknown


> nsContentUtils::AutocompleteAttrState
> nsContentUtils::SerializeAutocompleteAttribute(const nsAttrValue* aAttr,
>-                                           nsAString& aResult)
>-{
>+                                               nsAString& aResult,
>+                                               nsContentUtils::AutocompleteAttrState
>+                                                                        aCachedState)
Do you need nsContentUtils:: prefix here?

>+[uuid(a287295d-b4ff-4186-83eb-467b34f8314d)]
> interface nsIDOMHTMLSelectElement : nsISupports
> {
>            attribute boolean                     autofocus;
>+           attribute DOMString                   autocomplete;
>            attribute boolean                     disabled;
>   readonly attribute nsIDOMHTMLFormElement       form;
>            attribute boolean                     multiple;
>            attribute DOMString                   name;
>            attribute unsigned long               size;
> 
>   readonly attribute DOMString                   type;
> 
No need to add new stuff to .idl. .idl is deprecated for web features.

> interface HTMLSelectElement : HTMLElement {
>   [SetterThrows, Pure]
>            attribute boolean autofocus;
>   [SetterThrows, Pure]
>+           attribute DOMString autocomplete;
It is not clear to me why this needs to have SetterThrows, but since HTMLInputElement has that
too, better be consistent. 
Technically SetAttr may throw, but only in case of too many attributes, and perhaps in case of OOM.


Otherwise ok, but we don't really support <select> in the current autocomplete implementation, even on/off values, right?
So we'd need to either disable autocomplete in webidl, if there is no pref set, or make sure the toolkit/browser parts can deal with select.

For now, I think, it would be ok to just disable the attribute in webidl, if the pref isn't set.
So, add [Pref="<whatever pref we use for more complicated autocomplete stuff>"]

Comment 4

[Matthew N. [:MattN]]

Yeah, we should put this behind the dom.forms.autocomplete.experimental pref for now until we do something useful with it which is hopefully within a few months.

I also wondered if we should pass the input/select/textarea (bug 1020698) to SerializeAutocompleteAttribute instead of the attribute value and the cached state separately by having them implement a common autocompletable interface or something. That would also make it easier to SerializeAutocompleteAttribute to update more state on the elements like we may need for bug 1020496. e.g. persisting an AutocompleteCategory member.

Comment 5

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

Attachment: Implement @autocomplete for <select> ; r=smaug

> Do you need nsContentUtils:: prefix here?
Err. No :-) Thanks for the review!

(In reply to Matthew N. [:MattN] from comment #4)
> I also wondered if we should pass the input/select/textarea (bug 1020698) to
> SerializeAutocompleteAttribute instead of the attribute value and the cached
> state separately by having them implement a common autocompletable interface
> or something. That would also make it easier to
> SerializeAutocompleteAttribute to update more state on the elements like we
> may need for bug 1020496. e.g. persisting an AutocompleteCategory member.

That doesn't sound like a bad idea, it maybe a little bit too overweight though? Olli what do you think?

Comment 6

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

The patch in this bug moves already some more stuff to nsContentUtils, so perhaps a base
class isn't really needed here. Depends on what all will be added later.
But sounds like something for a separate bug.

(we do have nsGenericHTMLFormElement, but that is used for all the form controls so it wouldn't work for this.)

Comment 7

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

Comment on attachment 8441147 [details] [diff] [review]
Implement @autocomplete for <select> ; r=smaug

>+  SerializeAutocompleteAttribute(const nsAttrValue* aAttr,
>+                                 nsAString& aResult,
>+                                 AutocompleteAttrState
>+                                   aCachedState = eAutocompleteAttrState_Unknown);
                                   AutocompleteAttrState aCachedState =
                                     eAutocompleteAttrState_Unknown);

Comment 8

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

Attachment: Implement @autocomplete for <select> ; r=smaug

Thanks!

Linux try: https://tbpl.mozilla.org/?tree=Try&rev=305f5540af89

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d346871db5f6

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d346871db5f6

Comment 11

[Ed Morley [:emorley]]

Unfortunately I've had to back this out for intermittent test failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=42031278&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=42032819&tree=Fx-Team

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0c8a072f624f

Comment 12

[Ed Morley [:emorley]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=42062271&tree=Fx-Team

Comment 13

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

I tried to reproduce it today and I couldn't after 300 runs on my machine, a linux64 (I could reproduce it fairly reliably two weeks ago).

Try on Mac is also green: https://tbpl.mozilla.org/?tree=Try&rev=a7a74a5c4066

Maybe we can try to reland?

Comment 14

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

Yes, I guess we could do that.

Comment 15

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

All right. Thanks!

Comment 16

[Ryan VanderMeulen [:RyanVM]]

I trigger a Windows build on your Try push and confirmed that this still hits:
https://tbpl.mozilla.org/php/getParsedLog.php?id=42775287&tree=Try

Comment 17

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

Weird. Thanks for checking windows Ryan . I'll see if I can get hold of a windows machine.

Comment 18

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

Attachment: Implement @autocomplete for <select> ; r=smaug

Sorry for the delay here. I think I got it, more or less. The problem is that the pref is locked down whenever a HTMLSelectElement is first requested by javascript (maybe it's when the prototype object is created internally?). So if the pref is set after that moment, the test fails.

E.g.

var x = document.getElementById("select-element");

function inPref() {
  // here autocomplete will never be defined
  // because we have requested a HTMLSelectElement above
}

SpecialPowers.pushPrefEnv({"set": [["dom.forms.autocomplete.experimental", true]]}, inPref);

What I'm doing in this patch is moving the <script> block above the <body> to minimize the chance that a <select> element is created too soon. It looks like it's working. 

About this problem, I think it could causes confusion and/or intermittent tests? But I'm not sure if it's the intended behavior or if I should submit a bug.

Olli do you want to take a look? I just rebased the patch (some things in the older patch were commited with Bug 1020496) and changed the test.

Try: https://tbpl.mozilla.org/?tree=Try&rev=8d95bbddcc3c

Comment 19

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

(In reply to Giovanni Sferro (UTC-5) [:agi] from comment #18)
> Created attachment 8462183 [details] [diff] [review]
> Implement @autocomplete for <select> ; r=smaug
> 
> Sorry for the delay here. I think I got it, more or less. The problem is
> that the pref is locked down whenever a HTMLSelectElement is first requested
> by javascript (maybe it's when the prototype object is created internally?).
Ah yes. Usually when one hits this kind of issue, the solution is to set the pref in the main
document and then do the testing in an iframe which is loaded after the pref has been set.

Comment 20

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

...but just not accessing the relevant prototype before setting the pref should be enough.

Comment 21

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

> Ah yes. Usually when one hits this kind of issue, the solution is to set the pref in the main
document and then do the testing in an iframe which is loaded after the pref has been set.

Interesting, I'll keep that in mind if I encounter this problem again, thanks!

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57e293375c81

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/57e293375c81