14445 - [RBTN]<input type="radio"> not mutually exclusive without form

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P1)

Description

[Hixie (not reading bugmail)]

Radio buttons that have the same 'name' attribute but are not in a form are
not being considered as mututally exclusive. Clicking on one doesn't unselect
the others. This works fine in IE5.

<div>
  <label>One:   <input name="x" value="1" type="radio"> </label>
  <label>Two:   <input name="x" value="2" type="radio"> </label>
  <label>Three: <input name="x" value="2" type="radio"> </label>
</div>

The following test case shows this:
   http://www.bath.ac.uk/%7Epy8ieh/internet/projects/mozilla/widgets/03.html

TO REPRODUCE: Open up that URI. Click on "One". Click on "Two".
ACTUAL RESULT: Both radio buttons are selected.
EXPECTED RESULT: Only one radio button should ever be selected at any one time.

If the <input> elements are wrapped in a form, then it works fine. A form should
not be needed for this -- <form> elements are only wrappers for deciding what
fields to send to CGI scripts.

Comment 1

[karnaze (gone)]

Kevin, I don't think this worked before gfx form controls. Check to see what
Nav4.6 does and that's probably all we should do for beta.

Comment 2

[Hixie (not reading bugmail)]

Nav 4.x doesn't display the <input> elements _at all_ if they are not in a form,
so trying to emulate Nav 4 is probably a very bad idea.... ;-)

Comment 3

[Kevin McCluskey (gone)]

Moving to M13

Comment 4

[gerardok]

QA Contact update.

Comment 5

[Kevin McCluskey (gone)]

Moving to M15

Comment 6

[Kevin McCluskey (gone)]

Reassigning to Rod.

Comment 7

[rods (gone)]

The spec does not require that this work without a form. changing to LATER and
ENHANCEMENT

Comment 8

[Hixie (not reading bugmail)]

The spec doesn't say it has to work _in_ a form, either -- it just says:

# Radio buttons are like checkboxes except that when several share the same
# control name, they are mutually exclusive: when one is switched "on", all
# others with the same name are switched "off".
   - http://www.w3.org/TR/html4/interact/forms.html#h-17.2.1

Note that this works fine in IE.

rods: How difficult would this be to implement? Why do we currently _need_ a
form element for it to work?

Comment 9

[rods (gone)]

Yes, we do need the form. Right now the named radio "groupings" are keep in the
form, and at this time there is no other "easy" or logical place to put it. So
it would take a little bit of work with not much return.

Comment 10

[Hixie (not reading bugmail)]

Ok. Do you want to look at this for 5.0 at all? i.e., shall I reopen and mark
it as REMIND rather than LATER? Or shall we wait till 5.1?

Comment 11

[rods (gone)]

It's probably a 5.1 issue.

Comment 12

[Hixie (not reading bugmail)]

Ok, 5.1 it is.

Comment 13

[Christine Hoffman]

Not to belabor this issue, but I just wanted to make a point in terms of
standards compliance. Per the HTML 4.0 Transitional DTD, INPUT is not required
to be contained in a FORM. A radio button, which is a value of the 'type'
attribute of the INPUT element, should function regardless of whether there is a
FORM or not. Therefore, in the case where INPUT is not contained in a FORM, we
are not currently standards compliant with regards to the function of radio
buttons. Based on this, I am changing the severity from 'enhancement' to
'normal'. As indicated by Rod, resolution is LATERED as priority for fix is low.

Comment 14

[Hixie (not reading bugmail)]

Reopening and moving to Future, since "LATER" is now deprecated. rods: Sorry
if you didn't want me to do that; feel free to relater it if you don't like the
Future milestone.

Comment 15

[Matthew T (active 1999-2002)]

What you would seem to want here is to attach all unFORMed input elements in the 
document to some sort of dummy FORM element. That way you could check for radio 
buttons with the same name attribute in the same way as you do with any other 
FORM.

Comment 16

[rods (gone)]

Future is fine, but I want to bring this in a little, changing to M18

Comment 17

[rods (gone)]

Yes, XUL creates a "hidden" "dummy" form to place all form elements. I need to 
do this for HTML forms elements.

Comment 18

[rods (gone)]

This bug is marked "future" because it is not critical for RTM (Release To 
Manufacturing). If anyone believes it is critical, please explain why in
this bug. 

Comment 19

[Hixie (not reading bugmail)]

Well, one reason to have this fixed by first release is that we would be 
breaking standards compliance if we did not, and IE has been doing it right
for ages. Personally I would say this was an nsbeta3-, FCS-blocker bug, but
that's just me...

cc'ing Eric Krock for standards compliance judgement call.

Comment 20

[ekrock's old account (dead)]

Well, this didn't work in Nav4, it's only permitted for transitional and not in 
strict (correct me if I'm wrong in interpreting previous comments this way), and 
there's a simple workaround: enclose your radio buttons in a form element, and 
then it will work in both IE5 and Mozilla. Unless someone can provide examples 
of major sites that use this structure, I think we can live with release noting 
this due to press of other urgent work. Marking relnote, helpwanted, leaving 
FUTURE.

Comment 21

[rods (gone)]

Yes, if you wrap it in a form it works fine, release note it

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 41430 has been marked as a duplicate of this bug. ***

Comment 23

[bsharma]

Updating QA contact.

Comment 24

[Hixie (not reading bugmail)]

Taking QA per managerial policy.

Comment 25

[rods (gone)]

This should really be fixed for this next release, leaving as a P1

Comment 26

[ekrock's old account (dead)]

Nom. nsbeta1. There is a workaround (modifying the content), but it's confusing
as heck for the user to encounter a non-exclusive radio button, so we should fix
this to gracefully handle the markup out there on the web.

Comment 27

[Norman]

I have faced a big problem about radio button mutual exclusion in netscape 6.
The event-handling routine(e.g. onclick event) is handled before the update of 
status of the last selected radio button.  Therefore, at the moment while 
javascript code is running in the scope of onclick event. Both the last 
selected radio and the newly selected radio are declared themselves as selected.
Is there any method to solve it ? We need it urgently.

Here is the sample code, please try !

<HTML>
<HEAD>
<TITLE> Netscape 6 Radio Button Mutual Exclusion</TITLE>
</HEAD>

<BODY BGCOLOR="#FFFFFF">
<Script language=javascript>
function showradio()
{	for (var i=0; i<document.testradio.test.length; i++) 
	     if (document.testradio.test[i].checked) 
			alert("Radio button " + document.testradio.test
[i].value + " is selected !");
}

</script>

Instruction : Please click 1 , 2 , 3 several and you will see the problem.

<form name=testradio>
<input type=radio name=test value=1 onclick=showradio()>1
<input type=radio name=test value=2 onclick=showradio()>2
<input type=radio name=test value=3 onclick=showradio()>3
</form>



</BODY>
</HTML>

Comment 28

[rods (gone)]

This bug bug here is about using radiobuttons without an enclosing form. The 
bug you are expereincing has already been fixed. I don't remember the bug 
number, there is no work around. You just need to get a more recent build, then 
6.0 production release.

Comment 29

[Norman]

I think we have some mis-understanding between me and rods. I have add <input 
type=radio> tag between <Form> but still not work. I use the Netscape 6 
Production Release but still get the same result.

Comment 30

[Blake Ross]

Rod is saying to use a new Mozilla nightly, *not* Netscape 6.

Comment 31

[rods (gone)]

might wait for XBL form controls

Comment 32

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 94116 has been marked as a duplicate of this bug. ***

Comment 33

[Boris Zbarsky [:bzbarsky]]

*** Bug 99352 has been marked as a duplicate of this bug. ***

Comment 34

[Boris Zbarsky [:bzbarsky]]

*** Bug 99353 has been marked as a duplicate of this bug. ***

Comment 35

[Robert Kieffer]

Wow, going on 2.5 years for this bug to get fixed... *VERY* impressive.  Can we
maybe get this fixed sometime soon?

Here's my view on this:  As DHTML and JavaScript become more and more prevalent,
the utility of form controls *outside* of forms increases (i.e. since a lot of
logic occurs on the client, it's no longer necessary to actually submit a form.
 Instead, just get some info from form controls in the UI and process it using
javascript.)

However, if I *have* to embed this in a form tag, I now pay a UI hit since I get
all that nasty white space that a form tag forces (as a block element).  This is
a major nuisance and makes using radios a lot harder in DHTML.  :-(

Comment 36

[John Keiser (jkeiser)]

This will happen during or after bug 108308.  Setting dependent.

Comment 37

[Boris Zbarsky [:bzbarsky]]

*** Bug 121309 has been marked as a duplicate of this bug. ***

Comment 38

[John Keiser (jkeiser)]

Taking, leaving milestone set.  Whether this gets done with bug 108308 now
depends on how easy it is (it may still be easy).

Comment 39

[John Keiser (jkeiser)]

My changes in bug 108308 will make this a lot easier than it was, but there are
performance issues to work out before we finally decide how to do it.  See
comments in that bug for why it was not considered a good idea just yet.

Comment 40

[Pete Zha]

jkeiser,
I'm interesting on this bug. I hacked codes of radio button and find it will 
not uncheck other buttons when it is be checked because there is no mForm.
Do you have any good idea to resolve it?
I think maybe we can give all radio buttons(or all formControls) an anymouse 
form which are not in a form.
Do you agree?

Comment 41

[Pete Zha]

Attachment: testcase1

Comment 42

[Pete Zha]

Attachment: testcase2

Comment 43

[Pete Zha]

Attachment: First work arround patch

This patch works fine with attachment 81144 [details] but not works with attachment
81146 [details].In second test case, all radio button will be selected after page loaded.

I guess the reason is, in the second test case, those radio button is in a
table. When DoneCreatingElement in nsHTMLInputElement, The table element has
not created in content tree. So I can't iterate to table element and of course
can't uncheck those radio button.
jkeiser, do you have any idea on this problem?

Comment 44

[Pete Zha]

Attachment: Patch v1

jkeiser, per discussion on IRC, I get a new patch here. Please take a look.
I added a hashtable to document, And implemented it in nsDocument and
nsXULDocument.
Also, I found a bug(maybe a bug) in nsDoubleHashtable.h, it cause redefine
error when I want to use the hashtable in nsXULDocument.
This patch also include fix of form's radio walker and visitor.

There is another issue that when page reload, the value of radio group not
remembered. I think need to add something when the element restore and save
it's state.

Comment 45

[Pete Zha]

jst, jkeiser
When I was hacking on this bug, I found there is a mContentWrapperHash in 
nsDocument. It's for nsIDocument::AddReference and nsIDocument::RemoveReference.
I don't know it's usage. Fabian told me that maybe it is dead code. How do you 
think about it?

Comment 46

[Johnny Stenback (:jst)]

Pete, that code is used by the nsDOMClassInfo code to ensure rooting (in JS GC
terms) of content wrappers when non-predefined properties are set on DOM nodes.
It is not dead code...

Comment 47

[Pete Zha]

Attachment: patch(let document support radio button)

per discussion with jkeiser on IRC, I have worked out a new patch.
jkeiser, this patch include you comments, please review.
It also fix some other problems.
1) SetType in nsHTMLInputElement should call it's own SetAttr instead of
nsGenericHTMLLeafFormElement's
2) We should remove/add radio to/from group if it's type changed
-  if (aName == nsHTMLAtoms::name &&
+  if ((aName == nsHTMLAtoms::name || aName == nsHTMLAtoms::type) &&

I also added some comments, but I'm not sure they are enough, please check.
Thanks

Comment 48

[John Keiser (jkeiser)]

Comment on attachment 83529 [details] [diff] [review]
patch(let document support radio button)

Most of the changes below are localized, but there are enough that I'd like to
see another patch.  r=jkeiser with these fixed.

>Index: base/public/nsIDocument.h
>===================================================================
>+  NS_IMETHOD SetCurrentRadioButton(const nsAString& aName,
>+                                  nsISupports* aRadio) = 0;
>+                                 
>+  NS_IMETHOD GetCurrentRadioButton(const nsAString& aName,
>+                                  nsISupports** aRadio) = 0;

Spacing.  Also all these methods needs comments.  All new methods (or changed
methods) require JavaDoc-like comments (@param and @return and all that). 
@return is not necessary to document for NS_IMETHOD or nsresult, of course.

Also, it would mimick the form interface better and would be clearer if these
were passed and stored as nsIDOMHTMLInputElement.

>Index: base/src/nsDocument.cpp
>===================================================================
>+nsresult
>+nsDocument::GetRadioGroup(const nsAString& aName,
>+                          nsRadioGroupStruct **aRadioGroup)
...
>+    if (!radioGroup) return NS_ERROR_OUT_OF_MEMORY;

NS_ENSURE_TRUE(radioGroup, NS_ERROR_OUT_OF_MEMORY) will get you out of the
"always use brackets" rule :)

>+NS_IMETHODIMP
>+nsDocument::SetCurrentRadioButton(const nsAString& aName,
>+                                  nsISupports* aRadio)
>+{
>+  nsRadioGroupStruct* radioGroup = nsnull;
>+  GetRadioGroup(aName, &radioGroup);
>+  if (radioGroup) {
>+    radioGroup->mSelectedRadioButton = aRadio;
>+    NS_IF_ADDREF(radioGroup->mSelectedRadioButton);

Leak.  Need to release the current radio button before you set it, since you
addref it afterwards.

>+NS_IMETHODIMP
>+nsDocument::GetCurrentRadioButton(const nsAString& aName,
>+                                  nsISupports** aRadio)
>+{
>+  nsRadioGroupStruct* radioGroup = nsnull;
>+  GetRadioGroup(aName, &radioGroup);
>+  if (radioGroup)
>+    *aRadio = radioGroup->mSelectedRadioButton;

You *must* addref the result here.  Getters return strong references, and this
is no exception.

>+NS_IMETHODIMP
>+nsDocument::RemoveFromRadioGroup(const nsAString& aName,
>+                                 nsISupports* aRadio)
>+{
>+  nsRadioGroupStruct* radioGroup = nsnull;
>+  GetRadioGroup(aName, &radioGroup);
>+  if (radioGroup)
>+    radioGroup->mRadioButtons.RemoveElement(aRadio);

Leak.  Need to release the element too, since you addref'd when you did
AddToRadioGroup.

>+NS_IMETHODIMP
>+nsDocument::WalkRadioGroup(const nsAString& aName,
>+                           nsIRadioVisitor* aVisitor)
>+{
...
>+      nsCOMPtr<nsIFormControl> control;
>+      control = do_QueryInterface(NS_STATIC_CAST(nsISupports *,
>+                                  radioGroup->mRadioButtons.ElementAt(i)));

If you stored nsIFormControl in the array instead of nsISupports, and passed
that in to the Add/Remove functions instead of nsISupports, that would get rid
of this QueryInterface.  Less QI's is better, they are expensive.

>Index: html/content/src/nsHTMLInputElement.cpp
>===================================================================
>   NS_IMETHOD HandleDOMEvent(nsIPresContext* aPresContext, nsEvent* aEvent,
>                             nsIDOMEvent** aDOMEvent, PRUint32 aFlags,
>                             nsEventStatus* aEventStatus);
>+                            
>+  NS_IMETHOD SetDocument(nsIDocument* aDocument, PRBool aDeep,
>+                         PRBool aCompileEventHandlers);
>+
> #ifdef DEBUG
>   NS_IMETHOD SizeOf(nsISizeOfHandler* aSizer, PRUint32* aResult) const;
> #endif
>@@ -414,7 +418,7 @@
>                                   const nsAString* aValue,
>                                   PRBool aNotify)
> {
>-  if (aName == nsHTMLAtoms::name &&
>+  if ((aName == nsHTMLAtoms::name || aName == nsHTMLAtoms::type) &&
>       mType == NS_FORM_INPUT_RADIO) {
>     // XXX Because we are not doing anything that assumes the radio button has
>     // actually had its name changed at this point, we can get away with
>@@ -433,7 +437,7 @@
>   // When name changes, radio moves to a different group
>   // (type changes are handled in the form itself currently)
>   //
>-  if (aName == nsHTMLAtoms::name &&
>+  if ((aName == nsHTMLAtoms::name || aName == nsHTMLAtoms::type) &&
>       mType == NS_FORM_INPUT_RADIO) {
>     AddedToRadioGroup();
>   }

Type changes are handled in the form, so you only want to call
AddedToRadioGroup when type changes and the radio button is not in a form.

>     // This is only initialized in if (mForm) for use in later if (mForm)s

Need to update this comment for document also, it really should be just after
the declaration of nsAutoString name;.

>@@ -833,9 +844,12 @@
>   //
>   // Let the form know that we are now the One True Radio Button
>   //

Comment update (s/form/form and document).

>-  if (mForm && NS_SUCCEEDED(rv)) {
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (mForm)
>     rv = mForm->SetCurrentRadioButton(name, NS_STATIC_CAST(nsIDOMHTMLInputElement*, this));
>-  }
>+  else if (mDocument)
>+    rv = mDocument->SetCurrentRadioButton(name, NS_STATIC_CAST(nsISupports *,
>+                                          NS_STATIC_CAST(nsIDOMHTMLInputElement*, this)));

Put brackets around if's please.

>+NS_IMETHODIMP
>+nsHTMLInputElement::SetDocument(nsIDocument* aDocument, PRBool aDeep,
>+                                PRBool aCompileEventHandlers)
>+{
...
>+  if (GET_BOOLBIT(mBitField, BF_PARSER_CREATING))
>+    return NS_OK;

Need a comment there, that is counterintuitive even though it is the right
thing to do.  Also, you want to return early if the radio button is in a form;
otherwise you will add the radio button twice.	Braces around the if too.

>+  if (aDocument)
>+    AddedToRadioGroup();
>+  else
>+    RemovedFromRadioGroup(nsnull, nsnull);
>+    

Braces around if.

>@@ -2365,6 +2399,9 @@
>   }
> 
>   SET_BOOLBIT(mBitField, BF_SHOULD_INIT_CHECKED, PR_FALSE);
>+  
>+  if (!mForm && mType == NS_FORM_INPUT_RADIO)
>+    AddedToRadioGroup();
> 
>   return NS_OK;
> }

Comment.

> nsHTMLInputElement::AddedToRadioGroup()
> {
>   //
>-  // Currently the only time radio groups really happen is in forms
>-  //
>-  if (!mForm) {
>-    return NS_OK;
>-  }

Doesn't matter too much, but you may as well just change that to if (!mForm &&
!mDocument) so that radios not in a document / form don't get penalized.

>+  if (!mForm && mDocument) {
>+    nsAutoString name;
>+    GetName(name);
>+    mDocument->AddToRadioGroup(name, NS_STATIC_CAST(nsISupports *,
>+                               NS_STATIC_CAST(nsIDOMHTMLInputElement*, this)));
>+  }
>+

Comment here, and it's probably best if it is added after
SetCheckedChangedInternal(), because that code is logically grouped with the
VisitGroup() stuff above.

> nsHTMLInputElement::RemovedFromRadioGroup(nsIForm* aForm, nsAString* aName)
> {
>   //
>-  // Currently radio groups only happen in forms
>-  //
>-  if (!aForm) {
>-    return NS_OK;
>-  }

Same as in AddedToRadioGroup, if (!mForm && !mDocument) and update comment.

>+    if (aName)
>+      name = *aName;

This halfway defeats the purpose of passing in aName--this operation performs a
string copy.  To make the code easier to read, you could always use *aName but
when you do GetName(name) set aName = &name;  *Make sure name has a greater
scope than aName in that case.*

> nsresult
> nsHTMLInputElement::VisitGroup(nsIRadioVisitor* aVisitor)
> {
...
>+    mDocument->WalkRadioGroup(name, aVisitor);

If you do rv = mDocument->WalkRadioGroup() like above, you don't have to
initialize rv at the top.

>Index: xul/document/src/nsXULDocument.cpp
>===================================================================

nsXULDocument is going to need this, too, but you can just make a separate bug
for it.  Eventually XULDocument will extend nsDocument, so will probably never
have to worry about it.

Comment 49

[Pete Zha]

Attachment: patch (with jkeiser's comments)

I will file another bug to implement functions of nsIDocument in nsXULDocument
later

Comment 50

[John Keiser (jkeiser)]

Comment on attachment 83686 [details] [diff] [review]
patch (with jkeiser's comments)

One small new thing (sorry I missed it before) ... instead of typedef struct {
... } name; use struct <name> { ... };	It's what we do in most places.

You missed the first review comment in nsHTMLInputElement ... about not calling
AddedToRadioGroup if the radio button is in a form and the type changes.

In the comment in SetDocument you should say "and the parser is still creating
the element" instead of "BF_PARSER_CREATING is true", which doesn't tell the
reader much that they didn't already know :)

Also you didn't make a comment near the end of DoneCreatingElement to say why
you are adding to radio group ... need to explain that the radio button may
have been added to the document while the parser was still creating the element
but we did not want to waste time adding it to the group until everything was
done.

r=jkeiser with these, so especially since they are all comment fixes (except
the typedef struct thing) go ahead and go straight to sr stage when you get
them done :)

Comment 51

[John Keiser (jkeiser)]

Erk, the second comment there is also a necessary code change, shouldn't be big
though (just adding "&& !mForm")

Comment 52

[Pete Zha]

Attachment: patch(minor change with jkeiser's comments)

Comment 53

[Pete Zha]

Attachment: patch(fix some problem)

Fix this:
Don't AddedToRadioGroup/RemovedFromRadioGroup before parser DoneCreatingElement
or radio is in a form and type changes

jst, this patch is ready for sr=, please go ahead. Thanks!

Comment 54

[John Keiser (jkeiser)]

Comment on attachment 83695 [details] [diff] [review]
patch(fix some problem)

r=jkeiser

Comment 55

[Johnny Stenback (:jst)]

Comment on attachment 83695 [details] [diff] [review]
patch(fix some problem)

The more I think about this, the more I think that putting these methods in
nsIDocument is not a good idea. Since we now have some radio control related
methods duplicated and some methods that are very similar shared between
nsIDocument and nsIForm, seems to me that we're in need of something like a
nsIRadioControlContainer interface or somesuch. Pete, would you want to take a
stab at that or do you want me or jkeiser to?

Also, calls to GetRadioGroup() in nsDocument can fail (OOM) so you should
propagate the return value to the callers...

Other than that the changes look reasonable (didn't look all that close yet
though).

Comment 56

[Pete Zha]

jst,
I have already return it like this
NS_ENSURE_TRUE(radioGroup, NS_ERROR_OUT_OF_MEMORY);
(The jst-reviewer.pl need patch :-))

I think you are right. I discussed with jkeiser about this before and we all 
think it's a cool idea.
But it need more time and more thinking. I'm not sure I have much time to do it 
recently because I'm also working on accessibility stuff...
But, it's ok, I will do it when I have time and if anyone have time and 
interesting also can help.

Comment 57

[John Keiser (jkeiser)]

*** Bug 145647 has been marked as a duplicate of this bug. ***

Comment 58

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 154472 has been marked as a duplicate of this bug. ***

Comment 59

[TGOS]

Can someone please add the word "radiobutton" to the short description or the
keywords, as that's how it's commonly called and that's what people will search
for (I did as well, didn't find it and thus caused a new duplicate to that bug).
It's really important to use the terms in the short description or in the
keywords that the user will use when searching for the bug. And searching for
the HTML name of that tag where the last thing I had ever thought about. It is a
radiobutton (also named that way in programming languages), hence that's what
people will look for.

Comment 60

[John Keiser (jkeiser)]

Attachment: Patch v1.2.9 (WIP)

This patch doesn't work yet but has all the ingredients it needs.  Posting it
here as a transport mechanism between home and work :)

Comment 61

[John Keiser (jkeiser)]

Attachment: Patch v1.2.10 (WIP)

Yet another transfer patch.  Works fine for normal radios but on testcases
here, crashy crashy.

Comment 62

[Bradley Baetz (:bbaetz)]

*** Bug 157405 has been marked as a duplicate of this bug. ***

Comment 63

[John Keiser (jkeiser)]

Attachment: Patch v1.3

This should do it.

Changes:
- moves the generic radio group functions from nsIDocument into
nsIRadioGroupContainer (unfortunately there are a few that didn't fit into
forms as well--AddToRadioGroup and RemoveFromRadioGroup--so they stayed)
- changes RemovedFromRadioGroup to WillRemoveFromRadioGroup to simplify things
- fixed a few dynamic JS bugs (it's now fully tested on dynamic JS, I believe)

To test this stuff, go to http://www.johnkeiser.com/mozilla/radio/jsadd.html. 
The second group of radios is outside of a form, and I think you can test just
about every imaginable JS operation, including moving radios from inside a form
to outside a form and back.  Let me know if you can think of more.

Comment 64

[John Keiser (jkeiser)]

I want to point out that this patch was based on Pete Zha's working patch from
before.

Comment 65

[John Keiser (jkeiser)]

Attachment: Patch v1.3.1

This patch is identical to the previous one except that it moves
AddToRadioGroup and RemoveFromRadioGroup into nsIRadioGroupContainer, which
does in fact save us some code.  Forms just return NS_OK from these methods.

Comment 66

[rods (gone)]

Comment on attachment 91512 [details] [diff] [review]
Patch v1.3.1

r=rods

Comment 67

[Johnny Stenback (:jst)]

Comment on attachment 91512 [details] [diff] [review]
Patch v1.3.1

sr=jst

Comment 68

[Asa Dotzler [:asa]]

Comment on attachment 91512 [details] [diff] [review]
Patch v1.3.1

a=asa (on behalf of drivers) for checkin to 1.1

Comment 69

[John Keiser (jkeiser)]

Checked in to trunk.

Comment 70

[Aleksey Nogin]

The following chunk of the nsHTMLInputElement.cpp changes looks pretty suspicious:

@@ -2573,13 +2688,14 @@
 nsHTMLInputElement::VisitGroup(nsIRadioVisitor* aVisitor)
 {
   nsresult rv;
-  if (mForm) {
+  nsCOMPtr<nsIRadioGroupContainer> container = GetRadioGroupContainer();
+  if (container) {
     nsAutoString name;
     GetName(name);
-    rv = mForm->WalkRadioGroup(name, aVisitor);
+    rv = container->WalkRadioGroup(name, aVisitor);
   } else {
-    PRBool stop = PR_FALSE;
-    rv = aVisitor->Visit(this, &stop);
+    PRBool stop;
+    aVisitor->Visit(this, &stop);
   }
   return rv;
 }

This way in the "else" case the rv is no longer set to anything (and is returned
uninitialized!). 

P.S. I've noticed this by seeing that a new "may be used uninitialized" warning
have appeared on Brad TBox:

+content/html/content/src/nsHTMLInputElement.cpp:2690
+ `nsresult rv' might be used uninitialized in this function

More information on these warnings can be found in bug 59652.

Comment 71

[John Keiser (jkeiser)]

This bug is fixed.

Comment 72

[John Keiser (jkeiser)]

(Open a new bug on that stuff, I'm looking into it now.)

Comment 73

[John Keiser (jkeiser)]

Attachment: Patch For Uninitialized Var

Excellent catch.

Comment 74

[John Keiser (jkeiser)]

Attachment: Patch For Uninitialized Var

Excellent catch.

Comment 75

[John Keiser (jkeiser)]

So don't worry about opening a new bug unless you already have; we'll deal with
it in the bug here.  For future reference, opening bugs that are fixed is not
the right way to do things.  It is best to either comment or write a new bug.

Comment 76

[Johnny Stenback (:jst)]

Comment on attachment 92158 [details] [diff] [review]
Patch For Uninitialized Var

sr=jst

Comment 77

[Asa Dotzler [:asa]]

Comment on attachment 92158 [details] [diff] [review]
Patch For Uninitialized Var

a=asa (on behalf of drivers) for checkin to 1.1

Comment 78

[John Keiser (jkeiser)]

Fix checked in.

Comment 79

[Marek Z. Jeziorek]

batch: adding topembed per Gecko2 document
http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html

Comment 80

[John Keiser (jkeiser)]

*** Bug 150936 has been marked as a duplicate of this bug. ***