Closed Bug 555840 Opened 14 years ago Closed 14 years ago

Implement datalist element

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b7

People

(Reporter: mounir, Assigned: mounir)

References

()

Details

(Keywords: dev-doc-complete, html5)

Attachments

(1 file, 7 obsolete files)

The datalist element is used to create a list of suggestions for an input using the list attribute.
Blocks: 556007
Attached patch Tests v1 (obsolete) — Splinter Review
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #436297 - Flags: review?(jonas)
Keywords: dev-doc-needed
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #436299 - Flags: review?
Attachment #436299 - Flags: review? → review?(jonas)
I've just realized there is a small typo in the beginning of the patch (nsGkAtomList.h). Please, consider it fixed ;)
Attachment #436299 - Flags: review?(mrbkap)
Attachment #436299 - Flags: review?(jonas)
Attachment #436299 - Flags: review-
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.
(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.
(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.
Attachment #436299 - Flags: review?(mrbkap)
(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.
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 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.
Attachment #436297 - Flags: review?(jonas)
(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?
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.
Attached patch Refreshed patch (obsolete) — Splinter Review
Attachment #436299 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #436297 - Attachment is obsolete: true
Attachment #458118 - Attachment is obsolete: true
Attachment #458335 - Flags: review?(jonas)
Attachment #458335 - Flags: review?(mrbkap)
Though I don't think we should check this in until <datalist> is actually implemented enough that we have some UI for it.
Attachment #458335 - Flags: review?(mrbkap) → review+
(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.
Attachment #458335 - Flags: superreview?(Olli.Pettay)
Attached patch Patch v2.1 (obsolete) — Splinter Review
Updated patch to make it compile after Olli's changes for createElement optimization.
Attachment #458335 - Attachment is obsolete: true
Attachment #460267 - Flags: superreview?(Olli.Pettay)
Attachment #458335 - Flags: superreview?(Olli.Pettay)
Comment on attachment 460267 [details] [diff] [review]
Patch v2.1


>+NS_IMETHODIMP nsHTMLDataListElement::GetOptions(nsIDOMHTMLCollection** aOptions)
Nit, newline after NS_IMETHODIMP
Attachment #460267 - Flags: superreview?(Olli.Pettay) → superreview+
Blocks: 582248
Attached patch Patch v2.2 (obsolete) — Splinter Review
r=mrbkap,sicking sr=smaug
Attachment #460267 - Attachment is obsolete: true
Attached patch Patch v2.3 (obsolete) — Splinter Review
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 ;)
Attachment #460573 - Attachment is obsolete: true
Also, you forgot to add datalist to /editor/libeditor/html/nsHTMLEditUtils.cpp.  I'll upload a patch with that in a bit.
Attached patch Patch v2.4Splinter Review
I've added datalist to the editor and to a test.
Ehsan, could you review the changes in the editor?
Attachment #463566 - Attachment is obsolete: true
Attachment #463786 - Flags: review?(ehsan)
Comment on attachment 463786 [details] [diff] [review]
Patch v2.4

r=ehsan on the editor/ changes.
Attachment #463786 - Flags: review?(ehsan) → review+
Pushed:
http://hg.mozilla.org/mozilla-central/rev/43ae28d1240d
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
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?
Yup, doc complete. You're my hero. :)
No longer blocks: 582248
Blocks: 595282
Blocks: 595457
Depends on: 596422
Blocks: 639656
Depends on: 720385
You need to log in before you can comment on or make changes to this bug.