John Keiser (jkeiser) Assignee
	Description • 23 years ago

<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.

	John Keiser (jkeiser) Assignee
	Comment 1 • 23 years ago

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.

	John Keiser (jkeiser) Assignee
	Comment 2 • 23 years ago

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)

	John Keiser (jkeiser) Assignee
	Comment 3 • 23 years ago

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.

	Kevin McCluskey (gone)
	Comment 4 • 23 years ago

Marking nsbeta1+

	John Keiser (jkeiser) Assignee
	Comment 5 • 23 years ago

- 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

	John Keiser (jkeiser) Assignee
	Comment 6 • 23 years ago

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

	John Keiser (jkeiser) Assignee
	Comment 7 • 23 years ago

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.

	Peter Trudelle
	Comment 8 • 23 years ago

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? 

	rods (gone)
	Comment 9 • 23 years ago

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

	John Keiser (jkeiser) Assignee
	Comment 10 • 23 years ago

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..

	John Keiser (jkeiser) Assignee
	Comment 11 • 23 years ago

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.

	John Keiser (jkeiser) Assignee
	Comment 12 • 23 years ago

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

	Peter Trudelle
	Comment 13 • 23 years ago

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?

	John Keiser (jkeiser) Assignee
	Comment 14 • 23 years ago

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.

	Peter Trudelle
	Comment 15 • 23 years ago

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?

	Johnny Stenback (:jst)
	Comment 16 • 23 years ago

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

	Johnny Stenback (:jst)
	Comment 17 • 23 years ago

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

	Asa Dotzler [:asa]
	Comment 18 • 23 years ago

Comment on attachment 71705 [details] [diff] [review]
Patch v2.2

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

	John Keiser (jkeiser) Assignee
	Comment 19 • 23 years ago

Fix checked in (with JST's changes).