Closed
Bug 555840
Opened 14 years ago
Closed 14 years ago
Implement datalist element
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla2.0b7
People
(Reporter: mounir, Assigned: mounir)
References
()
Details
(Keywords: dev-doc-complete, html5)
Attachments
(1 file, 7 obsolete files)
28.75 KB,
patch
|
ehsan.akhgari
:
review+
roc
:
approval2.0+
|
Details | Diff | Splinter Review |
The datalist element is used to create a list of suggestions for an input using the list attribute.
Assignee | ||
Comment 1•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 2•14 years ago
|
||
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•14 years ago
|
Attachment #436299 -
Flags: review? → review?(jonas)
Assignee | ||
Comment 3•14 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•14 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.
Comment 6•14 years ago
|
||
(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•14 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)
Comment 10•14 years ago
|
||
(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•14 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.
Comment 12•14 years ago
|
||
Attachment #436299 -
Attachment is obsolete: true
Assignee | ||
Comment 13•14 years ago
|
||
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•14 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•14 years ago
|
Attachment #458335 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 15•14 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•14 years ago
|
Attachment #458335 -
Flags: superreview?(Olli.Pettay)
Assignee | ||
Comment 16•14 years ago
|
||
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 17•14 years ago
|
||
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+
Assignee | ||
Comment 18•14 years ago
|
||
r=mrbkap,sicking sr=smaug
Attachment #460267 -
Attachment is obsolete: true
Assignee | ||
Comment 19•14 years ago
|
||
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
Comment 20•14 years ago
|
||
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•14 years ago
|
||
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 22•14 years ago
|
||
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•14 years ago
|
||
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
Comment 24•14 years ago
|
||
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•14 years ago
|
||
Yup, doc complete. You're my hero. :)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•