Bug 20247 - setAttributeNode() does not work when attribute name has a capital letter in it
Summary: setAttributeNode() does not work when attribute name has a capital letter in it
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-07-31 17:57 PDT by Eric Roman
Modified: 2008-12-12 03:35 PST (History)
2 users (show)

See Also:


Attachments
sets attributes using setAttributeNode() -- those with capitals in their name fail (933 bytes, text/html)
2008-07-31 17:58 PDT, Eric Roman
no flags Details
get/set test case (694 bytes, text/html)
2008-08-01 02:05 PDT, Alexey Proskuryakov
no flags Details
Several attribute tests, with static compatibility summary for firefox2, firefox3, opera9.5, safari3.1, ie7 (7.02 KB, text/html)
2008-08-01 14:07 PDT, Eric Roman
no flags Details
Proposed fix: add bool to getAttributeItem to tune case sensitivity (11.18 KB, patch)
2008-08-22 09:39 PDT, Julien Chaffraix
darin: review-
Details | Formatted Diff | Diff
Updated version: tackles Darin's comments & removed one call to lower() (12.90 KB, patch)
2008-08-23 04:01 PDT, Julien Chaffraix
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Roman 2008-07-31 17:57:06 PDT
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.
Comment 1 Eric Roman 2008-07-31 17:58:05 PDT
Created attachment 22592 [details]
sets attributes using setAttributeNode() -- those with capitals in their name fail
Comment 2 Eric Roman 2008-07-31 22:25:16 PDT
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.
Comment 3 Alexey Proskuryakov 2008-08-01 02:05:09 PDT
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.
Comment 4 Alexey Proskuryakov 2008-08-01 02:07:42 PDT
<rdar://problem/6118218>
Comment 5 Eric Roman 2008-08-01 14:07:57 PDT
Created attachment 22610 [details]
Several attribute tests, with static compatibility summary for firefox2, firefox3, opera9.5, safari3.1, ie7
Comment 6 Julien Chaffraix 2008-08-22 09:39:44 PDT
Created attachment 22938 [details]
Proposed fix: add bool to getAttributeItem to tune case sensitivity
Comment 7 Darin Adler 2008-08-22 09:49:10 PDT
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.
Comment 8 Julien Chaffraix 2008-08-22 20:15:53 PDT
> 
>  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.
Comment 9 Julien Chaffraix 2008-08-23 04:01:04 PDT
Created attachment 22952 [details]
Updated version: tackles Darin's comments & removed one call to lower()
Comment 10 Darin Adler 2008-08-23 11:40:30 PDT
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?
Comment 11 Julien Chaffraix 2008-08-23 17:52:35 PDT
> 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.
Comment 12 Julien Chaffraix 2008-08-26 03:31:54 PDT
Landed in r35931 with a comment about the call to lower() in hasAttribute.

Thanks to Eric Roman for the nice reduced test case.
Comment 13 Alexey Proskuryakov 2008-12-03 03:54:17 PST
Note that the behavior still doesn't quite match Firefox though, according to the second attached test.
Comment 14 Julien Chaffraix 2008-12-12 03:35:25 PST
(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.