WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
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
<
rdar://problem/6118218
>
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.
Top of Page
Format For Printing
XML
Clone This Bug