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

RESOLVED FIXED in mozilla1.0.1

Status

()

defect
P1
normal
RESOLVED FIXED
20 years ago
5 years ago

People

(Reporter: ian, Assigned: john)

Tracking

(4 keywords)

Trunk
mozilla1.0.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: relnote-devel,DIGBug[HTML4-17.2.1][FIX], )

Attachments

(4 attachments, 10 obsolete attachments)

Reporter

Description

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

Updated

20 years ago
Assignee: karnaze → kmcclusk

Comment 1

20 years ago
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.
Reporter

Comment 2

20 years ago
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.... ;-)
Status: NEW → ASSIGNED
Target Milestone: M13
Moving to M13

Comment 4

20 years ago
QA Contact update.
Reporter

Updated

20 years ago
Blocks: html4.01
Target Milestone: M13 → M15
Moving to M15
Assignee: kmcclusk → rods
Status: ASSIGNED → NEW
Reassigning to Rod.

Updated

20 years ago
Severity: normal → enhancement
Status: NEW → RESOLVED
Last Resolved: 20 years ago
Resolution: --- → LATER

Comment 7

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

Comment 8

20 years ago
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

20 years ago
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.
Reporter

Comment 10

20 years ago
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

20 years ago
It's probably a 5.1 issue.
Reporter

Updated

20 years ago
Status: RESOLVED → VERIFIED
Reporter

Comment 12

20 years ago
Ok, 5.1 it is.

Updated

20 years ago
Severity: enhancement → normal

Comment 13

20 years ago
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.
Reporter

Comment 14

19 years ago
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.
Status: VERIFIED → REOPENED
Keywords: html4
Resolution: LATER → ---
Target Milestone: M15 → Future
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

19 years ago
Future is fine, but I want to bring this in a little, changing to M18
Target Milestone: Future → M17

Comment 17

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

Comment 18

19 years ago
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. 
Status: REOPENED → ASSIGNED
Target Milestone: M17 → Future
Reporter

Comment 19

19 years ago
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.
Keywords: 4xp
OS: Windows 98 → All
Hardware: PC → All
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.
Keywords: helpwanted, relnote

Comment 21

19 years ago
Yes, if you wrap it in a form it works fine, release note it
*** Bug 41430 has been marked as a duplicate of this bug. ***

Comment 23

19 years ago
Updating QA contact.
QA Contact: ckritzer → bsharma
Reporter

Comment 24

19 years ago
Taking QA per managerial policy.
Reporter

Updated

19 years ago
QA Contact: bsharma → py8ieh=bugzilla

Updated

19 years ago
Summary: <input type="radio"> not mutually exclusive without form → [RBTN]<input type="radio"> not mutually exclusive without form
Whiteboard: relnote-devel

Updated

19 years ago
Priority: P3 → P1
Target Milestone: Future → ---

Comment 25

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

Updated

19 years ago
Depends on: 39953
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.
Keywords: nsbeta1

Comment 27

19 years ago
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

19 years ago
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

19 years ago
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

19 years ago
Rod is saying to use a new Mozilla nightly, *not* Netscape 6.
Reporter

Updated

19 years ago

Comment 31

19 years ago
might wait for XBL form controls
Target Milestone: --- → mozilla1.0.1
*** Bug 94116 has been marked as a duplicate of this bug. ***
*** Bug 99352 has been marked as a duplicate of this bug. ***
*** Bug 99353 has been marked as a duplicate of this bug. ***

Updated

18 years ago
Blocks: 104166

Comment 35

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

Comment 36

18 years ago
This will happen during or after bug 108308.  Setting dependent.
*** Bug 121309 has been marked as a duplicate of this bug. ***
Whiteboard: relnote-devel → relnote-devel,DIGBug
Assignee

Comment 38

18 years ago
Taking, leaving milestone set.  Whether this gets done with bug 108308 now
depends on how easy it is (it may still be easy).
Assignee: rods → jkeiser
Status: ASSIGNED → NEW
Depends on: 108308
Assignee

Comment 39

18 years ago
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

17 years ago
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

17 years ago
Posted file testcase1

Comment 42

17 years ago
Posted file testcase2

Comment 43

17 years ago
Posted patch First work arround patch (obsolete) — Splinter Review
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?

Updated

17 years ago
Attachment #81144 - Attachment is patch: false
Attachment #81144 - Attachment mime type: text/plain → text/html

Comment 44

17 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #81148 - Attachment is obsolete: true

Comment 45

17 years ago
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?
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

17 years ago
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
Attachment #81279 - Attachment is obsolete: true
Assignee

Comment 48

17 years ago
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

17 years ago
I will file another bug to implement functions of nsIDocument in nsXULDocument
later
Attachment #83529 - Attachment is obsolete: true
Assignee

Comment 50

17 years ago
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 :)
Assignee

Comment 51

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

Comment 52

17 years ago
Attachment #83686 - Attachment is obsolete: true

Comment 53

17 years ago
Posted patch patch(fix some problem) (obsolete) — Splinter Review
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!
Attachment #83693 - Attachment is obsolete: true
Assignee

Comment 54

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

r=jkeiser
Attachment #83695 - Flags: review+
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).
Attachment #83695 - Flags: needs-work+

Comment 56

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

Comment 57

17 years ago
*** Bug 145647 has been marked as a duplicate of this bug. ***
*** Bug 154472 has been marked as a duplicate of this bug. ***

Comment 59

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

Comment 60

17 years ago
Posted patch Patch v1.2.9 (WIP) (obsolete) — Splinter Review
This patch doesn't work yet but has all the ingredients it needs.  Posting it
here as a transport mechanism between home and work :)
Assignee

Comment 61

17 years ago
Posted patch Patch v1.2.10 (WIP) (obsolete) — Splinter Review
Yet another transfer patch.  Works fine for normal radios but on testcases
here, crashy crashy.
Attachment #90972 - Attachment is obsolete: true
Whiteboard: relnote-devel,DIGBug → relnote-devel,DIGBug[HTML4-17.2.1]
*** Bug 157405 has been marked as a duplicate of this bug. ***
Assignee

Comment 63

17 years ago
Posted patch Patch v1.3 (obsolete) — Splinter Review
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.
Attachment #91074 - Attachment is obsolete: true
Assignee

Comment 64

17 years ago
I want to point out that this patch was based on Pete Zha's working patch from
before.
Status: NEW → ASSIGNED
Whiteboard: relnote-devel,DIGBug[HTML4-17.2.1] → relnote-devel,DIGBug[HTML4-17.2.1][FIX]
Assignee

Comment 65

17 years ago
Posted patch Patch v1.3.1Splinter Review
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.
Attachment #83695 - Attachment is obsolete: true
Attachment #91445 - Attachment is obsolete: true

Comment 66

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

r=rods
Attachment #91512 - Flags: review+
Comment on attachment 91512 [details] [diff] [review]
Patch v1.3.1

sr=jst
Attachment #91512 - Flags: superreview+
Comment on attachment 91512 [details] [diff] [review]
Patch v1.3.1

a=asa (on behalf of drivers) for checkin to 1.1
Attachment #91512 - Flags: approval+
Assignee

Comment 69

17 years ago
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 20 years ago17 years ago
Resolution: --- → FIXED

Comment 70

17 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 71

17 years ago
This bug is fixed.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED
Assignee

Comment 72

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

Comment 73

17 years ago
Excellent catch.
Assignee

Comment 74

17 years ago
Posted patch Patch For Uninitialized Var (obsolete) — Splinter Review
Excellent catch.
Assignee

Updated

17 years ago
Attachment #92159 - Attachment is obsolete: true
Assignee

Comment 75

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

Updated

17 years ago
Attachment #92158 - Flags: review+
Comment on attachment 92158 [details] [diff] [review]
Patch For Uninitialized Var

sr=jst
Attachment #92158 - Flags: superreview+
Comment on attachment 92158 [details] [diff] [review]
Patch For Uninitialized Var

a=asa (on behalf of drivers) for checkin to 1.1
Attachment #92158 - Flags: approval+
Assignee

Comment 78

17 years ago
Fix checked in.

Comment 79

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

Updated

17 years ago
Blocks: grouper
Assignee

Comment 80

17 years ago
*** Bug 150936 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.