<FORM NAME=mainform> <INPUT TYPE=radio NAME=btn VALUE="heck yeah" CHECKED STYLE="display: none"> <SCRIPT>alert(document.mainform.btn.checked)</SCRIPT> </FORM> should show true. Currently it shows false. This affects its ability to be properly submitted and manipulated with JavaScript.

Note that the script below may also fail if you remove display: none, because the frame may not have been created. It will *definitely* fail if you create the radio button in a script and try to access its CHECKED state before you actually add it to the document with appendChild() or some such, because you're guaranteed the frame will not be created until it's inserted into the document. On top of this, I will probably make radio buttons happy whether or not they're in a form--I'll be changing those parts anyway, so it's not a big deal. Finally, this change will probably finally obsolete nsFormFrame.cpp and make it a simple block frame (yay!). If this turns out to be the case, I will remove the class and make any resultant changes elsewhere.

First version. We need to move the nsPLDStringSupportsHashTable out somewhere else, it doesn't really belong in nsHTMLFormElement. Things this patch does not address (for various reasons): - grouping radio buttons outside of forms - ensuring at least one radio button in every group is checked - removing nsFormFrame entirely (which may be imminently possible)

And by the way, this bug will likely *not* address those 3 problems. The patch is almost complete. I almost scaled the first two problems, and then realized that the best way to deal with them would require mess with parser. Specifically we would want to suppress adding radios to groups until the <input> tag is totally parsed. The problem with checking at least one in a group is, when <input name=a> is created, first the <input> is added to the form, at which point it is added to the blank group, which only has that input in it, so it is checked. Then name=a is parsed and the attribute is set, and the <input>, which is now checked, is added into the group. Adding a checked radio button into a group leaves the button checked (which is kinda what you'd expect with JS) and therefore deselects any other radios in the group. The proper solution is to consider <input name=a> as a piece and not add the control into the group until it is fully parsed. The problem with radios not in the document is similar. When we add a node to the document it is not yet added to the form. At that point we have to determine the "checked changed" property of the group. Finding a radio in the group to get "checked changed" from will be an expensive operation unless we add a hashtable to the group, and it's not even necessary. This one may be more easily surmountable, but we really shouldn't do anything with the non-form group for form controls inside a form. Again, we either have to predict whether the form will be set or we have to be told when that operation is complete (like when the <input> is completely parsed). nsFormFrame I'm not getting rid of right here because it can easily be dealt with separately and this patch is already big enough. No other reason. This patch adds the infrastructure necessary to support both of these, however, with the nsIRadioVisitor interface. On the plus side, this patch adds very little memory requirements, and subtracts a lot.

Marking nsbeta1+

- uses nsDoubleHashtable (now in tree) - moved name change checking to nsHTMLInputElement.cpp, moved that code out so that SetAttr() and UnsetAttr() would have the same code paths - moved AddedToRadioGroup() and RemovedFromRadioGroup() (formerly CheckAddRadio and CheckRemoveRadio) from nsHTMLFormElement to nsHTMLInputElement to simplify slightly and to make it easier to make radio buttons outside of forms work

As per JST's comments in IRC: - add Init() method to nsHTMLFormElement - make operators that split lines show up at the end of the previous line - there were some places we were grabbing radio.value when we didn't need to (printf remnants) Also: - removed the accidentally-pulled-in stuff from other work in layout/html/forms

http://www.johnkeiser.com/mozilla/radio has extensive regression tests for radio button. Please note that when you add/remove radio buttons dynamically to/from the document, it's not repainting the test. This bug is unrelated to this patch, as it happens before applying as well. Just move a window over the affected area and you'll see what you're supposed to see. Here's a description of the radio button architecture and the patch for your reviewing pleasure: FUNCTIONALITY Radio Button Rules: - when a radio button is checked by JS or by a click, the checked radio button in the group will be unchecked. - when a *checked* radio button is added into a group, the checked radio button in the group will be unchecked. Grouping: - a radio button can be added to or removed from a group when it's type or name changes, or when it is added into the document underneath a form. - a radio button without a form is (currently) not in a group (bug 14445). Checked Changed: - "checked changed" is a bit that determines whether an element's checked property has been changed by JS or by a user click. When this has happened, setAttribute("checked") no longer affects the checkbox's value. (This is a compromise between the spec and real-world behavior, and is currently already done for checkbox and input type=text.) - "checked changed" applies to the entire group rather than to an individual button. If .checked is changed by JavaScript in one radio button, setAttribute("checked") will no longer work for any other radio button in the group. ARCHITECTURE HIGHLIGHTS Adding to/removing from a radio group: - nsHTMLInputElement::AddedToRadioGroup() and nsHTMLInputElement::RemovedFromRadioGroup() are called whenever an action occurs that changes an input element's being in a group. Generally nothing happens, unless the radio button in question is checked nsIRadioVisitor: - we use the visitor pattern to walk radio controls in a group. This is because: (a) there will be multiple walkers in the future--form controls not in a form vs. those in a form. (b) there are multiple operations that have to be performed on a group, and the walking code is non-trivial--a visitor is the easiest way to reuse this code. Though there are currently only two operations, a third will be necessary if/when we implement the rule that any radio group *must* have something checked, even if nothing is explicitly checked by the user. - nsIRadioVisitor is the interface implemented by visitors (called by form walkers). - nsHTMLFormElement::WalkRadioGroup() and nsHTMLInputElement::VisitGroup() currently handle this visitor pattern. Checked Changed: - To implement checked changed, when .checked is set using JS or the user clicks on a radio button, we iterate over all radio buttons to find the - To maintain data integrity, when a radio button is added to a radio group, its checked changed property is set to be the same as others in the same group.

John, do you have an estimate for the date this will land so that Brian can enable all the XBL Form Controls? Also, could you please add this to the mozilla landing plan page?

Comment on attachment 71020 [details] [diff] [review] Patch v2.1 Seems like you should check the return value of NS_GetRadioSetCheckedChangedVisitor and for some unknow reason it fails (although doubtful) you are not checking "visitor" and just passing it into VisitGroup VisitgGroup is defined as having a return code, and it appears to always returns NS_OK and doesn't appear to be in an interface, so just make it a void. There are some places where you are casting, you should probably use STATIC_CAST: + rv = ((nsHTMLInputElement*) + (nsIDOMHTMLInputElement*) + currentlySelected)->SetCheckedInternal(PR_FALSE); + rv = mForm->SetCurrentRadioButton(name, (nsIDOMHTMLInputElement*)this); Looks like you should have an assert here, since you removed the check on aType: nsHTMLInputElement::GetType(PRInt32* aType) Maybe comment about how you know it will always be loewer case: + checked ? NS_LITERAL_STRING("t") : Is NS_OK the right thing to return is mForm is NULL? +NS_IMETHODIMP +nsHTMLInputElement::AddedToRadioGroup() +{ + // + // Currently the only time radio groups really happen is in forms + // + if (!mForm) { + return NS_OK; + } Same for (is an assert a proper thing to do? Or is it just an no-op?): +nsHTMLInputElement::RemovedFromRadioGroup(nsIForm* aForm, nsAString* aName) +{ + // + // Currently radio groups only happen in forms + // + if (!aForm) { + return NS_OK; + } I see where WalkRadioGroup's return is not checked where it is used yet it can return alues other than NS_OK, should it be checked? In: + NS_IMETHOD Visit(nsIFormControl* aRadio, PRBool* aStop) + { + nsCOMPtr<nsIRadioControlElement> radio(do_QueryInterface(aRadio)); + radio->SetCheckedChangedInternal(mCheckedChanged); + *aStop = PR_TRUE; + return NS_OK; + } We should have an NS_ENSURE_POINTER_ARG Seems like we make the assumption that aRadio will always QI to nsIRadioControlElement we should at least have an assert here. In: + NS_IMETHOD Visit(nsIFormControl* aRadio, PRBool* aStop) + { + if (aRadio == mExcludeElement) { + return NS_OK; + } + nsCOMPtr<nsIRadioControlElement> radio(do_QueryInterface(aRadio)); + radio->GetCheckedChanged(mCheckedChanged); + *aStop = PR_TRUE; + return NS_OK; + } We should have an NS_ENSURE_POINTER_ARG Here: + static nsIRadioVisitor* visitorTrue = nsnull; + static nsIRadioVisitor* visitorFalse = nsnull; We need to have a BIG comment on why these are static and how holding onto their values are necessary. Also, I think maybe they need to be static class data members instead of being define static in the method. Overall the arch looks right, the above are just nits, but things that should be looked at. It's great to have this out of the frames. fix up the above r=rods

Fixed all except: 1. Should we return NS_OK when mForm is null in AddedToRadioGroup(), RemovedFromRadioGroup()? This can be called even if there is no form. It's a valid thing to do (we do it in SetAttr). In those cases the "radio group" is simply the button itself and there is nothing else to do. 2. In both Visit()s we should have an NS_ENSURE_POINTER_ARG and assert the result of QueryInterface. Both of these are really part of the contract; so I did an assertion instead and changed the documentation for the interface to explicitly reflect this..

rods changes above, one more change from his requests: - visitorTrue and visitorFalse remained static members in NS_GetRadioSetCheckedChangedVisitor, because to do otherwise required initializing the members outside of the class as well as making NS_NewSetCheckedChangedVisitor a friend of nsSetCheckedChangedVisitor. Comments added though.

Not making it for 0.9.9, hopefully we'll land early in 1.0 cycle

Hopefully? Having all the form controls as soon as possible is critical for multiple projects. Is there anything we can do to help you land this sooner?

It's conditional on sr=, the patch is done and has r=. It *will* land in 1.0 and I hope it will land *early* in 1.0.

I hear you John, but one milestone granularity is not sufficient, even with hope. We have dependent features that require a landing *date* in the next 2 weeks. Could you use some help?

I'm working on sr'ing these changes, I won't be done today, but hopefully I'll get through this sometime tomorrow...

Comment on attachment 71705 [details] [diff] [review] Patch v2.2 - In nsFormControlFrame.cpp: @@ -897,7 +895,7 @@ nsIDOMHTMLInputElement* inputElement; if (NS_OK == mContent->QueryInterface(NS_GET_IID(nsIDOMHTMLInputElement), (void**)&inputElement)) { inputElement->SetChecked(aState); - NS_RELEASE(inputElement); + NS_RELEASE(inputElement); nsCOMPtr! - In nsHTMLInputElement.cpp: + rv = (*aState)->SetStateProperty(NS_LITERAL_STRING("checked"), + checked ? NS_LITERAL_STRING("t") : + NS_LITERAL_STRING("f")); I believe this will get you into trouble on some compilers, I think it's safe to use two named literal strings and use those in the tri-statement, but the above is most likely not xp. - In nsRadioSetCheckedChangedVisitor, you don't need to declare any nsISupports stuff there since you're not directly inheriting any interfaces. Just let the base class deal with it. - In nsRadioGetCheckedChangedVisitor, same thing... Other than that, sr=jst

Comment on attachment 71705 [details] [diff] [review] Patch v2.2 a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Fix checked in (with JST's changes).