556007 - Implement list attribute

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

The list attribute lets the author to specify a suggestion list for users. The list comes from a datalist (bug 555840).

Comment 1

[David Zbarsky (:dzbarsky)]

I'm going to work on this.

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: Part 1: Provide support for list in nsHTMLInputElement

Comment 3

[David Zbarsky (:dzbarsky)]

Attachment: Part 2: Handle the autocomplete

This patch works by modifying nsFormAutoComplete.js.  I was talking to dolske, and he suggested that I should write a separate js component to handle the merging, which is what I'm going to work on next.

Comment 4

[David Zbarsky (:dzbarsky)]

I see the whitespace and newline problems, I'll fix those at the end.

Comment 5

[Mounir Lamouri (:mounir)]

Do we want @list to be autocompleted or always shown to the user? I think we are not in the same use case than the classic autocompletion feature: the autocompletion is designed to prevent the user to type something she/he already typed before and @list is designed to help the user choose a value or let him know what kind of values should be entered. If we autocomplete with @list the user may miss most of the propositions.

In another hand, not having an autocompletion list would be really annoying if the provided list is really long but that's an edge case. One of the solutions would be to show the list for only a few seconds maybe?

Also, keep in mind the propositions in @list will have to be filtered if they do not respect the element constraint validations.

Comment 6

[David Zbarsky (:dzbarsky)]

>If we autocomplete with @list the
user may miss most of the propositions.

I don't quite understand what you mean.

I think the @list should be always shown. If autocomplete is enabled, the list is supplemented with form history values.

I will take a look at the constraint validation next.

Comment 7

[David Zbarsky (:dzbarsky)]

Attachment: Part 2: Handle the autocomplete

I am now using a separate js component, in case we decide that @list should work with the login manager (it will be very simple to fix).

Comment 8

[Mounir Lamouri (:mounir)]

Comment on attachment 459447 [details] [diff] [review]
Part 1: Provide support for list in nsHTMLInputElement

I can't review your patch (no right for that) but I can do a feedback.

I've set Jonas as a reviewer. Feel free to change.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 459447 [details] [diff] [review]
Part 1: Provide support for list in nsHTMLInputElement

>diff --git a/content/html/content/src/nsHTMLInputElement.cpp b/content/html/content/src/nsHTMLInputElement.cpp
>--- a/content/html/content/src/nsHTMLInputElement.cpp
>+++ b/content/html/content/src/nsHTMLInputElement.cpp
>@@ -405,16 +405,20 @@ nsHTMLInputElement::AfterSetAttr(PRInt32
>     //
>     if ((aName == nsGkAtoms::name ||
>          (aName == nsGkAtoms::type && !mForm)) &&
>         mType == NS_FORM_INPUT_RADIO &&
>         (mForm || !(GET_BOOLBIT(mBitField, BF_PARSER_CREATING)))) {
>       AddedToRadioGroup();
>     }
> 
>+    if (aName == nsGkAtoms::list) {
>+      mDataListId.Assign(*aValue);
>+    }

It's probably not worth caching this value. Simply use GetAttr in the places where we need it instead.

>+nsHTMLInputElement::GetList(nsIDOMHTMLElement** aValue)
>+{
>+  if (!mDataListId.IsEmpty()) {
>+    Element* elem = GetCurrentDoc()->GetElementById(mDataListId);

GetCurrentDoc() can return null, so you need to add a nullcheck here.

>@@ -2105,16 +2126,20 @@ nsHTMLInputElement::ParseAttribute(PRInt
>       return aResult.ParseIntWithBounds(aValue, 0);
>     }
>     if (aAttribute == nsGkAtoms::border) {
>       return aResult.ParseIntWithBounds(aValue, 0);
>     }
>     if (aAttribute == nsGkAtoms::align) {
>       return ParseAlignValue(aValue, aResult);
>     }
>+    if (aAttribute == nsGkAtoms::list) {
>+      aResult.ParseStringOrAtom(aValue);
>+      return PR_TRUE;
>+    }

Is there a reason for this change? Without it we'll always store the value as a string which seems like it should work fine.

>diff --git a/dom/interfaces/html/nsIDOMHTMLInputElement.idl b/dom/interfaces/html/nsIDOMHTMLInputElement.idl
>--- a/dom/interfaces/html/nsIDOMHTMLInputElement.idl
>+++ b/dom/interfaces/html/nsIDOMHTMLInputElement.idl
>@@ -56,17 +56,19 @@ interface nsIDOMHTMLInputElement : nsIDO
>            attribute DOMString             accept;
>            attribute DOMString             accessKey;
>            attribute DOMString             align;
>            attribute DOMString             alt;
>            attribute boolean               checked;
>            attribute boolean               disabled;
>            attribute long                  maxLength;
>            attribute DOMString             name;
>-           attribute boolean               readOnly;
>+           attribute boolean               readOnly;
>+  readonly attribute nsIDOMHTMLElement     list;

You need to update the iid here.

Looks like this is the right approach in general though.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 459502 [details] [diff] [review]
Part 2: Handle the autocomplete

One thing that I think we might want to support is dynamic changes to the list.

This would be useful for implementing dynamically populated <datalist>s. For example many search interfaces allow you to start typing, and then suggests better and better alternatives as you are typing.

An example is the 'assign to' field in bugzilla.

One way to implement this would be to add a function on nsIInputListAutoComplete that allows a callback to be registered which is called any time the list of items change.

Comment 11

[Mounir Lamouri (:mounir)]

Comment on attachment 459447 [details] [diff] [review]
Part 1: Provide support for list in nsHTMLInputElement

>+    if (aName == nsGkAtoms::list) {
>+      mDataListId.Assign(*aValue);
>+    }

Why do you want to keep this value ? That does not seem needed.

> nsresult
>+nsHTMLInputElement::GetList(nsIDOMHTMLElement** aValue)
>+{
>+  if (!mDataListId.IsEmpty()) {
>+    Element* elem = GetCurrentDoc()->GetElementById(mDataListId);

You should call GetAttr instead of using a cached value.

>+    if (elem) {
>+      nsCOMPtr<nsIDOMHTMLElement> list = do_QueryInterface(elem);

According to the spec if the element is not a <datalist> you have to return null.

>+    if (aAttribute == nsGkAtoms::list) {
>+      aResult.ParseStringOrAtom(aValue);
>+      return PR_TRUE;
>+    }

I don't think you need that.

Comment 12

[Mounir Lamouri (:mounir)]

(In reply to comment #6)
> >If we autocomplete with @list the
> user may miss most of the propositions.
> 
> I don't quite understand what you mean.
> 
> I think the @list should be always shown. If autocomplete is enabled, the list
> is supplemented with form history values.
> 
> I will take a look at the constraint validation next.

I meant the @list should always be entirely shown even when you've started typing. It should not behave like the autocompletion which is showing only values related to what you are typing.

I thought you were going to implement that like autocompletion. I've probably have misunderstand.

Comment 13

[David Zbarsky (:dzbarsky)]

(In reply to comment #10)
> Comment on attachment 459502 [details] [diff] [review]
> Part 2: Handle the autocomplete
> 
> One thing that I think we might want to support is dynamic changes to the list.

Maybe I'm misunderstanding you, but I think this already happens.  My approach looks up the list with every keystroke, so it will automatically find changes.

Comment 14

[David Zbarsky (:dzbarsky)]

Attachment: Support for @list

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 459630 [details] [diff] [review]
Support for @list

>diff --git a/content/base/src/nsGkAtomList.h b/content/base/src/nsGkAtomList.h
>--- a/content/base/src/nsGkAtomList.h
>+++ b/content/base/src/nsGkAtomList.h
>@@ -495,16 +495,17 @@ GK_ATOM(leftmargin, "leftmargin")
> GK_ATOM(leftpadding, "leftpadding")
> GK_ATOM(legend, "legend")
> GK_ATOM(length, "length")
> GK_ATOM(letterValue, "letter-value")
> GK_ATOM(level, "level")
> GK_ATOM(li, "li")
> GK_ATOM(line, "line")
> GK_ATOM(link, "link")
>+GK_ATOM(list, "list")
> GK_ATOM(listbox, "listbox")
> GK_ATOM(listboxbody, "listboxbody")
> GK_ATOM(listcell, "listcell")
> GK_ATOM(listcol, "listcol")
> GK_ATOM(listcols, "listcols")
> GK_ATOM(listener, "listener")
> GK_ATOM(listhead, "listhead")
> GK_ATOM(listheader, "listheader")
>diff --git a/content/html/content/src/nsHTMLInputElement.cpp b/content/html/content/src/nsHTMLInputElement.cpp
>--- a/content/html/content/src/nsHTMLInputElement.cpp
>+++ b/content/html/content/src/nsHTMLInputElement.cpp
>@@ -45,16 +45,17 @@
> #include "nsIPhonetic.h"
> 
> #include "nsIControllers.h"
> #include "nsFocusManager.h"
> #include "nsPIDOMWindow.h"
> #include "nsContentCID.h"
> #include "nsIComponentManager.h"
> #include "nsIDOMHTMLFormElement.h"
>+#include "nsIDOMHTMLDataListElement.h"
> #include "nsIDOMEventTarget.h"
> #include "nsGkAtoms.h"
> #include "nsStyleConsts.h"
> #include "nsPresContext.h"
> #include "nsMappedAttributes.h"
> #include "nsIFormControl.h"
> #include "nsIForm.h"
> #include "nsFormSubmission.h"
>@@ -1988,16 +1989,38 @@ nsHTMLInputElement::MaybeLoadImage()
>       GetAttr(kNameSpaceID_None, nsGkAtoms::src, uri) &&
>       (NS_FAILED(LoadImage(uri, PR_FALSE, PR_TRUE)) ||
>        !LoadingEnabled())) {
>     CancelImageRequests(PR_TRUE);
>   }
> }
> 
> nsresult
>+nsHTMLInputElement::GetList(nsIDOMHTMLElement** aValue)
>+{
>+  nsString dataListId;
>+  if (GetAttr(kNameSpaceID_None, nsGkAtoms::list, dataListId)) {

You probably also want to avoid stepping into this code if the list attribute is empty. getElementById will put a warning in the error console when that happens. So change the code to something like

nsString dataListId;
GetAttr(kNameSpaceID_None, nsGkAtoms::list, dataListId);
if (!dataListId.IsEmpty()) {



>+    nsIDocument* doc = GetCurrentDoc();
>+
>+    if (doc) {
>+      Element* elem = doc->GetElementById(dataListId);
>+
>+      if (elem) {
>+        nsCOMPtr<nsIDOMHTMLDataListElement> list = do_QueryInterface(elem);
>+        NS_IF_ADDREF(*aValue = list);
>+        return NS_OK;

This can be faster written as

*aValue = list.forget().get();

This will avoid the extra addref and release that your version has. I.e. in your version we addref in the NS_IF_ADDREF macro, and release when the nsCOMPtr goes out of scope.

Sorry for not catching this before.

r=me with those things fixed!

Comment 16

[Mounir Lamouri (:mounir)]

Comment on attachment 459630 [details] [diff] [review]
Support for @list

> nsresult
>+nsHTMLInputElement::GetList(nsIDOMHTMLElement** aValue)
>+{
>+  nsString dataListId;
>+  if (GetAttr(kNameSpaceID_None, nsGkAtoms::list, dataListId)) {
>+    nsIDocument* doc = GetCurrentDoc();

I think nsAutoString should be used instead of nsString here.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #13)
> (In reply to comment #10)
> > Comment on attachment 459502 [details] [diff] [review] [details]
> > Part 2: Handle the autocomplete
> > 
> > One thing that I think we might want to support is dynamic changes to the list.
> 
> Maybe I'm misunderstanding you, but I think this already happens.  My approach
> looks up the list with every keystroke, so it will automatically find changes.

I think we want to update the list even if there are no keystrokes. Imagine an application like this:

myInputElement.oninput = function() {
  var xhr = new XMLHttpRequest();
  xhr.open("GET", "/autocompletevalues.cgi?value=" + myInputElement.value);
  xhr.send();
  xhr.onload = function() {
    myInputElement.list.innerHTML = xhr.responseText;
  }
}

In this example, the contents of the <datalist> will change a few milliseconds *after* every keystroke. But we still want to pick up those changes as soon as they happen, rather than waiting until the next keystroke.

Also, like mounir points out, we want to populate the autocomplete with *all* values in the <datalist>. Not just the ones that start with the values typed by the user. Consider the above example code, it could produce an autocomplete with all values that contained the type strings, not just the ones that start with it. It could also contain spelling corrections and similar suggestions.

Though admittedly there's an interesting usecase to automatically filter out only the values that start with, or contain, the values typed by the user. Maybe that's a feature we should enable using some additional, "-moz"-prefixed  attribute.

Comment 18

[David Zbarsky (:dzbarsky)]

Attachment: Part 2: Handle the autocomplete

The previous version had a bug where if you started typing something that would autocomplete, and then typed other letters that didn't match those results, they wouldn't disappear.  This is now fixed.

Currently, the patch looks for a datalist element with an id matching the @list.  If it finds one, it extracts the options.  It places all options from the datalist in the autocomplete popup, below the form history suggestions.  If form history is disabled, it only displays the suggestions from the datalist.

The next task is handling updates, as Jonas suggested.

Comment 19

[David Zbarsky (:dzbarsky)]

Attachment: Part 1: support for @list (r=sicking)

review comments

Comment 20

[David Zbarsky (:dzbarsky)]

Attachment: Part 3: Dynamically update the popup when datalist changes

I used the following code to test this:

<input name=email type=url list=hpurls id="aa" size=50>
<datalist id=hpurls>
 <option value="http://www.google.com/" label="Google">
 <option value="http://www.reddit.com/" label="Reddit">
</datalist>

<script>
document.getElementById("aa").onclick = function() {
setTimeout("doit()", 2000);
}

function doit()
{
  document.getElementById("hpurls").innerHTML += "<option>aa</option>";
}
</script>

I'm not sure about my changes in nsAutoCompleteController because I don't quite understand the point of mFirstSearchResult in the first place.  Also, when JS calls .innerHTML += "stuff" we end up removing all children of the element and reappending them.  Is there a reason for that?

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 459710 [details] [diff] [review]
Part 2: Handle the autocomplete

The whitespace in InputListAutoComplete.prototype.autoCompleteSearch is completely messed up...

Comment 22

[David Zbarsky (:dzbarsky)]

Oh wow. It doesn't show up like that in my editor or when I look at the diff locally.  I'll fix it before I check in.

Comment 23

[David Zbarsky (:dzbarsky)]

Attachment: Part 2: Handle the autocomplete

Fix the weird whitespace

Comment 24

[David Zbarsky (:dzbarsky)]

Attachment: Part 3: Dynamically update the popup when datalist changes

Comment 25

[:Gavin Sharp [email: [e-mail]]]

I'm not sure I'll be able to get to this review request in the near future (next week or two). One thing I noticed from a quick skim is that you're duplicating SuggestAutoCompleteResult - we should avoid that, possibly by putting it in a JS module that can be import()ed in both places.

Comment 26

[David Zbarsky (:dzbarsky)]

Attachment: Screenshot

Comment 27

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 460276 [details] [diff] [review]
Part 2: Handle the autocomplete

Looking good. Only change I would do is to show the "label" attribute in the pulldown if it is defined, and fall back to "value" if it's not.

Comment 28

[Alex Limi (:limi) — Firefox UX Team]

To clarify, given this code:

<datalist id=hpurls>
 <option value="http://www.google.com/" label="Google">The Google site</option>
 <option value="http://www.reddit.com/">Reddit</option>
 <option value="http://limi.net" />
</datalist>

* First one would show "Google"
* Second one would show "Reddit"
* Third would show "http://limi.net"

All of them would insert the "value" in the actual input field (at least this seems to be what the spec says — seems a bit weird to me, especially in cases where the value is e.g a UUID). Would be more natural to show the label/content to be consistent with the pulldown.

Comment 29

[David Zbarsky (:dzbarsky)]

Attachment: Part 2: Handle the autocomplete

This patch now exhibits the behavior described by limi.

Comment 30

[David Zbarsky (:dzbarsky)]

Attachment: Part 2.5: Combine JS implementations of nsIAutoCompleteResult into JS Module

per gavin's comment

Comment 31

[David Zbarsky (:dzbarsky)]

Attachment: Part 2.5: Combine JS implementations of nsIAutoCompleteResult into JS Module

Comment 32

[David Zbarsky (:dzbarsky)]

Attachment: Part 2.5: Combine JS implementations of nsIAutoCompleteResult into JS Module

One of these days I'll learn to use hg...

Comment 33

[[e-mail]]

(In reply to comment #15)
>(From update of attachment 459630 [details] [diff] [review])
>>+        NS_IF_ADDREF(*aValue = list);
>>+        return NS_OK;
>This can be faster written as
>*aValue = list.forget().get();
Oh dear, 65 hits in MXR already. Please learn to use
list.forget(aValue);

Comment 34

[[e-mail]]

(In reply to comment #33)
> (In reply to comment #15)
> >(From update of attachment 459630 [details] [diff] [review])
> >>+        NS_IF_ADDREF(*aValue = list);
> >>+        return NS_OK;
> >This can be faster written as
> >*aValue = list.forget().get();
> Oh dear, 65 hits in MXR already.
Oops, this is the case when *aValue and list have different types.
In that case, yes, it gets ugly. Sorry for the confusion.

Comment 35

[David Zbarsky (:dzbarsky)]

Neil, I changed that to use CallQueryInterface.  I just didn't update the patch here yet.

Comment 36

[David Zbarsky (:dzbarsky)]

Attachment: Part 1: support for @list (r=sicking)

Get rid of that .forget().get() business

Comment 37

[David Zbarsky (:dzbarsky)]

Attachment: Part 3: Dynamically update the popup when datalist changes

Previous one had a leak

Comment 38

[David Zbarsky (:dzbarsky)]

Attachment: Part 4: update implementers of nsIAutoCompleteResult

This isn't really a standalone part, but it got separated from the other patches in my queue.  The uses of nsIAutoCompleteResult in comm-central will be fixed by Bug 582494

Comment 39

[[e-mail]]

Comment on attachment 460785 [details] [diff] [review]
Part 2: Handle the autocomplete

>   /**
>    * Get the value of the result at the given index
>    */
>   AString getValueAt(in long index);
> 
>   /**
>+   * This returns the string that should be placed in the input.
>+   */
>+  AString getActualValueAt(in long index);
From the description of the function, it would seem to be more useful to make the new method getLabelAt, and use that for the display label, and keep getValueAt for the actual value, and maybe implement some sort of fallback so that the form fill code tries to get the label, and if that is blank it simply uses the value instead.

Comment 40

[David Zbarsky (:dzbarsky)]

Neil, the patch is a little out of date.  I am waiting for Jonas to get back before I attach a new version.
In the newest version, what I did was create a new method getLabelAt, which in almost all cases returns the same thing as getValueAt(except in this particular case).  Instead of doing a fallback as you suggest, I think we should later go through and audit callers of getValueAt, and decide if they want the value or the label, and change them accordingly.

Comment 41

[David Zbarsky (:dzbarsky)]

Attachment: Part 2: Handle the autocomplete refreshed

Comment 42

[David Zbarsky (:dzbarsky)]

Attachment: Part 2.5: Combine JS implementations of nsIAutoCompleteResult into JS Module refreshed

Comment 43

[David Zbarsky (:dzbarsky)]

Attachment: Part 3: Dynamically update the popup when datalist changes refreshed

Comment 44

[David Zbarsky (:dzbarsky)]

Attachment: Part 4: update implementers of nsIAutoCompleteResult

Comment 45

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

See also http://www.w3.org/Bugs/Public/show_bug.cgi?id=9785

I think a reasonable default would be to apply filtering to the values coming from the <datalist>, but enable filtering to be disabled using a boolean attribute, possibly -moz- prefixed for now. We might want to do the attribute as a separate followup bug.

Comment 46

[Mounir Lamouri (:mounir)]

(In reply to comment #45)
> See also http://www.w3.org/Bugs/Public/show_bug.cgi?id=9785
> 
> I think a reasonable default would be to apply filtering to the values coming
> from the <datalist>, but enable filtering to be disabled using a boolean
> attribute, possibly -moz- prefixed for now. We might want to do the attribute
> as a separate followup bug.

Why would we want to show a suggestion that will not pass form validation? The only use case I can think of would be when a form use @novalidate. But having this parameter would let authors allow any value even those who will make the form invalid thus reducing the user experience.

Comment 47

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

What I said didn't relate to validation at all. What I suggest is:

Given the following markup:

<input list="suggestions">
<datalist id="suggestions">
  <option>alpha</option>
  <option>bravo</option>
  <option>charlie</option>
</datalist>

if the user types "al" we should only show "alpha" as suggestion. But for something like

<input list="suggestions">
<datalist -moz-show-all id="suggestions">
  <option>alpha</option>
  <option>American League</option>
  <option>Abraham Lincoln</option>
</datalist>

if the user types "al" we would display all three options as suggestions.


For markup like

<input list="suggestions" pattern="\d\d\d-\d\d\d-\d\d\d\d">
<datalist id="suggestions">
  <option>650-</option>
  <option>415-</option>
  <option>512-</option>
</datalist>

I think we should still display all three options, even though none of them would validate. If the page author provides those suggestions I don't think we should second-guess the author and remove some of his suggestions. (Until the user starts typing of course)

Comment 48

[Mounir Lamouri (:mounir)]

(In reply to comment #47)
> What I said didn't relate to validation at all. What I suggest is:
> 
> Given the following markup:
> 
> <input list="suggestions">
> <datalist id="suggestions">
>   <option>alpha</option>
>   <option>bravo</option>
>   <option>charlie</option>
> </datalist>
> 
> if the user types "al" we should only show "alpha" as suggestion. But for
> something like
> 
> <input list="suggestions">
> <datalist -moz-show-all id="suggestions">
>   <option>alpha</option>
>   <option>American League</option>
>   <option>Abraham Lincoln</option>
> </datalist>
> 
> if the user types "al" we would display all three options as suggestions.

Oups, I misinterpreted your comment. Sorry.

> For markup like
> 
> <input list="suggestions" pattern="\d\d\d-\d\d\d-\d\d\d\d">
> <datalist id="suggestions">
>   <option>650-</option>
>   <option>415-</option>
>   <option>512-</option>
> </datalist>
> 
> I think we should still display all three options, even though none of them
> would validate. If the page author provides those suggestions I don't think we
> should second-guess the author and remove some of his suggestions. (Until the
> user starts typing of course)

I think we should not show to the user an invalid value which may lead him to misunderstandings. The author may suggest values that he/she doesn't know (generating, xhr, etc.) then rely on UA's filtering. Maybe this could be disabled if <form> has @novalidate specified.

By the way, that is what the specifications are requesting at the moment:
"User agents must filter the suggestions to hide suggestions that the user would not be allowed to enter as the input  element's value, and should filter the suggestions to hide suggestions that would cause the element to not satisfy its constraints."

Comment 49

[David Zbarsky (:dzbarsky)]

Attachment: Part 5: Handle input/form autocomplete="off" and tests

Before, if the input or the form had autocomplete="off", no suggestions appeared.  Now, the datalist suggestions appear.

Comment 50

[David Zbarsky (:dzbarsky)]

Attachment: Fix dynamic updates of popup (previous attempt broke tests)

Comment 51

[Justin Dolske [:Dolske]]

Comment on attachment 460968 [details] [diff] [review]
Part 1: support for @list (r=sicking)

>+++ b/dom/interfaces/html/nsIDOMHTMLInputElement.idl

Nit: Your patch to this file is adding DOS-style line endings. The other parts of this patch are fine.

Comment 52

[Justin Dolske [:Dolske]]

Comment on attachment 462677 [details] [diff] [review]
Part 2: Handle the autocomplete refreshed

r- for general collection of nits below, but things seem well on the right track!


>+++ b/toolkit/components/autocomplete/public/nsIAutoCompleteResult.idl

Need to update the UIUD here since you're changing the interface.

Also flag this bug with "dev-doc-needed" keyword, so that we know to have DevMo updated with the new interface / use thereof.

>+  AString getLabelAt(in long index);

Probably ought to update nsIAutoCompleteController.idl, and add this method there too so they remain similar.


>+++ b/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp

> nsresult
>+nsAutoCompleteController::GetResultLabelAt(PRInt32 aIndex, PRBool aValueOnly, nsAString & _retval)

This is a clone of ::GetResultValueAt()...

Let's consolidate some of this -- and ::GetCommentAt(), ::GetStyleAt(), and ::GetImageAt() -- to a new ::GetResultAt() helper. ::GetResultValueAt() and ::GetResultLabelAt() just need to call it and do their |searchResult == nsIAutoCompleteResult::FOO| checking.


>+++ b/toolkit/components/autocomplete/src/nsAutoCompleteSimpleResult.cpp
> NS_IMETHODIMP
>+nsAutoCompleteSimpleResult::GetActualValueAt(PRInt32 aIndex, nsAString& _retval)
>+{
>+  return GetValueAt(aIndex, _retval);
>+}
>+
>+NS_IMETHODIMP

Hrm, you're adding this here, and then changing it Patch 2.5 to be ::GetLabelAt(). Please update the patches to avoid intermediate changes (unless it's unavoidable for the sake of splitting things into multiple pieces, but that doesn't seem to be the case here).


>+++ b/toolkit/components/satchel/public/nsIInputListAutoComplete.idl

This needs an MPL header... Just copy one from another IDL here and modify with:

 * The Original Code is input list autocomplete code.
 *
 * The Initial Developer of the Original Code is David Zbarsky.
 * Portions created by the Initial Developer are Copyright (C) 2010
 * the Initial Developer. All Rights Reserved.
 *
 * Contributor(s):
 *  David Zbarsky <[e-mail]> (original author)

(I think you can actually leave the Contributors bit off since you're the initial author, but it seems nice to have a single list of contributors already started.)


>+++ b/toolkit/components/satchel/src/nsFormAutoComplete.js
...
>+    getActualValueAt: function(index) {
>+        return getValueAt(index);
>+    },

Oddly, your later patches don't seem to change this to getLabelAt. Just do it here?

>+++ b/toolkit/components/satchel/src/nsFormFillController.cpp
...
>+    nsCOMPtr<nsIAutoCompleteResult> tempResult;

formHistoryResult


>+++ b/toolkit/components/satchel/src/nsInputListAutoComplete.js
...
>+InputListAutoComplete.prototype = {
>+    classID          : Components.ID("{bf1e01d0-953e-11df-981c-0800200c9a66}"),

You're using the same UUID here as in nsIInputListAutoComplete.idl... They're used for completely different things (so I don't think this technically breaks anything), but they should really be different. Change the IDL, since that UUID only exists in one place.

>+    QueryInterface   : XPCOMUtils.generateQI([Ci.nsIInputListAutoComplete, Ci.nsISupportsWeakReference]),

I don't think you need nsISupportsWeakReference here... Hmm, well, maybe you do. If everything works with out it, remove it. :)

>+    autoCompleteSearch : function (formHistoryResult, aUntrimmedSearchString, aField) {
>+        var comments = []; // "comments" column values for suggestions
>+        var comments = [];

You only need one of these. You only need one of these. ;)

Also, as they say, "let is the new var". s/var/let/ throughout this code.

>+        var displayValues = [];
>+        var actualValues = [];

just "values" and "labels"

>+        this.getSuggestions(aField, displayValues, actualValues);

getListSuggestions()?

Instead of passing in empty arrays, it would be clearer to do:

  let [values, labels] = this.getSuggestions(aField)

with

  function getSuggestions() {
      let foo = [], bar = [];
      ...
      return [foo, bar];
  }

>+        var historyResults = [];
>+        var historyComments = [];

historyValues (but see below).

Form history doesn't use comments, so historyComments should be nuked.


>+
>+        //formHistoryResult will be null if form autocomplete is disabled.  
>+        //We still want the list values to display.

Nit: space after "//".

>+        if (formHistoryResult) {
>+            entries = formHistoryResult.wrappedJSObject.entries;
>+            for (var i = 0; i < entries.length; ++i) {
>+                historyResults.push(entries[i].text);
>+                historyComments.push("");
>+            }
>+        }

Seems like it would be preferable to avoid slurping out all of formHistoryResults values, and instead stick it into SuggestAutoCompleteResult, and have it handle mapping the requested index X to formHistoryResult.getValueAt(Y) or the input's suggestion[Z].

>+        // fill out the comment column for the suggestions
>+        for (var i = 0; i < displayValues.length; ++i)
>+            comments.push("");

Just have getCommentAt() always return "", instead of creating an array.

>+
>+        // if we have any suggestions, put a label at the top
>+        if (comments.length > 0)
>+            comments[0] = "test";

Hmm, not sure we want to ship with "test" here. :)


>+        return new SuggestAutoCompleteResult(

So, one thing that's mildly surprising not to see here is any tracking/management of a previous autocomplete result, ala nsFormAutoComplete. I guess that's a side effect of how this currently does no filtering of the provided list attribute, and just always presents it as a static list?

I should probably look at this more in the next review pass...

>+    getSuggestions : function (aField, displayValues, actualValues) {
>+      
>+      if (!aField)
>+         return;

This can't happen, can it?


>+      for (var i=0; i < length; i++) {
>+          let item = options.item(i);
>+          if (item.label != "")
>+              displayValues.push(item.label);

That can just be |if (item.label)|.

>+          else if (item.text != "")
>+              displayValues.push(item.text);

Ditto

>+let component = [InputListAutoComplete];
>+var NSGetFactory = XPCOMUtils.generateNSGetFactory(component);

Nit: Move this to the bottom of the file.


>+SuggestAutoCompleteResult.prototype = {
...
>+  _displayValues: [],
>+  _actualValues: [],

Again, I think "values" and "labels" would be more consistent here.

Also, don't use [] in the prototype... Unless each instance sets this.foo to a new value, all the instances will share the same foo array and that's never what we'd want here... Just init these to [] in the constructor.

eg
  ...
  Blah.prototype = { foo: [], ...}
  let b1 = new Blah();
  let b2 = new Blah();
  b1.foo[0] = "ohnoes";
  b2.foo[0] == "ohnoes"; // <-- true!

better:
  function Blah() {
    this.foo = [];
  }


>+  getValueAt: function(index) {
>+    return this._displayValues[index];
>+  },

For this and all the other getFooAt() functions, you should check index bounds (obviously not for memory safety, but to preserve expected errors). Steal a copy of _checkIndexBounds() from nsFormAutoComplete.


>+  removeValueAt: function(index, removeFromDatabase) {
>+    // Forward the removeValueAt call to the underlying result if we have one
>+    // Note: this assumes that the form history results were added to the top
>+    // of our arrays.
>+    this._displayValues.splice(index, 1);
>+    this._actualValues.splice(index, 1);
>+    this._comments.splice(index, 1);
>+  },

This needs to forward on formHistoryResult when appropriate, so that form history entries can get deleted. [We have tests for that, is that changed in another patch in this series or were they failing?]

>+  // nsISupports
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIAutoCompleteResult])

Nit: Put this at/near the top of SuggestAutoCompleteResult.prototype.

Comment 53

[Justin Dolske [:Dolske]]

Comment on attachment 462678 [details] [diff] [review]
Part 2.5: Combine JS implementations of nsIAutoCompleteResult into JS Module refreshed

Clearing review, please merge with an updated Part 2 (including previous review comments for it).

>+++ b/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp
...
> NS_IMETHODIMP
> nsAutoCompleteController::GetValueAt(PRInt32 aIndex, nsAString & _retval)
> {
>-  GetResultValueAt(aIndex, PR_FALSE, _retval);
>+  GetResultLabelAt(aIndex, PR_FALSE, _retval);
> 
>   return NS_OK;
> }

Unless this ends up causing vast changes, seems like it would be good to bite the bullet and make callers properly use GetValueAt / GetLabelAt  depending on what they want. Maybe this is good enough for now, and we can just do that in a followup... Basically, I just want to avoid the surprise of calling GetValueAt() and getting labels when you might have wanted the actual values.

Addons might be the deciding factor here, but the addons mxr is down again, so I can't see if there are many implementations os nsIAutoCompleteResults / Controller there. Sigh.


>+++ b/toolkit/components/satchel/src/nsFormAutoCompleteResult.jsm

Oh, blargh, now I see that a lot of the nsInputListAutoComplete.js from the first patch seems to have been based on the existing nsSearchSuggestions.js, which is now moving this shared module. Sigh.

Update this with the comments that seem appropriate / worthy from the review of the last patch, and submit a new Part 2 that just directly jumps to using the .jsm (ie, merge with this patch). I'll do another review of that.

Comment 54

[Justin Dolske [:Dolske]]

Comment on attachment 462680 [details] [diff] [review]
Part 4: update implementers of nsIAutoCompleteResult


>+++ b/toolkit/components/places/src/nsTaggingService.js
...,
> 
>+  getLabelAt: function PTACR_getActualValueAt(index) {
>+    return this.getValueAt(index);
>+  },

PTACR_getLabelAt

r+ with that.

Comment 55

[Justin Dolske [:Dolske]]

I think the HTML5 spec needs to specifically address what's supposed to happen when |autocomplete=off| is used in conjunction with the |list| attribute.

Looks like we're assuming that with...

  <input list="suggestions" autocomplete="off">
  <datalist id="suggestions">
    <option>foo</option>
    <option>bar</option>
    <option>baz</option>
  </datalist>

...the UA should display an autocomplete dropdown offering only suggestions from the datalist, and not any other existing history (in addition to not saving the value in form history, which might be useful in some cases).

This seems entirely sensible, but I could see an alternate interpretation for having |autocomplete=off| mean "don't show the damn dropdown evar!", and the web mage will handle that UI itself. Using @list/<datalist> is a bit superfluous in that case, but hey why not.

Similarly, what happens when someone puts the |autocomplete=off| in the <form>, and later adds the input/datalist expecting it to just work?

Comment 56

[Justin Dolske [:Dolske]]

Comment on attachment 463546 [details] [diff] [review]
Part 5: Handle input/form autocomplete="off" and tests

r- here, but again just for a couple small things and generally looks good.

>+++ b/toolkit/components/satchel/src/nsFormFillController.cpp
...
>+    nsCOMPtr<nsIAutoCompleteResult> tempResult;
>+    if (!mDisableFormHistory) {
>+      nsCOMPtr <nsIFormAutoComplete> formAutoComplete =
>+        do_GetService("@mozilla.org/satchel/form-autocomplete;1", &rv);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      rv = formAutoComplete->AutoCompleteSearch(aSearchParam,
>+                                                aSearchString,
>+                                                mFocusedInput,
>+                                                aPreviousResult,
>+                                                getter_AddRefs(tempResult));

Why the mDisableFormHistory check? Looks like formAutoComplete->AutoCompleteSearch() will already just return null when history is disabled.

...oh, this is a new member you've added to preserve autocomplete=off state between calls.

Hmm, I'm wary of doing that because when the controller rebinds to other inputs, we'd have to make sure all that state is updated (and I don't think we're doing anything like that already). [This probably also breaks changing the value of autocomplete dynamically in some cases, although I dunno if that's actually useful.]

Instead of a mDisableFormHistory, make this a IsInputAutocompleteOff() call to calculate it on demand?

>-    PRBool isReadOnly = PR_FALSE;
>-    input->GetReadOnly(&isReadOnly);
>-                                  
>-    nsAutoString autocomplete; 
>-    input->GetAttribute(NS_LITERAL_STRING("autocomplete"), autocomplete);
>+  PRBool isReadOnly = PR_FALSE;
>+  input->GetReadOnly(&isReadOnly);
>+                                
>+  nsAutoString autocomplete; 
>+  input->GetAttribute(NS_LITERAL_STRING("autocomplete"), autocomplete);

There seems to be some bogus whitespace changes here, can you remove that from the updated patch?

It might make sense to just merge this down with the Part 2 patch, as well?


>+++ b/toolkit/components/satchel/test/test_form_autocomplete_with_list.html

Note to self: this test looks fine, but I'd like to make one more pass to see if there are any additional edgecasy things we should check.

Comment 57

[Justin Dolske [:Dolske]]

Comment on attachment 463826 [details] [diff] [review]
Fix dynamic updates of popup (previous attempt broke tests)


>+//// nsIMutationObserver
>+NS_IMPL_NSIMUTATIONOBSERVER_CORE_STUB(nsFormFillController)

Hmm. Generally mutation observers make Gecko all sad and grumpy, due to poor performance. But maybe it's perfectly fine in this case, since we're only adding a mutation observer when there's a focused text input?

Jonas, any opinion here?

Comment 58

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #57)
> Comment on attachment 463826 [details] [diff] [review]
> Fix dynamic updates of popup (previous attempt broke tests)
> 
> 
> >+//// nsIMutationObserver
> >+NS_IMPL_NSIMUTATIONOBSERVER_CORE_STUB(nsFormFillController)
> 
> Hmm. Generally mutation observers make Gecko all sad and grumpy, due to poor
> performance. But maybe it's perfectly fine in this case, since we're only
> adding a mutation observer when there's a focused text input?
> 
> Jonas, any opinion here?

nsIMutationObservers are fine, as long as they are used with care (don't put them higher up in the tree than needed etc). It's DOM Mutation Events that you want to avoid.

Comment 59

[David Zbarsky (:dzbarsky)]

Attachment: Merge of 2 and 2.5: Handle the autocomplete

Comment 60

[David Zbarsky (:dzbarsky)]

Attachment: Part 2: Handle the autocomplete

I merged parts 2, 2.5, 4, and 5 into this patch, because they are all needed in order for this to work correctly.  The dynamic updates is a separate part.

Comment 61

[David Zbarsky (:dzbarsky)]

Attachment: Part 3: dynamic updates

Comment 62

[David Zbarsky (:dzbarsky)]

Attachment: Part 2: Handle the autocomplete

update...

Comment 63

[David Zbarsky (:dzbarsky)]

Attachment: Part 3: dynamic updates

I had to update to fix a crash.  Apparently, when autocompleting for search suggestions, nsFormFillController's mFocusedInput is null.  Why is that?

Comment 64

[Justin Dolske [:Dolske]]

Comment on attachment 464581 [details] [diff] [review]
Part 2: Handle the autocomplete

Close but a few small things to fix up.

I also want to check (tomorrow, yawn) to make sure that the way nsFormFillController.cpp now controls inputs when autocomplete=off is present is ok in all the edge cases. It probably is, since it half ignores it already for password manager inputs, but I just want to make doubly sure.


>+++ b/toolkit/components/autocomplete/public/nsIAutoCompleteController.idl
>+++ b/toolkit/components/autocomplete/public/nsIAutoCompleteResult.idl

These interfaces are both changing, so you need to change the UUID at the top of the file. ("/msg firebot UUID" on IRC is a simple way to get a new UUID, but it's just a random number...)


>+++ b/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp
...
>nsresult
> nsAutoCompleteController::GetResultValueAt(PRInt32 aIndex, PRBool aValueOnly, nsAString & _retval)
> {

Please update this function to use the new GetResultAt(), just a small change...

It would still be nice to just consolidate GetResultValueAt and GetResultLabelAt to a single function, with an arg to control value vs label... Except for 1 line, these two functions do the same thing.


>+++ b/toolkit/components/satchel/src/nsFormAutoCompleteResult.jsm
...
>+  /**
>+   * The user's query string
>+   * @private
>+   */
>+  _searchString: "",

These (copied) comments are kind of gross... Reduce them down to just a single "// foo" comment when they don't need to be multiline (the @private can go too) [actually, see below first, these can just go away]. I don't think having JavaDoc comments is useful here, especially since the comments themselves are not terribly useful as documentation.

>+  /**
>+   * The list of words returned by the Suggest Service
>+   * @private
>+   */

This is no longer the Suggest Service. :)

+  _labels: [],
+  _values: [],
+  _comments: [],

As noted before, arrays in prototypes can be bad because they're shared. The constructor here is clearly overriding them, though, so just change the prototype too be "_foo : null". Or, better yet, just remove all these pointless entries in the prototype entirely, and let the constructor's this._foo take care of the whole thing. This avoids any confusion about them being default values, and the gross comments can just be deleted entirely.

>+};
>\ No newline at end of file

Nit!


>+++ b/toolkit/components/satchel/src/nsInputListAutoComplete.js
...
>+        if (formHistoryResult) {
>+            entries = formHistoryResult.wrappedJSObject.entries;
>+            for (let i = 0; i < entries.length; ++i) {
>+                historyResults.push(entries[i].text);
>+                historyComments.push("");
>+            }
>+        }

Sigh, I still want to avoid this copying of results, but I guess the nsSearchSuggestions has been doing this all along... At least it's avoiding XPCOM here.


>+        // fill out the comment column for the suggestions
>+        for (let i = 0; i < values.length; ++i)
>+            comments.push("");
>+
>+        // if we have any suggestions, put a label at the top
>+        if (comments.length > 0)
>+            comments[0] = "separator";

A bit less mind-bendy as:

  if (values.length)
    comments[0] = "separator";
  for (let i = 1; i < values.length; ++i)
    comments.push("");


>+        if (list) {
>+          let options = aField.list.options;
>+
>+          let length = options.length;
>+          for (let i = 0; i < length; i++) {

Nit: move blank line to before the for, or just drop it.


>+++ b/toolkit/components/satchel/test/test_form_autocomplete_with_list.html
...
>+  <datalist id="suggest">
>+    <option value="Google" label="PASS1">FAIL</option>
>+    <option value="Reddit">PASS2</option>
>+    <option value="final">
>+  </datalist>

Is the lack of </option> on the last entry intentional?

>+    function setForm(value) {
>+    input.value = value;
>+    input.focus();
>+    }

Nit: what happened to the indenting? Seems to be missing in a number of places here...

>+    switch(testNum) {
>+    case 1:

It would be worth adding a test somewhere here to ensure that deleting a form history entry (when a @list is also present) is correctly forwarded onto the DB. The existing test_form_autocomplete.html has the bits to do this, so it's an easy cut'n'paste.

Comment 65

[David Zbarsky (:dzbarsky)]

Attachment: Part 2 v2

Comment 66

[Justin Dolske [:Dolske]]

Comment on attachment 468176 [details] [diff] [review]
Part 2 v2

>+++ b/toolkit/components/satchel/src/nsFormFillController.cpp
...
>-    PRBool isReadOnly = PR_FALSE;
>-    input->GetReadOnly(&isReadOnly);
>-                                  
>+  PRBool isReadOnly = PR_FALSE;
>+  input->GetReadOnly(&isReadOnly);
>+                                
>+  nsAutoString autocomplete; 
>+  input->GetAttribute(NS_LITERAL_STRING("autocomplete"), autocomplete);
>+
>+  PRInt32 dummy;
>+  PRBool isPwmgrInput = PR_FALSE;
>+  if (mPwmgrInputs.Get(input, &dummy))
>+      isPwmgrInput = PR_TRUE;
>+
>+  nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(input);

Are there some stray whitespace changes confusing diff here? Looks like the only real change is after the last line I quoted here.


>+++ b/toolkit/components/satchel/src/nsInputListAutoComplete.js
...
>+    getListSuggestions : function (aField) {
...
>+        let list = aField.list;
>+        if (list) {
>+          let options = aField.list.options;

Nit: |let options = list.options|

Oh, well, |list| isn't used again, so then just |if (aField.list) {|, and drop |list|.

Comment 67

[Justin Dolske [:Dolske]]

Comment on attachment 464582 [details] [diff] [review]
Part 3: dynamic updates

Finally! Sorry this has taken so long to plod through. :(

>+++ b/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp
...
>+nsFormFillController::AttributeChanged(nsIDocument* aDocument, nsIContent* aContent, PRInt32 aNameSpaceID,
>+                                       nsIAtom* aAttribute, PRInt32 aModType)
>+{
>+  //if(aAttribute == nsGkAtoms::list)
>+    RevalidateDataList();

Nit: remove commented out code.

Also, looks like code added in Patch Part 3 isn't being tested by the tests in Patch Part 2 (ie, test_form_autocomplete_with_list.html). Should be simple to add to that test so it triggers the mutation observer stuff.

r+ with that!

Comment 68

[Justin Dolske [:Dolske]]

Attachment: Part 3.5 - unbitrot part 3

Looks like bug 562698 changed the nsIMutationObserver args slightly, this additional patch fixes it up for me.

Comment 69

[Boris Zbarsky [:bzbarsky]]

So what's the stats here?  There are patches, reviews, approvals....  Is this ready to land?  If so, are the attached patches what needs to land, or are there review nits that need dealing with first?

Comment 70

[Mounir Lamouri (:mounir)]

(In reply to comment #69)
> So what's the stats here?  There are patches, reviews, approvals....  Is this
> ready to land?  If so, are the attached patches what needs to land, or are
> there review nits that need dealing with first?

I fixed the nits and I will land that probably tonight.

Comment 71

[Mounir Lamouri (:mounir)]

I've open:
- bug 595067, because we want to have to list to be filtered by the current field value. The patch is mostly ready and we would like to have it for beta6.
- bug 595069, for the tests dolske mentioned in comment 67
- bug 595070, for the DOM tests

Comment 72

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/2dc709fe2fcf
http://hg.mozilla.org/mozilla-central/rev/e6cc9d708dbf
http://hg.mozilla.org/mozilla-central/rev/d26a99c48df0

I've applied requested changes.

Comment 73

[Jean-Yves Perrier [:teoli]]

I've updated 
https://developer.mozilla.org/en/HTML/Element/Input 

(@list was documented but marked as unimplemented)

and added a § about @list in : 

https://developer.mozilla.org/en/HTML/HTML5/Forms_in_HTML5#section_1

dev-doc-needed -> dev-doc-complete?

Comment 74

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We also need to document the <datalist> element.

Comment 75

[Jean-Yves Perrier [:teoli]]

(In reply to comment #74)
> We also need to document the <datalist> element.

Already done, see https://bugzilla.mozilla.org/show_bug.cgi?id=555840#c24

Comment 76

[Mounir Lamouri (:mounir)]

Eric, can you confirm that the doc has been done? (see comment 73)

Comment 77

[Eric Shepherd [:sheppy]]

This is indeed now done, documentation-wise.