Closed Bug 779723 Opened 12 years ago Closed 10 years ago

Implement RadioNodeList

Categories

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

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: dhtmlkitchen, Assigned: djc, Mentored)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [lang=c++])

Attachments

(1 file, 5 obsolete files)

Implement RadioNodeList from HTML5 spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#radionodelist The most desirable feature here is knowing which radio button is selected: radioNodeList . value [ = value ] And the problem is that you need a loop and it's ugly. But nowadays loops ain't cool and jQuery is cool so you may see some examples all over the place (stackoverflow, etc) like this:- $("input[name='tab']:checked").val() . That came from the example linked from here: http://hacks.mozilla.org/2011/01/html5guitar/
I just ran into an issue with this, where another developer wrote some code relying on .value to work here, and it didn't work in Firefox.
Component: General → DOM
Product: Firefox → Core
Whiteboard: [mentor=bzbarsky@mit.edu][lang=c++]
So generally speaking the way to fix this is: 1) Create a subclass of nsSimpleContentList that implements the RadioNodeList interface. 2) Change the code in HTMLFormElement::AddElementToTableInternal to create an instance of that subclass, not nsSimpleContentList.
Yeah, I'm going to probably need something a little bit more detailed than that. So, we have nsSimpleContentList in content/base/src/nsContentList.h. HTMLFormElement lives in content/html/src/HTMLFormElement.h. Do I create HTMLRadioNodeList.{cpp,h} in content/html/src/? I can't find any nsIDOMHTMLFormControlsCollection.idl even though I can find HTMLFormControlsCollection.h, why's that? Would RadioNodeList need IDL added somewhere? Does RadioNodeList just need, say, a public nsTArray<HTMLInputElement*> mElements and something for the value thing? Supposedly there need to be a getter and a setter that just iterate over the mElements and then do their thing? Can you point me to any good examples of how to build getters/setters?
Flags: needinfo?(bzbarsky)
Sorry I wasn't cced. :( > Do I create HTMLRadioNodeList.{cpp,h} in content/html/src/? Probably create RadioNodeList.{cpp,h} in content/html/content/src. > I can't find any nsIDOMHTMLFormControlsCollection.idl even though I can find > HTMLFormControlsCollection.h, why's that? Because this object is on WebIDL bindings, so it doesn't need a .idl. > Would RadioNodeList need IDL added somewhere? It would need a .webidl file added. RadioNodeList.webidl in dom/webidl, containing the interface definition from http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#htmlformcontrolscollection-0 You'll also want to remove the existing typedef in HTMLFormControlsCollection.webidl. > Does RadioNodeList just need, say, a public nsTArray<HTMLInputElement*> mElements and > something for the value thing? It should inherit from nsSimpleContentList, so shouldn't need any data members. It _will_ need to override the WrapObject() method to create a JS object that's an instance of RadioNodeList. It'll also need a getter and setter for "value". https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#DOMString has an example of how you'd declare those in the C++.
Flags: needinfo?(bzbarsky)
Okay, so I have this skeleton kind of setup, please check for stupidity. Trying to compile this, I get a bunch of crap... I'm afraid I need some more help, here. 8:05.00 In file included from /Users/djc/src/mozilla-central/obj-ff-dbg/dom/bindings/UnifiedBindings10.cpp:162: 8:05.00 ./HTMLFormControlsCollectionBinding.cpp:30:9: error: no matching member function for call to 'NamedItem' 8:05.00 self->NamedItem(NonNullHelper(Constify(arg0)), result); 8:05.00 ~~~~~~^~~~~~~~~ 8:05.00 ../../dist/include/mozilla/dom/HTMLFormControlsCollection.h:38:3: note: candidate function not viable: no known conversion from 'Nullable<mozilla::dom::OwningRadioNodeListOrElement>' to 'nsIDOMNode **' for 2nd argument Not sure why my changes cause failures in HTMLFormControlsCollectionBinding.cpp. Is it because I removed the RadioNodeList typedef? Do I have to add some kind of link back? 8:05.00 NS_DECL_NSIDOMHTMLCOLLECTION 8:05.00 ^ 8:05.00 ../../dist/include/nsIDOMHTMLCollection.h:47:14: note: expanded from macro 'NS_DECL_NSIDOMHTMLCOLLECTION' 8:05.00 NS_IMETHOD NamedItem(const nsAString & name, nsIDOMNode * *_retval); 8:05.00 ^ 8:05.00 ../../dist/include/mozilla/dom/HTMLFormControlsCollection.h:51:3: note: candidate function not viable: no known conversion from 'Nullable<mozilla::dom::OwningRadioNodeListOrElement>' to 'Nullable<mozilla::dom::OwningNodeListOrElement> &' for 2nd argument 8:05.00 NamedItem(const nsAString& aName, 8:05.00 ^ This seems sort of legit. Do I have to add some macros to RadioNodeList.h or something? Not sure what to do with these: 8:05.00 In file included from /Users/djc/src/mozilla-central/obj-ff-dbg/dom/bindings/UnifiedBindings10.cpp:2: 8:05.00 In file included from ./HTMLDListElementBinding.cpp:3: 8:05.00 In file included from ../../dist/include/mozilla/dom/HTMLDListElementBinding.h:10: 8:05.00 ../../dist/include/mozilla/dom/Nullable.h:25:5: error: field has incomplete type 'mozilla::dom::OwningNodeListOrElement' 8:05.00 T mValue; 8:05.00 ^ 8:05.00 ./HTMLFormControlsCollectionBinding.cpp:275:61: note: in instantiation of template class 'mozilla::dom::Nullable<mozilla::dom::OwningNodeListOrElement>' requested here 8:05.00 self->NamedGetter(NonNullHelper(Constify(name)), found, result); 8:05.00 ^ 8:05.00 ../../dist/include/mozilla/dom/HTMLFormControlsCollection.h:23:7: note: forward declaration of 'mozilla::dom::OwningNodeListOrElement' 8:05.00 class OwningNodeListOrElement; 8:05.00 ^ 8:05.00 ../../dist/include/mozilla/dom/Nullable.h:104:3: note: candidate template ignored: could not match 'nsTArray<type-parameter-0-0>' against 'mozilla::dom::OwningNodeListOrElement' 8:05.00 operator const Nullable< nsTArray<U> >&() const { 8:05.00 ^ 8:05.00 ../../dist/include/mozilla/dom/Nullable.h:111:3: note: candidate template ignored: could not match 'FallibleTArray<type-parameter-0-0>' against 'mozilla::dom::OwningNodeListOrElement' 8:05.00 operator const Nullable< FallibleTArray<U> >&() const { 8:05.00 ^ 8:05.00 ../../dist/include/mozilla/dom/HTMLFormControlsCollection.h:49:50: note: passing argument to parameter 'aResult' here 8:05.00 Nullable<OwningNodeListOrElement>& aResult); 8:05.00 ^ 8:05.01 In file included from /Users/djc/src/mozilla-central/obj-ff-dbg/dom/bindings/UnifiedBindings10.cpp:162: 8:05.01 ./HTMLFormControlsCollectionBinding.cpp:317:59: error: no viable conversion from 'Nullable<class mozilla::dom::OwningRadioNodeListOrElement>' to 'Nullable<class mozilla::dom::OwningNodeListOrElement>' 8:05.01 self->NamedGetter(NonNullHelper(Constify(name)), found, result); 8:05.01 ^~~~~~ 8:05.01 ../../dist/include/mozilla/dom/Nullable.h:104:3: note: candidate template ignored: could not match 'nsTArray<type-parameter-0-0>' against 'mozilla::dom::OwningNodeListOrElement' 8:05.01 operator const Nullable< nsTArray<U> >&() const { 8:05.01 ^ 8:05.01 ../../dist/include/mozilla/dom/Nullable.h:111:3: note: candidate template ignored: could not match 'FallibleTArray<type-parameter-0-0>' against 'mozilla::dom::OwningNodeListOrElement' 8:05.01 operator const Nullable< FallibleTArray<U> >&() const { 8:05.01 ^ 8:05.01 ../../dist/include/mozilla/dom/HTMLFormControlsCollection.h:49:50: note: passing argument to parameter 'aResult' here 8:05.01 Nullable<OwningNodeListOrElement>& aResult); 8:05.01 ^ 8:05.01 In file included from /Users/djc/src/mozilla-central/obj-ff-dbg/dom/bindings/UnifiedBindings10.cpp:162: 8:05.01 ./HTMLFormControlsCollectionBinding.cpp:402:61: error: no viable conversion from 'Nullable<class mozilla::dom::OwningRadioNodeListOrElement>' to 'Nullable<class mozilla::dom::OwningNodeListOrElement>' 8:05.01 self->NamedGetter(NonNullHelper(Constify(name)), found, result); 8:05.01 ^~~~~~ 8:05.01 ../../dist/include/mozilla/dom/Nullable.h:104:3: note: candidate template ignored: could not match 'nsTArray<type-parameter-0-0>' against 'mozilla::dom::OwningNodeListOrElement' 8:05.01 operator const Nullable< nsTArray<U> >&() const { 8:05.01 ^ 8:05.01 ../../dist/include/mozilla/dom/Nullable.h:111:3: note: candidate template ignored: could not match 'FallibleTArray<type-parameter-0-0>' against 'mozilla::dom::OwningNodeListOrElement' 8:05.01 operator const Nullable< FallibleTArray<U> >&() const { 8:05.01 ^ 8:05.01 ../../dist/include/mozilla/dom/HTMLFormControlsCollection.h:49:50: note: passing argument to parameter 'aResult' here 8:05.01 Nullable<OwningNodeListOrElement>& aResult); 8:05.01 ^ 8:05.01 In file included from /Users/djc/src/mozilla-central/obj-ff-dbg/dom/bindings/UnifiedBindings10.cpp:162: 8:05.01 ./HTMLFormControlsCollectionBinding.cpp:485:61: error: no viable conversion from 'Nullable<class mozilla::dom::OwningRadioNodeListOrElement>' to 'Nullable<class mozilla::dom::OwningNodeListOrElement>' 8:05.01 self->NamedGetter(NonNullHelper(Constify(name)), found, result); 8:05.01 ^~~~~~ 8:05.01 ../../dist/include/mozilla/dom/Nullable.h:104:3: note: candidate template ignored: could not match 'nsTArray<type-parameter-0-0>' against 'mozilla::dom::OwningNodeListOrElement' 8:05.01 operator const Nullable< nsTArray<U> >&() const { 8:05.01 ^ 8:05.01 ../../dist/include/mozilla/dom/Nullable.h:111:3: note: candidate template ignored: could not match 'FallibleTArray<type-parameter-0-0>' against 'mozilla::dom::OwningNodeListOrElement' 8:05.01 operator const Nullable< FallibleTArray<U> >&() const { 8:05.01 ^ 8:05.01 ../../dist/include/mozilla/dom/HTMLFormControlsCollection.h:49:50: note: passing argument to parameter 'aResult' here 8:05.01 Nullable<OwningNodeListOrElement>& aResult); 8:05.01 ^ 8:05.01 In file included from /Users/djc/src/mozilla-central/obj-ff-dbg/dom/bindings/UnifiedBindings10.cpp:162: 8:05.01 ./HTMLFormControlsCollectionBinding.cpp:573:61: error: no viable conversion from 'Nullable<class mozilla::dom::OwningRadioNodeListOrElement>' to 'Nullable<class mozilla::dom::OwningNodeListOrElement>' 8:05.01 self->NamedGetter(NonNullHelper(Constify(name)), found, result); 8:05.01 ^~~~~~ 8:05.01 ../../dist/include/mozilla/dom/Nullable.h:104:3: note: candidate template ignored: could not match 'nsTArray<type-parameter-0-0>' against 'mozilla::dom::OwningNodeListOrElement' 8:05.01 operator const Nullable< nsTArray<U> >&() const { 8:05.01 ^ 8:05.01 ../../dist/include/mozilla/dom/Nullable.h:111:3: note: candidate template ignored: could not match 'FallibleTArray<type-parameter-0-0>' against 'mozilla::dom::OwningNodeListOrElement' 8:05.01 operator const Nullable< FallibleTArray<U> >&() const { 8:05.01 ^ 8:05.01 ../../dist/include/mozilla/dom/HTMLFormControlsCollection.h:49:50: note: passing argument to parameter 'aResult' here 8:05.01 Nullable<OwningNodeListOrElement>& aResult); 8:05.01 ^ 8:05.01 6 errors generated.
> Is it because I removed the RadioNodeList typedef? Yes. The C++ signature for NamedItem on HTMLFormControlsCollection right now looks like this: 50 void 51 NamedItem(const nsAString& aName, 52 Nullable<OwningNodeListOrElement>& aResult) But with your change it'll be OwningRadioNodeListOrElement instead of OwningNodeListOrElement. Similar for the NamedGetter on HTMLFormControlsCollection. And in the implementation, you'd do SetAsRadioNodeList() instead of SetAsNodeList(). You'll probably just want to static_cast "nodelist.get()" to "RadioNodeList*". > Do I have to add some macros to RadioNodeList.h or something? No, this is still the same issue as above; it's just telling you about all the NamedItem functions the compiler _tried_ using and why it couldn't use them. > field has incomplete type 'mozilla::dom::OwningNodeListOrElement' That type no longer exists with your change, since it was generated just for this interface and now we're generating OwningRadioNodeListOrElement. This, too, will be fixed by changing the type in the call signature as above.
Mentor: bzbarsky
Whiteboard: [mentor=bzbarsky@mit.edu][lang=c++] → [lang=c++]
Here's another attempt. This has at least two salient problems: 0:11.85 /Users/djc/src/mozilla-central/content/html/content/src/HTMLFormControlsCollection.cpp:361:12: error: cannot initialize return object of type 'mozilla::dom::Element *' with an rvalue of type 'mozilla::dom::RadioNodeList *' 0:11.85 return result.GetAsRadioNodeList().get(); 0:11.85 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ I'm not sure what GetFirstNamedElement should return. Should I declare one of these RadioNodeListOrElement things for this? When I change the return type to that, it starts complaining that "return type of virtual function 'GetFirstNamedElement' is not covariant with the return type of the function it overrides ('mozilla::dom::RadioNodeListOrElement' is incomplete)".
Attachment #8437907 - Attachment is obsolete: true
Attachment #8451231 - Flags: review?(bzbarsky)
Comment on attachment 8451231 [details] [diff] [review] Another attempt, slightly further along > I'm not sure what GetFirstNamedElement should return Returning Element* there is fine. You basically do not want to change GetFirstNamedElement except for renaming the union type. >+++ b/content/html/content/src/HTMLFormControlsCollection.h >+class RadioNodeListOrElement; Shouldn't need this. >+++ b/content/html/content/src/RadioNodeList.cpp You need to have a QueryInterface implementation here, and give this class an IID, since you're relying on that when you do the QI in HTMLFormControlsCollection::NamedGetter. >+#include "jsfriendapi.h" js/TypeDecls.h, please. You just need declarations for JSContext and JSObject, not all of jsapi. GetValue and SetValue need implementations, of course. You need to actually create instances of the new class in HTMLFormElement::AddElementToTableInternal. The rest of this is looking good!
Attachment #8451231 - Flags: review?(bzbarsky) → feedback+
(In reply to Boris Zbarsky [:bz] from comment #9) > Returning Element* there is fine. You basically do not want to change > GetFirstNamedElement except for renaming the union type. Hmm. I now have: @@ -380,8 +380,8 @@ aResult.SetValue().SetAsElement() = element; return; } - if (nsCOMPtr<nsINodeList> nodelist = do_QueryInterface(item)) { - aResult.SetValue().SetAsNodeList() = nodelist; + if (nsCOMPtr<RadioNodeList> nodelist = do_QueryInterface(item)) { + aResult.SetValue().SetAsRadioNodeList() = nodelist.get(); return; } MOZ_ASSERT_UNREACHABLE("Should only have Elements and NodeLists here."); But this results in: 0:11.96 /Users/djc/src/mozilla-central/content/html/content/src/HTMLFormControlsCollection.cpp:361:18: error: no viable conversion from 'OwningNonNull<mozilla::dom::RadioNodeList>' to 'nsINodeList' 0:11.96 nsINodeList& nodelist = result.GetAsRadioNodeList(); 0:11.96 ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~ So what should be the way to get the actual first element out of my RadioNodeList? > >+++ b/content/html/content/src/RadioNodeList.cpp > > You need to have a QueryInterface implementation here, and give this class > an IID, since you're relying on that when you do the QI in > HTMLFormControlsCollection::NamedGetter. Can you unpack this a bit? I can't figure it out. I found https://developer.mozilla.org/en/docs/Implementing_QueryInterface, but there are no users of NS_IMPL_QUERY_INTERFACE in content/html. Should RadioNodeList inherit from nsISupports? Should it also inherit from NSIDOMRadioNodeList, or other things? > GetValue and SetValue need implementations, of course. > > You need to actually create instances of the new class in > HTMLFormElement::AddElementToTableInternal. Yeah, I'd just like to have something that compiles first...
> But this results in: You want: RadioNodeList& nodelist = result.GetAsRadioNodeList(); since what you have on the right-hand side is getting out a RadioNodeList reference. > Can you unpack this a bit? Sure. What you want to do is to add NS_DECL_ISUPPORTS_INHERITED in the class declaration. You also want to create an iid for the interface, with some name assigned via a #define, put NS_DECLARE_STATIC_IID_ACCESSOR with that name in the class decl, and NS_DEFINE_STATIC_IID_ACCESSOR after the class decl. content/media/mediasource/MediaSource.h is a good example to crib from here. Then in the C++ file, you'd do the relevant NS_IMPL_ADDREF/RELEASE_INHERITED bits (like MediaSource) and: NS_IMPL_ISUPPORTS_INHERITED(RadioNodeList, nsSimpleContentList, RadioNodeList)
Attached patch Patch + basic tests (obsolete) — Splinter Review
Attachment #8451231 - Attachment is obsolete: true
Attachment #8456411 - Flags: review?(bzbarsky)
Comment on attachment 8456411 [details] [diff] [review] Patch + basic tests >+ aResult.SetValue().SetAsRadioNodeList() = nodelist.get(); Why do you need the .get() here? I'd think you don't. >+HTMLInputElement* getRadio(nsIContent* node) { This should probably be a static function called GetAsRadio. It should also probably look more like this: HTMLInputElement* el = HTMLInputElement::FromContent(node); if (el && el->GetType() == NS_FORM_INPUT_RADIO) { return el; } return nullptr; >+void RadioNodeList::GetValue(nsString& retval) { void RadioNodeList::GetValue(nsString& retval) { is the local style. >+ if (!test) { >+ continue; >+ } I suspect the code would be simpler if you just did: HTMLInputElement* test = GetAsRadio(Item(i)); if (test && test->Checked()) { element->GetValue(retval); return; } in the body of the loop, plus rename "test" to "maybeRadio". >+ retval = NS_LITERAL_STRING(""); retval.Truncate(); >+void RadioNodeList::SetValue(const nsAString& value) { Again the style nit. And again, the logic in the loop would be simpler if it just used an early return when it finds the right node. >+++ b/content/html/content/src/RadioNodeList.h >+#include "nsWrapperCache.h" Why do you need this here? The rest looks great. r=me with the above nits picked, and thank you for sticking with this!
Attachment #8456411 - Flags: review?(bzbarsky) → review+
Attached patch bug-779723 (obsolete) — Splinter Review
Patch with comments addressed. I initially tried to keep the loop in the getRadio() helper function and I guess moved the loop over in too much of a hurry to think properly. Thanks for the comments, this is much better.
Assignee: nobody → dirkjan
Attachment #8456411 - Attachment is obsolete: true
Status: NEW → ASSIGNED
reuben pushed this to try for me: https://tbpl.mozilla.org/?tree=Try&rev=f4e1a36882c7
Comment on attachment 8456440 [details] [diff] [review] bug-779723 >+HTMLInputElement* GetAsRadio(nsIContent* node) { HTMLInputElement* GetAsRadio(nsIContent* node) { >+RadioNodeList::GetValue(nsString& retval) { >+RadioNodeList::SetValue(const nsAString& value) { Curlies on next line.
Attached patch bug-779723 (obsolete) — Splinter Review
Fix up issues reported by Try run: diff --git a/content/html/content/src/HTMLFormElement.cpp b/content/html/content/src/HTMLFormElement.cpp --- a/content/html/content/src/HTMLFormElement.cpp +++ b/content/html/content/src/HTMLFormElement.cpp @@ -50,0 +51,1 @@ +#include "RadioNodeList.h" diff --git a/dom/imptests/failures/html/html/semantics/forms/the-form-element/mochitest.ini b/dom/imptests/failures/html/html/semantics/forms/the-form-element/mochitest.ini deleted file mode 100644 --- a/dom/imptests/failures/html/html/semantics/forms/the-form-element/mochitest.ini +++ /dev/null @@ -1,6 +0,0 @@ -# THIS FILE IS AUTOGENERATED BY parseFailures.py - DO NOT EDIT -[DEFAULT] -support-files = - - -[test_form-elements-nameditem-01.html.json] diff --git a/dom/imptests/failures/html/html/semantics/forms/the-form-element/test_form-elements-nameditem-01.html.json b/dom/imptests/failures/html/html/semantics/forms/the-form-element/test_form-elements-nameditem-01.html.json deleted file mode 100644 --- a/dom/imptests/failures/html/html/semantics/forms/the-form-element/test_form-elements-nameditem-01.html.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "RadioNodeList should exist": true -} diff --git a/dom/imptests/moz.build b/dom/imptests/moz.build --- a/dom/imptests/moz.build +++ b/dom/imptests/moz.build @@ -26,1 +25,0 @@ - 'failures/html/html/semantics/forms/the-form-element/mochitest.ini', diff --git a/dom/tests/mochitest/general/test_interfaces.html b/dom/tests/mochitest/general/test_interfaces.html --- a/dom/tests/mochitest/general/test_interfaces.html +++ b/dom/tests/mochitest/general/test_interfaces.html @@ -816,0 +817,2 @@ + "RadioNodeList", +// IMPORTANT: Do not change this list without review from a DOM peer!
Attachment #8456440 - Attachment is obsolete: true
Attachment #8456648 - Flags: review?(bzbarsky)
Comment on attachment 8456648 [details] [diff] [review] bug-779723 Yep, makes sense. I should have thought of the test_interfaces thing. :( r=me
Attachment #8456648 - Flags: review?(bzbarsky) → review+
I also filed a spec bug for the "" vs "on" issue for undefined value attributes. https://www.w3.org/Bugs/Public/show_bug.cgi?id=26353
Push to TRY via the kindness of strangers :-) https://tbpl.mozilla.org/?tree=Try&rev=128c6c6c9f82
Comment on attachment 8456648 [details] [diff] [review] bug-779723 Review of attachment 8456648 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/test/forms/test_radio_radionodelist.html @@ +24,5 @@ > +var rdoList = document.forms[0].elements.namedItem('rdo'); > +ok(rdoList.value === "", "The value attribute should be empty"); > + > +document.getElementById('r2').checked = true; > +ok(rdoList.value, "2", "The value attribute should be 2"); This does not do what you think it's doing. Use is() or (better) ise().
Keywords: checkin-needed
Attachment #8456648 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Keywords: dev-doc-needed
Depends on: 1162492
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: