Last Comment Bug 555840 - Implement datalist element
: Implement datalist element
Status: RESOLVED FIXED
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla2.0b7
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
http://dev.w3.org/html5/spec/forms.ht...
Depends on: 596422 720385
Blocks: html5forms 444851 556007 559766 595282 595457 639656
  Show dependency treegraph
 
Reported: 2010-03-29 14:53 PDT by Mounir Lamouri (:mounir)
Modified: 2013-05-07 14:58 PDT (History)
16 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Tests v1 (5.62 KB, patch)
2010-03-31 14:23 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Review
Patch v1 (18.97 KB, patch)
2010-03-31 14:29 PDT, Mounir Lamouri (:mounir)
jonas: review-
Details | Diff | Review
Refreshed patch (18.59 KB, patch)
2010-07-17 14:31 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Patch v2 (22.13 KB, patch)
2010-07-19 07:15 PDT, Mounir Lamouri (:mounir)
jonas: review+
mrbkap: review+
Details | Diff | Review
Patch v2.1 (22.22 KB, patch)
2010-07-26 09:36 PDT, Mounir Lamouri (:mounir)
bugs: superreview+
Details | Diff | Review
Patch v2.2 (22.22 KB, patch)
2010-07-27 10:33 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Review
Patch v2.3 (23.33 KB, patch)
2010-08-06 09:10 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Review
Patch v2.4 (28.75 KB, patch)
2010-08-07 05:48 PDT, Mounir Lamouri (:mounir)
ehsan: review+
roc: approval2.0+
Details | Diff | Review

Description Mounir Lamouri (:mounir) 2010-03-29 14:53:19 PDT
The datalist element is used to create a list of suggestions for an input using the list attribute.
Comment 1 Mounir Lamouri (:mounir) 2010-03-31 14:23:21 PDT
Created attachment 436297 [details] [diff] [review]
Tests v1
Comment 2 Mounir Lamouri (:mounir) 2010-03-31 14:29:07 PDT
Created attachment 436299 [details] [diff] [review]
Patch v1

The datalist element is only working correctly with html5 parser but I've been told we don't really care about the old parser... even more because this an html5 element.
Comment 3 Mounir Lamouri (:mounir) 2010-04-01 08:44:46 PDT
I've just realized there is a small typo in the beginning of the patch (nsGkAtomList.h). Please, consider it fixed ;)
Comment 4 Jonas Sicking (:sicking) 2010-04-15 13:58:33 PDT
Comment on attachment 436299 [details] [diff] [review]
Patch v1

Is there really a point in checking in this amount of support for <datalist>? I don't see much reason to until we have the UI hooked up.

I'm totally fine with having separate bugs for the rest of the support if you prefer. But I'm wary of checking this in until the whole thing is there. At the very least until we know for sure that we'll be able to complete the rest before shipping.

>diff -r be033fe3031c content/base/src/nsGkAtomList.h
>+GK_ATOM(dataliste, "datalist")

There's an extra 'e' here.

>diff -r be033fe3031c content/html/content/src/nsHTMLDataListElement.cpp

>+  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED_NO_UNLINK(nsHTMLDataListElement,
>+                                                     nsGenericHTMLElement)
>+protected:
>+
>+  nsRefPtr<nsContentList> mOptions;

You should probably unlink mOptions.

>+nsHTMLDataListElement::MatchOptions(nsIContent* aContent, PRInt32 aNamespaceID,

>+  if (ni->NamespaceID() != aNamespaceID ||
>+      ni->NameAtom() != nsGkAtoms::option) {
>+    return PR_FALSE;
>+  }

aNamespaceID is always going to be the XHTML namespace, so you might as well test for that here directly. And not pass in the namespace in the ctor of course.

Also, use nsINodeInfo::Equals(nsIAtom*, PRInt32)

>+  // Getting the value directly from nsIDOMHTMLOptionElement with QI
>+  // so we do not have to guess how the value is managed internally.
>+  nsAutoString value;
>+  nsCOMPtr<nsIDOMHTMLOptionElement> option(do_QueryInterface(aContent));
>+  option->GetValue(value);
>+  return !value.IsEmpty();

Hmm.. this is sort of a problem. I don't think we'll call in to the MatchOptions function whenever the textnode inside the <option> element changes. That needs to be fixed in order to ensure that the list is always up to date.

Though really, it seems like the perf hit being taken here is not worth it. IMHO the spec shouldn't require that empty options are removed from the list, even though removing them from the UI seems like a good idea. I think always taking a perf-hit for something that will happen as rarely as empty <option>s, is a bad tradeoff.

>diff -r be033fe3031c layout/style/forms.css
>+datalist {
>+  display: none !important;
>+}

This shouldn't be !important.

http://www.whatwg.org/specs/web-apps/current-work/?slow-browser#display-types

>b/parser/htmlparser/src/nsElementTable.cpp
>+  // XXX: datalist tag doesn't have to behave correctly with this old parser.
>+  // This code is only here to make sur the parser is still compiling.
>+  {
>+    /*tag*/                             eHTMLTag_datalist,
>+    /*requiredAncestor*/                eHTMLTag_unknown, eHTMLTag_unknown,
>+    /*rootnodes,endrootnodes*/          &gRootTags,&gRootTags,
>+    /*autoclose starttags and endtags*/ 0,0,0,0,
>+    /*parent,incl,exclgroups*/          kNone, kNone, kNone,
>+    /*special props, prop-range*/       0,0,
>+    /*special parents,kids*/            0,&gContainsOpts,
>+  },

Actually, would love to have mrbkap look at this part. I agree it doesn't matter much that things work correctly in the old parser. But at least I want to avoid exploitable crashes.

Also, if we easily can get it to work in the old parser, then we might as well.
Comment 5 Mounir Lamouri (:mounir) 2010-04-15 16:34:49 PDT
(In reply to comment #4)
> (From update of attachment 436299 [details] [diff] [review])
> Is there really a point in checking in this amount of support for <datalist>? I
> don't see much reason to until we have the UI hooked up.
> 
> I'm totally fine with having separate bugs for the rest of the support if you
> prefer. But I'm wary of checking this in until the whole thing is there. At the
> very least until we know for sure that we'll be able to complete the rest
> before shipping.

I understand so I will probably come back to this patch later when @list will be in the top of my todo list.

> >diff -r be033fe3031c content/html/content/src/nsHTMLDataListElement.cpp
> 
> >+  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED_NO_UNLINK(nsHTMLDataListElement,
> >+                                                     nsGenericHTMLElement)
> >+protected:
> >+
> >+  nsRefPtr<nsContentList> mOptions;
> 
> You should probably unlink mOptions.

What do you mean by 'unlink' ?

> >+nsHTMLDataListElement::MatchOptions(nsIContent* aContent, PRInt32 aNamespaceID,
> 
> >+  if (ni->NamespaceID() != aNamespaceID ||
> >+      ni->NameAtom() != nsGkAtoms::option) {
> >+    return PR_FALSE;
> >+  }
> 
> aNamespaceID is always going to be the XHTML namespace, so you might as well
> test for that here directly. And not pass in the namespace in the ctor of
> course.
> 
> Also, use nsINodeInfo::Equals(nsIAtom*, PRInt32)

Ok.

> >+  // Getting the value directly from nsIDOMHTMLOptionElement with QI
> >+  // so we do not have to guess how the value is managed internally.
> >+  nsAutoString value;
> >+  nsCOMPtr<nsIDOMHTMLOptionElement> option(do_QueryInterface(aContent));
> >+  option->GetValue(value);
> >+  return !value.IsEmpty();
> 
> Hmm.. this is sort of a problem. I don't think we'll call in to the
> MatchOptions function whenever the textnode inside the <option> element
> changes. That needs to be fixed in order to ensure that the list is always up
> to date.
> 
> Though really, it seems like the perf hit being taken here is not worth it.
> IMHO the spec shouldn't require that empty options are removed from the list,
> even though removing them from the UI seems like a good idea. I think always
> taking a perf-hit for something that will happen as rarely as empty <option>s,
> is a bad tradeoff.

Indeed, I realized the option list was not updated while reading your review.
Actually, I think we can minimize the perf-hit and update the list with nsIMutationObserver, right ?

> >diff -r be033fe3031c layout/style/forms.css
> >+datalist {
> >+  display: none !important;
> >+}
> 
> This shouldn't be !important.
> 
> http://www.whatwg.org/specs/web-apps/current-work/?slow-browser#display-types

Didn't know this link, thanks :)

> >b/parser/htmlparser/src/nsElementTable.cpp
> >+  // XXX: datalist tag doesn't have to behave correctly with this old parser.
> >+  // This code is only here to make sur the parser is still compiling.
> >+  {
> >+    /*tag*/                             eHTMLTag_datalist,
> >+    /*requiredAncestor*/                eHTMLTag_unknown, eHTMLTag_unknown,
> >+    /*rootnodes,endrootnodes*/          &gRootTags,&gRootTags,
> >+    /*autoclose starttags and endtags*/ 0,0,0,0,
> >+    /*parent,incl,exclgroups*/          kNone, kNone, kNone,
> >+    /*special props, prop-range*/       0,0,
> >+    /*special parents,kids*/            0,&gContainsOpts,
> >+  },
> 
> Actually, would love to have mrbkap look at this part. I agree it doesn't
> matter much that things work correctly in the old parser. But at least I want
> to avoid exploitable crashes.
> 
> Also, if we easily can get it to work in the old parser, then we might as well.
Comment 6 Blake Kaplan (:mrbkap) (please use needinfo!) 2010-04-20 16:21:40 PDT
(In reply to comment #5)
> > Actually, would love to have mrbkap look at this part. I agree it doesn't
> > matter much that things work correctly in the old parser. But at least I want
> > to avoid exploitable crashes.

IMO we should just make the datalist element follow the span model. I think that the way you have things set up here, you might accidentally trigger hangs; though I'm not sure.
Comment 7 Jonas Sicking (:sicking) 2010-04-21 13:35:58 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 436299 [details] [diff] [review] [details])
> > Is there really a point in checking in this amount of support for <datalist>? I
> > don't see much reason to until we have the UI hooked up.
> > 
> > I'm totally fine with having separate bugs for the rest of the support if you
> > prefer. But I'm wary of checking this in until the whole thing is there. At the
> > very least until we know for sure that we'll be able to complete the rest
> > before shipping.
> 
> I understand so I will probably come back to this patch later when @list will
> be in the top of my todo list.

Sounds good.

> > >diff -r be033fe3031c content/html/content/src/nsHTMLDataListElement.cpp
> > 
> > >+  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED_NO_UNLINK(nsHTMLDataListElement,
> > >+                                                     nsGenericHTMLElement)
> > >+protected:
> > >+
> > >+  nsRefPtr<nsContentList> mOptions;
> > 
> > You should probably unlink mOptions.
> 
> What do you mean by 'unlink' ?

See the macro you are using, it has 'unlink' in the name :)

I was trying to find a similar example where we don't just traverse, but also unlink. However every place I find doesn't unlink. Not sure why that is. cc'ing peterv who might know


> > >+  // Getting the value directly from nsIDOMHTMLOptionElement with QI
> > >+  // so we do not have to guess how the value is managed internally.
> > >+  nsAutoString value;
> > >+  nsCOMPtr<nsIDOMHTMLOptionElement> option(do_QueryInterface(aContent));
> > >+  option->GetValue(value);
> > >+  return !value.IsEmpty();
> > 
> > Hmm.. this is sort of a problem. I don't think we'll call in to the
> > MatchOptions function whenever the textnode inside the <option> element
> > changes. That needs to be fixed in order to ensure that the list is always up
> > to date.
> > 
> > Though really, it seems like the perf hit being taken here is not worth it.
> > IMHO the spec shouldn't require that empty options are removed from the list,
> > even though removing them from the UI seems like a good idea. I think always
> > taking a perf-hit for something that will happen as rarely as empty <option>s,
> > is a bad tradeoff.
> 
> Indeed, I realized the option list was not updated while reading your review.
> Actually, I think we can minimize the perf-hit and update the list with
> nsIMutationObserver, right ?

That's still a perf hit though. And it means that you can't use nsContentList nearly as automatically, so you likely won't be able to get as good perf without a lot of code to handle the various cases as well as nsContentList does.

I really don't see a reason to add the complexity and perf hit. Sure, we could do it, but what's the point? It's all to deal with an edge case that most pages likely will never hit, and it's only when they do something silly.

I think it'll be much better use of code and time to ask the spec to be changed.
Comment 8 Jonas Sicking (:sicking) 2010-04-21 13:52:04 PDT
Ok, I conferred with Peter and we should unlink here. The macro you should use is 

NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR

Examples how to use it:
http://mxr.mozilla.org/mozilla-central/ident?i=NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR
Comment 9 Jonas Sicking (:sicking) 2010-04-21 14:53:17 PDT
Comment on attachment 436297 [details] [diff] [review]
Tests v1

resetting review request on this given that it might change depending on if the spec changes (right?)

Feel free to rerequest if you prefer.
Comment 10 :Ms2ger 2010-07-17 10:00:21 PDT
(In reply to comment #4)
> Though really, it seems like the perf hit being taken here is not worth it.
> IMHO the spec shouldn't require that empty options are removed from the list,
> even though removing them from the UI seems like a good idea. I think always
> taking a perf-hit for something that will happen as rarely as empty <option>s,
> is a bad tradeoff.

Did anyone file a spec bug about this?
Comment 11 Mounir Lamouri (:mounir) 2010-07-17 10:08:08 PDT
Yes, here: http://www.w3.org/Bugs/Public/show_bug.cgi?id=9624
But we got no answer...

Sorry for not pasting the url before.
Comment 12 David Zbarsky (:dzbarsky) 2010-07-17 14:31:57 PDT
Created attachment 458118 [details] [diff] [review]
Refreshed patch
Comment 13 Mounir Lamouri (:mounir) 2010-07-19 07:15:51 PDT
Created attachment 458335 [details] [diff] [review]
Patch v2

This patch is including the changes we've requested to the specs.

I don't remember why I was checking for <option> namespace. I guess it was for a valid reason (except if it was requested by the specs before and it has been removed?). Anyway, I kept that.

Blake, I've changed <datalist> declaration for the old html parser but I'm wondering if we should add |gContainsOpts| and things like that or if that do not worth it?

David, I think you can use this version for your work on @list.
Comment 14 Jonas Sicking (:sicking) 2010-07-19 14:15:26 PDT
Though I don't think we should check this in until <datalist> is actually implemented enough that we have some UI for it.
Comment 15 Mounir Lamouri (:mounir) 2010-07-19 14:45:54 PDT
(In reply to comment #14)
> Though I don't think we should check this in until <datalist> is actually
> implemented enough that we have some UI for it.

Sure. The idea is just to have something done so David can work on @list.
Comment 16 Mounir Lamouri (:mounir) 2010-07-26 09:36:59 PDT
Created attachment 460267 [details] [diff] [review]
Patch v2.1

Updated patch to make it compile after Olli's changes for createElement optimization.
Comment 17 Olli Pettay [:smaug] 2010-07-26 13:22:00 PDT
Comment on attachment 460267 [details] [diff] [review]
Patch v2.1


>+NS_IMETHODIMP nsHTMLDataListElement::GetOptions(nsIDOMHTMLCollection** aOptions)
Nit, newline after NS_IMETHODIMP
Comment 18 Mounir Lamouri (:mounir) 2010-07-27 10:33:21 PDT
Created attachment 460573 [details] [diff] [review]
Patch v2.2

r=mrbkap,sicking sr=smaug
Comment 19 Mounir Lamouri (:mounir) 2010-08-06 09:10:52 PDT
Created attachment 463566 [details] [diff] [review]
Patch v2.3

It looks like the reftests took some vacation. I found them on the beach and they are now back in the patch. Thanks David for the denouncement ;)
Comment 20 David Zbarsky (:dzbarsky) 2010-08-06 18:21:07 PDT
Also, you forgot to add datalist to /editor/libeditor/html/nsHTMLEditUtils.cpp.  I'll upload a patch with that in a bit.
Comment 21 Mounir Lamouri (:mounir) 2010-08-07 05:48:54 PDT
Created attachment 463786 [details] [diff] [review]
Patch v2.4

I've added datalist to the editor and to a test.
Ehsan, could you review the changes in the editor?
Comment 22 :Ehsan Akhgari (busy, don't ask for review please) 2010-08-07 07:23:34 PDT
Comment on attachment 463786 [details] [diff] [review]
Patch v2.4

r=ehsan on the editor/ changes.
Comment 23 Mounir Lamouri (:mounir) 2010-09-10 02:23:42 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/43ae28d1240d
Comment 24 Jean-Yves Perrier [:teoli] 2010-09-10 04:13:30 PDT
I've created both
https://developer.mozilla.org/en/HTML/Element/datalist for the element, and
https://developer.mozilla.org/en/DOM/HTMLDataListElement for the DOM Interface.

I've also created a new section at
https://developer.mozilla.org/en/HTML/HTML5/Forms_in_HTML5#section_2

dev-doc-needed -> dev-doc-complete?
Comment 25 Eric Shepherd [:sheppy] 2010-09-10 11:05:42 PDT
Yup, doc complete. You're my hero. :)

Note You need to log in before you can comment on or make changes to this bug.