Bug 20247 – setAttributeNode() does not work when attribute name has a capital letter in it

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Eric Roman]

When an attribute is set using setAttributeNode(), it does not get set when the name has a capital letter in it.

This problem causes problems on this site:
http://www.staralliance.com/en/travellers/index.html

Works fine in Firefox and IE, and I don't see anything in the spec which restricts the attribute names in this fashion (moreover the plain old setAttribute() does not have this problem).

Reproduces for me in Safari 3.1, and with nightly webkit.
Test case on its way.

[Eric Roman]

Created attachment 22592 [details]
sets attributes using setAttributeNode() -- those with capitals in their name fail

[Eric Roman]

The problem is that when inserting an attribute via setAttributeNode(), we do not normalize the key to lowercase.

The code in getAttribute() expects all the keys to be lowercased, since it does an equality test on the lowercase of the name.

[Alexey Proskuryakov]

Created attachment 22595 [details]
get/set test case

Interesting - looks like in Firefox, Element.setAttributeNode(node) does not change the case of node's name, but the inserted node has a lowercase name:
        var attrib = document.createAttribute("myAttrib");
        attrib.nodeValue = "XXX";
        getNode().setAttributeNode(attrib);
        alert(getNode().getAttributeNode("myAttrib").name);
        alert(attrib.name);

Also, getAttributeNode is case-insensitive in HTML in Firefox 3.

[Alexey Proskuryakov]

<rdar://problem/6118218>

[Eric Roman]

Created attachment 22610 [details]
Several attribute tests, with static compatibility summary for firefox2, firefox3, opera9.5, safari3.1, ie7

[Julien Chaffraix]

Created attachment 22938 [details]
Proposed fix: add bool to getAttributeItem to tune case sensitivity

[Darin Adler]

Comment on attachment 22938 [details]
Proposed fix: add bool to getAttributeItem to tune case sensitivity

Looks like a great fix!

 10         Add a boolean parameter to getAttributeItem to choose between case sensitive and case insisitive

Typo here. It should be "insensitive".

 177             if (name == (shouldIgnoreAttributeCase ? m_attributes[i]->name().toString().lower() : m_attributes[i]->name().toString()))

Calling lower() is a slower way to do a case insensitive comparison that often allocates memory. A faster way is to call equalIgnoringCase.

 71     Attribute* getAttributeItem(const String& name, bool shouldIgnoreAttributeCase = false) const;

Can we do without the default value? How many callers are there that rely on the default?

What's the performance impact of this change? The function seems simple enough that I could imagine having multiple copies of it if there are callers who have no reason to pass the boolean, but on the other hand it's possibly slow enough already that this extra work has little impact.

 82 function testAttribNodeNamePreservesCaseGetNode2() {

I'd prefer that even JavaScript follow our coding guidelines and put braces on a separate line for function definitions.

 92     if (!a) {
 93         return "FAIL";
 94     }

And omit braces for one line if statements.

review- because of the equalIgnoringCase issue; please consider the other comments too.

[Julien Chaffraix]

> 
>  177             if (name == (shouldIgnoreAttributeCase ?
> m_attributes[i]->name().toString().lower() :
> m_attributes[i]->name().toString()))
> 
> Calling lower() is a slower way to do a case insensitive comparison that often
> allocates memory. A faster way is to call equalIgnoringCase.

Ok, I will change that.

> 
>  71     Attribute* getAttributeItem(const String& name, bool
> shouldIgnoreAttributeCase = false) const;
> 
> Can we do without the default value? How many callers are there that rely on
> the default?

There is ~5/6 callers that do so. 

> What's the performance impact of this change? The function seems simple enough
> that I could imagine having multiple copies of it if there are callers who have
> no reason to pass the boolean, but on the other hand it's possibly slow enough
> already that this extra work has little impact.

I have not evaluated the performance issues. As there is a few callers and it is showing the peculiarities in how strings are handled to match an attribute (case sensitive or not, using lower case name or not, ...), I think it is better to simply remove the default value as you suggested.

>  82 function testAttribNodeNamePreservesCaseGetNode2() {
> 
> I'd prefer that even JavaScript follow our coding guidelines and put braces on
> a separate line for function definitions.
> 
>  92     if (!a) {
>  93         return "FAIL";
>  94     }
> 
> And omit braces for one line if statements.
> 

I will make it abide by our coding style rules.

[Julien Chaffraix]

Created attachment 22952 [details]
Updated version: tackles Darin's comments & removed one call to lower()

[Darin Adler]

Comment on attachment 22952 [details]
Updated version: tackles Darin's comments & removed one call to lower()

r=me

Why didn't you change Element::hasAttribute to remove the call to lower() there?

[Julien Chaffraix]

> Why didn't you change Element::hasAttribute to remove the call to lower()
> there?

I did try to remove this call too but as strange as it may seem, it is required: if removed we would fail several tests.

I have tried several approaches before finding this one that does not lead to any test failure. Maybe it could be removed but it is better for now to just keep it. I will add a comment about the possibility to remove the call before landing.

[Julien Chaffraix]

Landed in r35931 with a comment about the call to lower() in hasAttribute.

Thanks to Eric Roman for the nice reduced test case.

[Alexey Proskuryakov]

Note that the behavior still doesn't quite match Firefox though, according to the second attached test.

[Julien Chaffraix]

(In reply to comment #13)
> Note that the behavior still doesn't quite match Firefox though, according to
> the second attached test.
> 

We discussed this over IRC with Alexey. IE and Firefox have different behaviours so I have sided with IE (and Opera) when fixing the 2nd test case. So it is normal that we do not match Firefox on this one.

Sorry for not mentioning this in the first place.