RESOLVED FIXED 20247
setAttributeNode() does not work when attribute name has a capital letter in it
https://bugs.webkit.org/show_bug.cgi?id=20247
Summary setAttributeNode() does not work when attribute name has a capital letter in it
Eric Roman
Reported 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.
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
get/set test case (694 bytes, text/html)
2008-08-01 02:05 PDT, Alexey Proskuryakov
no flags
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
Proposed fix: add bool to getAttributeItem to tune case sensitivity (11.18 KB, patch)
2008-08-22 09:39 PDT, Julien Chaffraix
darin: review-
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+
Eric Roman
Comment 1 2008-07-31 17:58:05 PDT
Created attachment 22592 [details] sets attributes using setAttributeNode() -- those with capitals in their name fail
Eric Roman
Comment 2 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.
Alexey Proskuryakov
Comment 3 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.
Alexey Proskuryakov
Comment 4 2008-08-01 02:07:42 PDT
Eric Roman
Comment 5 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
Julien Chaffraix
Comment 6 2008-08-22 09:39:44 PDT
Created attachment 22938 [details] Proposed fix: add bool to getAttributeItem to tune case sensitivity
Darin Adler
Comment 7 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.
Julien Chaffraix
Comment 8 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.
Julien Chaffraix
Comment 9 2008-08-23 04:01:04 PDT
Created attachment 22952 [details] Updated version: tackles Darin's comments & removed one call to lower()
Darin Adler
Comment 10 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?
Julien Chaffraix
Comment 11 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.
Julien Chaffraix
Comment 12 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.
Alexey Proskuryakov
Comment 13 2008-12-03 03:54:17 PST
Note that the behavior still doesn't quite match Firefox though, according to the second attached test.
Julien Chaffraix
Comment 14 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.
Note You need to log in before you can comment on or make changes to this bug.