Bug 90617 – Radio button name is case sensitive

• Component: DOM
• Status: RESOLVED WONTFIX

[Bryan]

Condition: A set of radio buttons with the *same* name but different capitalization

 - Checking one should un-check the others. 
 - Instead, they are treated as separate sets of radio buttons.

     IE 9                    : OK 
     Firefox 13              : OK
     Chrome 22.0.1195.0      : Fail  
     Chrome 20.0.1132.47 m   : Fail

Here's an example in jsFiddle: http://jsfiddle.net/rNdLu/ . It works fine in IE and FF. 

W3C guide to HTML5 3.2.3: "...In HTML, attribute names are case insensitive..."

[Alexey Proskuryakov]

JSFiddle is not working for me now. Can you please attach a test case here?

> W3C guide to HTML5 3.2.3: "...In HTML, attribute names are case insensitive..."

This is unrelated, this bug is about attribute values, not names. Attribute name for the button name is "name".

[Bryan]

Created attachment 151158 [details]
Three radio buttons in two different sets.

[Kent Tamura]

Spec:

http://www.whatwg.org/specs/web-apps/current-work/multipage/states-of-the-type-attribute.html#radio-button-group
> They both have a name attribute, their name attributes are not empty, and the value of a's name attribute is a compatibility caseless match for the value of b's name attribute.

http://www.whatwg.org/specs/web-apps/current-work/multipage/infrastructure.html#compatibility-caseless
> Comparing two strings in a compatibility caseless manner means using the Unicode compatibility caseless match operation to compare the two strings. [UNICODE]

According to Unicode 6.1 Chapter 3, Compatibility caseless match for a string X and a stringY is:
   NFKD(toCaseFold(NFKD(toCasefold(NFD(X))))) = NFKD(toCaseFold(NFKD(toCasefold(NFD(Y)))))

[Pablo Flouret]

Created attachment 151804 [details]
Patch

[Kent Tamura]

Comment on attachment 151804 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151804&action=review

> Source/WebCore/dom/CheckedRadioButtons.cpp:188
> -    OwnPtr<RadioButtonGroup>& group = m_nameToGroupMap->add(element->name().impl(), PassOwnPtr<RadioButtonGroup>()).iterator->second;
> +    OwnPtr<RadioButtonGroup>& group = m_nameToGroupMap->add(element->name().lower().impl(), PassOwnPtr<RadioButtonGroup>()).iterator->second;

I agree that lower() is enough even though the specification asks "compatibility caseless match."  However we should add a comment about it.

Would you make a function like
  AtomicStringImpl* normalizeName(const AtomicString& name)
  {
      // FIXME: The specification asks compatibility ... blah blah
      return name.lower().impl();
  }

[Alexey Proskuryakov]

Comment on attachment 151804 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151804&action=review

> LayoutTests/fast/forms/radio/radio-group-name-case-expected.txt:19
> +form.elements[0].name = 'radio';
> +form.elements[1].name = 'rAdIO';
> +form.elements[2].name = 'RADIO';
> +PASS form.elements[0].checked is false
> +PASS form.elements[1].checked is false
> +PASS form.elements[2].checked is true
> +
> +form.elements[0].name = '\u00F1';
> +form.elements[1].name = '\u00F1';
> +form.elements[2].name = '\u00D1';

Please test for something beyond 8 bits, and ideally beyond BMP too (e.g. Deseret). Also, something with non-trivial case changing behavior (e.g. Straße/STRASSE). I think that the latter test will fail with proposed implementation.

[Pablo Flouret]

(In reply to comment #6)
> (From update of attachment 151804 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151804&action=review
> 
> > LayoutTests/fast/forms/radio/radio-group-name-case-expected.txt:19
> > +form.elements[0].name = 'radio';
> > +form.elements[1].name = 'rAdIO';
> > +form.elements[2].name = 'RADIO';
> > +PASS form.elements[0].checked is false
> > +PASS form.elements[1].checked is false
> > +PASS form.elements[2].checked is true
> > +
> > +form.elements[0].name = '\u00F1';
> > +form.elements[1].name = '\u00F1';
> > +form.elements[2].name = '\u00D1';
> 
> Please test for something beyond 8 bits, and ideally beyond BMP too (e.g. Deseret). Also, something with non-trivial case changing behavior (e.g. Straße/STRASSE). I think that the latter test will fail with proposed implementation.

Deseret works fine, but straße fails, as you suggested. I can use upper() instead of lower(), which i suppose would fix that specific one, but i imagine there must be examples of the opposite version of that in unicode...

Did you have a solution in mind or should i just make it explicit in the test and go with the FIXME?

[Alexey Proskuryakov]

> there must be examples of the opposite version of that in unicode...

Non-trivial examples of case conversion are collected here: <http://unicode.org/Public/UNIDATA/SpecialCasing.txt>.

> Did you have a solution in mind or should i just make it explicit in the test and go with the FIXME?

I think that uppercasing is correct in more cases than lowercasing, but you need to compare two strings to get it right in all cases.

Also, at first I didn't realize that the spec called for compatibility caseless matching. This is weird, and there is an open spec open issue about this: <http://www.w3.org/International/track/issues/105>. At the very least, you need to also add tests that check the difference between compatibility and canonical caseless matching. Ideally, you could figure out what other browsers do, and advise spec authors.

[Pablo Flouret]

Created attachment 152102 [details]
Patch

[Pablo Flouret]

(In reply to comment #9)
> Created an attachment (id=152102) [details]
> Patch

I made the change Kent suggested, and improved the test slightly.

(In reply to comment #8)
> Also, at first I didn't realize that the spec called for compatibility caseless matching. This is weird, and there is an open spec open issue about this: <http://www.w3.org/International/track/issues/105>. At the very least, you need to also add tests that check the difference between compatibility and canonical caseless matching. Ideally, you could figure out what other browsers do, and advise spec authors.

The results of the test are:
WebKit: Pass all.
IE9: Fail test 5
Gecko: Fail test 4 and 6.
Presto: Fail test 4, 5 and 6.

Can't say i'm sure what everyone's doing.

I tried to find how to test the difference between compatibility and canonical matching, but unicode makes my brain hurt, i'll take suggestions on how to improve the tests if anyone is willing to give them.

[Kent Tamura]

(In reply to comment #10)
> The results of the test are:
> WebKit: Pass all.
> IE9: Fail test 5
> Gecko: Fail test 4 and 6.
> Presto: Fail test 4, 5 and 6.

Oh, this is really hard situation.
In such case, we should follow the IE behavior IMO.

[Alexey Proskuryakov]

Test 5 is Deseret, and Windows is notoriously bad at non-BMP Unicode. It's probably not important to match its bugs here.

Perhaps we need more detailed testing, not sure.

[Darin Adler]

Comment on attachment 152102 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152102&action=review

> Source/WebCore/dom/CheckedRadioButtons.cpp:175
> +    return name.upper().impl();

We have a function called foldCase() that I think is better than upper() for this sort of thing.

> Source/WebCore/dom/CheckedRadioButtons.h:48
> +    typedef HashMap<RefPtr<AtomicStringImpl>, OwnPtr<RadioButtonGroup> > NameToGroupMap;

AtomicString objects themselves work fine as hash keys, so there is no need to use RefPtr<AtomicStringImpl>.

But also, we have hash tables that ignore case that will do just as well as upper() or foldCase() without requiring that we make copies of every key, so you could leave the type alone and just change the hash function. The hash for this is called CaseFoldingHash and you can find quite a few examples that use it. You would just do this:

    typedef HashMap<AtomicStringImpl*, OwnPtr<RadioButtonGroup>, CaseFoldingHash> NameToGroupMap;

And maybe we’d need some fixes in StringHash.h to make it work. But then you wouldn’t need any changes at all to CheckedRadioButtons.cpp to fix this bug.

[Darin Adler]

Comment on attachment 152102 [details]
Patch

Please use the CaseFoldingHash approach.

[Darin Adler]

Comment on attachment 152102 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152102&action=review

> Source/WebCore/ChangeLog:16
> +        Accomplish this by using an uppercased version of the radio button's
> +        name. This raises the need for the NameToGroupMap to keep the string
> +        alive, so the key type needs to change to RefPtr<AtomicStringImpl>.
> +
> +        Tangentially, this is useful to get rid of a FIXME claiming there
> +        shouldn't be a need to remove empty RadioButtonGroups from the hash when
> +        they become empty (i.e. potentially trade off a bit of memory for the
> +        decreased performance when the group's size goes from 0 to 1 and back
> +        and forth).

If you want to deal with this tangential issue, please do it in a separate patch, because CaseFoldingHash lets you deal with the case issue without doing this.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Please also test U+0130 (LATIN CAPITAL LETTER I WITH DOT ABOVE) and U+0131 (LATIN SMALL LETTER DOTLESS I).

[Pablo Flouret]

Created attachment 187180 [details]
Patch

[Pablo Flouret]

I added the two tests Ms2ger suggested. With the CaseFoldingHash solution things would look like:

WebKit: Fail test 4, 5, 7, 8.
IE9: Fail test 5 (couldn't check 7 and 8, virtualbox is not cooperating today)
Gecko: Fail test 4, 6, 8.
Presto: Fail test 4, 5, 6, 7, 8.

Can't really say if this is an improvement or not :P

[Kent Tamura]

(In reply to comment #18)
> I added the two tests Ms2ger suggested. With the CaseFoldingHash solution things would look like:
> 
> WebKit: Fail test 4, 5, 7, 8.
> IE9: Fail test 5 (couldn't check 7 and 8, virtualbox is not cooperating today)
> Gecko: Fail test 4, 6, 8.
> Presto: Fail test 4, 5, 6, 7, 8.
> 
> Can't really say if this is an improvement or not :P

Using CaseFoldingHash is very consistent in WebKit. I think the patch is ok as the first step.
We had better add FIXME comment and/or file a bug for this incompatibility.

[Ian 'Hixie' Hickson]

I'm surprised that this bug has no duplicates. Maybe we should change the spec to be case-sensitive instead?

[Domenic Denicola]

In https://github.com/whatwg/html/commit/6acdb2122298d2bb7bb839c0a61b4e1f9b0f9bc9 we have updated the spec to match WebKit's current behavior and use case-sensitive matching for radio button groups. So this bug can be closed.

I will open a separate bug for the usemap cases which also changed in that commit.

[Chris Dumez]

Closing based on Domenic's comment.