Implement datalist element

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Blocks: 1 bug, {dev-doc-complete, html5})

Trunk
mozilla2.0b7
dev-doc-complete, html5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

7 years ago
The datalist element is used to create a list of suggestions for an input using the list attribute.
(Assignee)

Updated

7 years ago
Blocks: 556007
(Assignee)

Comment 1

7 years ago
Created attachment 436297 [details] [diff] [review]
Tests v1
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #436297 - Flags: review?(jonas)
(Assignee)

Updated

7 years ago
Keywords: dev-doc-needed
(Assignee)

Comment 2

7 years ago
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.
Attachment #436299 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #436299 - Flags: review? → review?(jonas)
(Assignee)

Comment 3

7 years ago
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.
(Assignee)

Comment 5

7 years ago
(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.

Updated

7 years ago
Blocks: 559766
(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.

Updated

7 years ago
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?
(Assignee)

Comment 11

7 years ago
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.
Created attachment 458118 [details] [diff] [review]
Refreshed patch
Attachment #436299 - Attachment is obsolete: true
(Assignee)

Comment 13

7 years ago
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.
Attachment #436297 - Attachment is obsolete: true
Attachment #458118 - Attachment is obsolete: true
Attachment #458335 - Flags: review?(jonas)
(Assignee)

Updated

7 years ago
Attachment #458335 - Flags: review?(mrbkap)
Attachment #458335 - Flags: review?(jonas) → review+
Though I don't think we should check this in until <datalist> is actually implemented enough that we have some UI for it.

Updated

7 years ago
Attachment #458335 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 15

7 years ago
(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.
(Assignee)

Updated

7 years ago
Attachment #458335 - Flags: superreview?(Olli.Pettay)
(Assignee)

Comment 16

7 years ago
Created attachment 460267 [details] [diff] [review]
Patch v2.1

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
(Assignee)

Comment 18

7 years ago
Created attachment 460573 [details] [diff] [review]
Patch v2.2

r=mrbkap,sicking sr=smaug
Attachment #460267 - Attachment is obsolete: true
(Assignee)

Comment 19

7 years ago
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 ;)
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.
(Assignee)

Comment 21

7 years ago
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?
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+
Attachment #463786 - Flags: approval2.0+
(Assignee)

Comment 23

7 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/43ae28d1240d
Status: ASSIGNED → RESOLVED
Last Resolved: 7 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. :)
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Updated

7 years ago
No longer blocks: 582248
(Assignee)

Updated

7 years ago
Blocks: 595282

Updated

7 years ago
Blocks: 595457
(Assignee)

Updated

7 years ago
Depends on: 596422
(Assignee)

Updated

6 years ago
Blocks: 639656
Depends on: 720385
You need to log in before you can comment on or make changes to this bug.