Closed Bug 1020697 Opened 6 years ago Closed 5 years ago

Implement @autocomplete for <select>

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: MattN, Assigned: Agi)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 3 obsolete files)

The @autocomplete attribute can be specified on <select> so requestAutocomplete knows what option to select for the user.
Attached patch autocompleteselect.patch (obsolete) — Splinter Review
Attachment #8440449 - Flags: review?(bugs)
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 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>"]
Attachment #8440449 - Flags: review?(bugs) → review-
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.
Assignee: nobody → agi.novanta
Status: NEW → ASSIGNED
> 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?
Attachment #8440449 - Attachment is obsolete: true
Attachment #8441147 - Flags: review?(bugs)
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 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);
Attachment #8441147 - Flags: review?(bugs) → review+
Thanks!

Linux try: https://tbpl.mozilla.org/?tree=Try&rev=305f5540af89
Attachment #8441147 - Attachment is obsolete: true
Attachment #8441814 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d346871db5f6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla33 → ---
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?
Flags: needinfo?(bugs)
Yes, I guess we could do that.
Flags: needinfo?(bugs)
All right. Thanks!
Keywords: checkin-needed
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
Keywords: checkin-needed
Weird. Thanks for checking windows Ryan . I'll see if I can get hold of a windows machine.
Depends on: 1020496
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
Attachment #8441814 - Attachment is obsolete: true
Attachment #8462183 - Flags: review?(bugs)
(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.
Attachment #8462183 - Flags: review?(bugs) → review+
...but just not accessing the relevant prototype before setting the pref should be enough.
> 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!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/57e293375c81
Status: REOPENED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
Duplicate of this bug: 1333353
You need to log in before you can comment on or make changes to this bug.