177269 - Radio button names are not being treated as case-insensitive

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Stephen Perry]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20021016
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20021016

Ran into this problem on the Huggies Club reward page (you may need to
register first).

Page contains a form with a number of radioboxes (the age selection
ones).  The last radio button for "all ages" is named "ageID", whereas the
other three are named "AgeID".  According to the HTML 4 spec
(http://www.w3.org/TR/html4/interact/forms.html#adef-name-INPUT),
INPUT element names are case *in*sensitive so these radio buttons
should all be treated as part of the same group.  Unfortunately
Mozilla seems to be treating the names as case-sensitive and treats
them as two groups, resulting in values for both controls being
submitted as part of the form.

IE6 and Opera 6 get this right, Mozilla 1.2b and Netscape 4 get this
wrong.  I'll attach a test page for this problem...


Reproducible: Always

Steps to Reproduce:
1.  Load the attached test page
2.  Select either button one, two or three
3.  Select button four

Actual Results:  
Button four is selected as well as one of the other buttons.

Expected Results:  
Button four should be selected, and the other button should be de-selected.

Comment 1

[Stephen Perry]

Attachment: Test page demonstrating problem

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: another testcase...

Comment 3

[Boris Zbarsky [:bzbarsky]]

To form controls...

Looks like for DOM purposes the name is CS in IE and for layout it's CI...

I say we make it CI throughout.

Comment 4

[Boris Zbarsky [:bzbarsky]]

This would also be easy to do if we had a simple way to use a case-insensitive
string key for an nsInterfaceHashtable...

Comment 5

[Benjamin Smedberg]

In this hashtable do you care about case preservation? if not, just lowercase
all strings before they go in. Otherwise, we'd need to write a new string
keyclass with a hash function that lowercases as it hashes, and uses a
case-insensitive key compare method.

--BDS

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, nevermind.  "another testcase" shows that in IE radio groups and
document.form.foo lists are separate entities, whereas in Mozilla they are the
same.  We need to decide whether we want to change this, first....

Comment 7

[Logan Ingalls]

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

Comment 8

[Hixie (not reading bugmail)]

bz: There is a patch in bugh 255168 that may or may not be useful.

Comment 9

[Boris Zbarsky [:bzbarsky]]

[e-mail], what's the exact behavior your patch produces?  See comment 6.

Comment 10

[limonan]

(In reply to comment #9)
> [e-mail], what's the exact behavior your patch produces?  See
comment 6.
> 

It will fix the second test case.

Comment 11

[limonan]

(In reply to comment #9)
> [e-mail], what's the exact behavior your patch produces?  See
comment 6.
> 

Oh, all the test will be Ok.

Comment 12

[Boris Zbarsky [:bzbarsky]]

What do you mean by "all the tests"?  Does the patch produce fully
case-insensitive behavior, or the IE behavior?

Comment 13

[limonan]

(In reply to comment #9)
> [e-mail], what's the exact behavior your patch produces?  See
comment 6.
> 

In fact, in Mozilla , Radiobutton in form and document has two implement.
In document, the radiobutton names are treated as case-insensitive.But in form,
It not.
so, I move the struct nsRadioGroupStruct define from nsDocument.cpp to
content/html/content/public/nsIRadioGroupContainer.h so it can be shared in form
and document.
And then, I just simply 'copy' the RadioGroup Code in nsDocument.cpp to the
nsHTMLFormElement.cpp.
All done. And the Radiobutton in form has the right action according to W3C's spec.

That's All. 

I made this patch on Mozilla 1.6 Code, it works fine. And I does not test it on
the lastest code.

>What do you mean by "all the tests"?  Does the patch produce fully
>case-insensitive behavior, or the IE behavior? 
Yes, fully case-insensitive.

Comment 14

[Boris Zbarsky [:bzbarsky]]

See, that's the problem.  How many sites will break if we implement fully
case-insensitive behavior?

Comment 15

[limonan]

Sorry, I misunderstand.
The patch is IE behavior, not fully case-insensitive behavior as W3C's SPEC.
Though there is still leave some thing, It better than nothing. ;-)
And I think, IE behavior is enough.

the value of document.foo.bar.length is the same as IE.

I think no site will break. But we DO break some site now. :-)

Comment 16

[Boris Zbarsky [:bzbarsky]]

If the patch is the IE behavior, that's great!  Mind cleaning it up (remove code
instead of commenting it out, etc) and attach it to this bug?

Comment 17

[limonan]

Attachment: IE behavior patch

My debug version of mozilla is dead, Did somebody want to test the patch?
based on cvs(8.10)

Comment 18

[limonan]

Warning, this patch can works on mozilla 1.6x
But will BREAK the build in the lastest version.
I'll fix it as quick as possible

Comment 19

[limonan]

Attachment: IE behavior patch V2

Fix the build error

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 155922 [details] [diff] [review]
IE behavior patch V2

jst, I'm having trouble wrapping my head around this code... could you check
the patch out?

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 155922 [details] [diff] [review]
IE behavior patch V2

- In nsDocument::GetRadioGroup():

+  nsAutoString tmKey;
+  tmKey.Assign(aName);

Maybe make that:

+  nsAutoString tmKey(aName);

+  ToLowerCase(tmKey); //should case-insensitive.

and only make this call if IsCaseSensitive() is false, so that this only
happens for HTML documents, not for XHTML or XML documents.

+  //nsStringKey key(aName);
+  //printf("Call Get RadionGroup %s\n",NS_ConvertUTF8toUCS2(aName).get());
+  //title.Equals(aData, nsCaseInsensitiveStringComparator()));

Take out those comments.

- In nsHTMLFormElement::GetRadioGroup():

+nsHTMLFormElement::GetRadioGroup(const nsAString& aName,
+			   nsRadioGroupStruct **aRadioGroup)

Fix the indentation of the second line arguments.

+  nsAutoString tmKey;
+  tmKey.Assign(aName);

Same here, pass aName to tmKey's ctor.

+  ToLowerCase(tmKey); //should case-insensitive.

Here, you should only do this if mNodeInfo->NameSpaceEquals(kNameSpaceID_None)
to keep things case sensitive for XHTML.

+  //nsStringKey key(aName);
+  //printf("Call Get RadionGroup %s\n",NS_ConvertUTF8toUCS2(aName).get());

Remove those comments.

+  if (!radioGroup) {
+    radioGroup = new nsRadioGroupStruct();
+    NS_ENSURE_TRUE(radioGroup, NS_ERROR_OUT_OF_MEMORY);
+    mRadioGroups.Put(&key, radioGroup);

Here you allocate new nsRadioGroupStruct's, but you don't ever delete them.
You'll need to do that in the nsHTMLFormElement destructor or you'll leak them.

- In nsHTMLFormElement::SetCurrentRadioButton():

+  nsRadioGroupStruct* radioGroup = nsnull;
+  GetRadioGroup(aName, &radioGroup);
+  if (radioGroup) {
+    radioGroup->mSelectedRadioButton = aRadio;
+  }

   return NS_OK;

How about:

+  nsRadioGroupStruct* radioGroup = nsnull;
+  GetRadioGroup(aName, &radioGroup);
+  if (!radioGroup) {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }
+
+  radioGroup->mSelectedRadioButton = aRadio;

   return NS_OK;

there in stead?

- In nsHTMLFormElement::GetCurrentRadioButton():

+  nsRadioGroupStruct* radioGroup = nsnull;
+  GetRadioGroup(aName, &radioGroup);
+  if (radioGroup) {
+     *aRadio = radioGroup->mSelectedRadioButton;
+     NS_IF_ADDREF(*aRadio);
+  }
   return NS_OK;

This can return w/o ever setting *aRadio, you need to initialize that to nsnull
if you don't set it... And return NS_ERROR_OUT_OF_MEMORY if !radioGroup.

- In nsHTMLFormElement::GetNextRadioButton():

+  nsRadioGroupStruct* radioGroup = nsnull;
+  GetRadioGroup(aName, &radioGroup);
+  if (!radioGroup) {
+    return NS_ERROR_FAILURE;

Make that NS_ERROR_OUT_OF_MEMORY too.

-  nsBaseContentList *radioGroup =
-    NS_STATIC_CAST(nsBaseContentList *, (nsIDOMNodeList *)radioNodeList);
-  if (!radioGroup) {
-    return NS_ERROR_FAILURE;
+    currentRadio = radioGroup->mSelectedRadioButton;
+    if (!currentRadio) {
+      return NS_ERROR_FAILURE;
+    }
   }
-
-  nsCOMPtr<nsIContent> currentRadioNode(do_QueryInterface(currentRadio));
-  NS_ASSERTION(currentRadioNode, "No nsIContent for current radio button");
-  PRInt32 index = radioGroup->IndexOf(currentRadioNode, PR_TRUE);
+  
+  nsCOMPtr<nsIFormControl> radioControl(do_QueryInterface(currentRadio));
+  PRInt32 index = radioGroup->mRadioButtons.IndexOf(radioControl);

The code you're removing ensures that the next radio button is alwats the next
in document order etc, as expected. The new code just returns the next one in
the array of radio buttons, but I don't see any code anywhere that ensures that
that's in document order. You'll need to add code to the places where you add
to the array (in nsHTMLFormElement::AddToRadioGroup()) to ensure the the radio
button ends up in the right point in the array, w/o that you'll make keyboard
selection order in radio groups random if radio buttons are added to the group
out of order.

...
   PRBool disabled;
   nsCOMPtr<nsIDOMHTMLInputElement> radio;
-
   do {
     if (aPrevious) {
       if (--index < 0) {

Nit-pick of the day: Leave that empty line alone :-)

nsHTMLFormElement::WalkRadioGroup():

 {
+  /*  //Origin Code. 
+  See Bug 177269, I'm afraid that my code might cause new bug.
+  this is the origin code before the bug fixed. Hope somebody will clean it
later.
+
   nsresult rv = NS_OK;

   PRBool stopIterating = PR_FALSE;
@@ -1566,12 +1597,36 @@
   }

   return rv;
+  */

Don't leave large blobs of code commented out, remove it. I'm not sure what
you're worried about breaking here, please elaborate.

+  nsRadioGroupStruct* radioGroup = nsnull;
+  GetRadioGroup(aName, &radioGroup);
+  if (!radioGroup) {
+    return NS_OK;

No radioGroup is never ok, return NS_ERROR_OUT_OF_MEMORY.

Thanks for supplying a patch for this! But all of the above needs to be fixed
before this can go in, are you willing to create a new patch?

Comment 22

[limonan]

But there still has some question.

In nsDocuemnt.cpp the orig code is like following

- nsDocument::GetCurrentRadioButton
  nsRadioGroupStruct* radioGroup = nsnull;
  GetRadioGroup(aName, &radioGroup);
  if (radioGroup) {
    *aRadio = radioGroup->mSelectedRadioButton;
    NS_IF_ADDREF(*aRadio);
  }                                                                                
  return NS_OK;
Notice the Code that has in mozilla's Codebase does not do such check.And dose
not set *aRadio to null also. I don't know why.

- nsDocument::SetCurrentRadioButton
{
  nsRadioGroupStruct* radioGroup = nsnull;
  GetRadioGroup(aName, &radioGroup);
  if (radioGroup) {
    radioGroup->mSelectedRadioButton = aRadio;
  }
The same reasion as above.
 
- nsDocument::GetRadioGroup
                                                                              
  nsRadioGroupStruct *radioGroup =
    NS_STATIC_CAST(nsRadioGroupStruct *, mRadioGroups.Get(&key));
                                                                                
  if (!radioGroup) {
    radioGroup = new nsRadioGroupStruct();
    NS_ENSURE_TRUE(radioGroup, NS_ERROR_OUT_OF_MEMORY);
    mRadioGroups.Put(&key, radioGroup);
  }
The mRadioGroups is just a member of nsDocuemnt. And the nsDocument does not
free the new alloced radioGroup struct. I purpose hashtable will free the
radioGroup when the object destoryed.

I will fix the string missuse, but the code I refered above really makes me
puzzled. 

Do I need the fix all above in the new patch?

Comment 23

[Johnny Stenback (:jst)]

(In reply to comment #22)
> In nsDocuemnt.cpp the orig code is like following
> 
> - nsDocument::GetCurrentRadioButton
>   nsRadioGroupStruct* radioGroup = nsnull;
>   GetRadioGroup(aName, &radioGroup);
>   if (radioGroup) {
>     *aRadio = radioGroup->mSelectedRadioButton;
>     NS_IF_ADDREF(*aRadio);
>   }                                                                          
     
>   return NS_OK;
> Notice the Code that has in mozilla's Codebase does not do such check.And dose
> not set *aRadio to null also. I don't know why.

I guess the lack of code to always initialize *aRadio in the current code is an
oversight, but it should IMO be fixed here. I guess I'm not fully understanding
your comment here about what check the current code does or doesn't do.

> 
> - nsDocument::SetCurrentRadioButton
> {
>   nsRadioGroupStruct* radioGroup = nsnull;
>   GetRadioGroup(aName, &radioGroup);
>   if (radioGroup) {
>     radioGroup->mSelectedRadioButton = aRadio;
>   }
> The same reasion as above.
>  
> - nsDocument::GetRadioGroup
>                                                                               
>   nsRadioGroupStruct *radioGroup =
>     NS_STATIC_CAST(nsRadioGroupStruct *, mRadioGroups.Get(&key));
>                                                                                 
>   if (!radioGroup) {
>     radioGroup = new nsRadioGroupStruct();
>     NS_ENSURE_TRUE(radioGroup, NS_ERROR_OUT_OF_MEMORY);
>     mRadioGroups.Put(&key, radioGroup);
>   }
> The mRadioGroups is just a member of nsDocuemnt. And the nsDocument does not
> free the new alloced radioGroup struct. I purpose hashtable will free the
> radioGroup when the object destoryed.

In the current code, mSelectedRadioButtons was of type
nsInterfaceHashtable<nsStringHashKey,nsIDOMHTMLInputElement>, and such a hash
table does release its content on destruction, but in your patch you changed
that to mRadioGroups of type nsHashtable and nsHashtable knows nothing about its
content (only its keys), so it can't delete its content, the user of that class
is responsible of memory management of the data in the nsHashtable.

> Do I need the fix all above in the new patch?

The hash content leak needs to be fixed, and the order of the radio buttons in
the array needs to be fixed at least...

Comment 24

[limonan]

Attachment: IE behavior patch V3

The hash content leak is still there,the order of the radio buttons in
the array has fixed.
I'll fix the hash leak in a new patch.

Comment 25

[limonan]

Attachment: Patch V4

Comment 26

[Boris Zbarsky [:bzbarsky]]

Er... did you actually mean to mark review+, or to request review?

Comment 27

[limonan]

Sorry, I means I want a review.

Comment 28

[limonan]

Sorry, I means the patch requests a reviewer.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 185381 [details] [diff] [review]
Patch V4

Then you want to set the review flags accordingly...

Comment 30

[Johnny Stenback (:jst)]

Comment on attachment 185381 [details] [diff] [review]
Patch V4

r+sr=jst, sorry for the way too long delay here.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.  My apologies also that this took so long -- the last few months
have been sort of full of security releases and then the 1.8 Gecko release.  :(