Bug 6648

Summary: Safari strips namespace prefix when using setAttribute(), but should treat it as part of name
Product: WebKit Reporter: Sjoerd Mulder <sjoerdmulder>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: ap, boris, cdumez, darin
Priority: P2    
Version: 420+   
Hardware: PC   
OS: Windows XP   
Bug Depends on:    
Bug Blocks: 6628    
Attachments:
Description Flags
Testcase to reproduce
none
Patch
darin: review+
ChangeLog entry
none
Layout test
none
Layout test - expected result
none
test case to examine behavior of get/set/has/removeAttribute and get/set/has/removeNS none

Sjoerd Mulder
Reported 2006-01-18 00:32:51 PST
When you try to set the attribute 'b:name' using elm.setAttribute('b:name','myvalue'); Safari strips off the namespace, it set the attribute 'name'. All other browsers (IE6, Moz and Opera) are working correctly (setting 'b:name').
Attachments
Testcase to reproduce (630 bytes, text/html)
2006-01-18 00:33 PST, Sjoerd Mulder 		no flags
Details
Patch (1.84 KB, patch)
2006-02-03 19:12 PST, Boris Daljevi&#263; 		darin: review+
DetailsFormatted DiffDiff
ChangeLog entry (365 bytes, text/plain)
2006-02-03 19:14 PST, Boris Daljevi&#263; 		no flags
Details
Layout test (961 bytes, text/html)
2006-02-03 19:15 PST, Boris Daljevi&#263; 		no flags
Details
Layout test - expected result (144 bytes, text/plain)
2006-02-03 19:16 PST, Boris Daljevi&#263; 		no flags
Details
test case to examine behavior of get/set/has/removeAttribute and get/set/has/removeNS (5.80 KB, text/html)
2006-02-06 05:47 PST, Boris Daljevi&#263; 		no flags
Details
View All Add attachment proposed patch, testcase, etc.
Sjoerd Mulder
Comment 1 2006-01-18 00:33:22 PST
Created attachment 5759 [details] Testcase to reproduce
Sjoerd Mulder
Comment 2 2006-01-18 00:37:40 PST
Comment on attachment 5759 [details] Testcase to reproduce ><!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN" "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd"> ><HTML xmlns:s="http://www.backbase.com/s" xmlns:b="http://www.backbase.com/b" xmlns="http://www.w3.org/1999/xhtml" xml:lang="en"><HEAD> > > > > <META content="text/html; utf-8" http-equiv="Content-Type"/><TITLE>Safari setAttribute testcase</TITLE></HEAD><BODY> > <SCRIPT type="text/javascript"> > var elm = document.createElement('div'); > elm.setAttribute('b:name','myvalue'); > alert("b:name='"+elm.getAttribute('b:name')+ "' - name='"+elm.getAttribute('name')+"'"); > </SCRIPT> > ></BODY></HTML>
Sjoerd Mulder
Comment 3 2006-01-18 03:37:03 PST
Actually, it seems that problem is in getAttribute/getAttributeNode, because if you do: oElm.setAttribute('b:name','myvalue') var oAttr = oElm.getAttributeNode('name'); alert(oAttr.prefix + ':' + oAttr.localName + ' = ' + oAttr.nodeValue); Result message will be "b:name = myvalue".
Alexey Proskuryakov
Comment 4 2006-01-18 21:35:55 PST
A workaround is to use getAttributeNS/setAttributeNS.
Boris Daljevi&#263;
Comment 5 2006-02-03 19:12:15 PST
Created attachment 6231 [details] Patch
Boris Daljevi&#263;
Comment 6 2006-02-03 19:13:05 PST
Problem is in setAttribute function which parses argument "name", though it shouldn't according to DOM specification. Attached is the patch that fixes the problem, LayoutTest and ChangeLog entry.
Boris Daljevi&#263;
Comment 7 2006-02-03 19:14:48 PST
Created attachment 6232 [details] ChangeLog entry
Boris Daljevi&#263;
Comment 8 2006-02-03 19:15:43 PST
Created attachment 6233 [details] Layout test
Boris Daljevi&#263;
Comment 9 2006-02-03 19:16:36 PST
Created attachment 6234 [details] Layout test - expected result
Darin Adler
Comment 10 2006-02-03 19:40:18 PST
Comment on attachment 6231 [details] Patch I don't understand this patch. The getAttribute, setAttribute, and removeAttribute implementations all do the same thing, calling through to get/set/removeAttributeNS with a namespace URI of null. How can it possibly be correct to change only setAttribute and not the other two? If you can answer that question, it seems OK to set this to review? again. By the way, it's best to include the change log entry and the layout test case in the patch.
Boris Daljevi&#263;
Comment 11 2006-02-06 05:35:09 PST
Yes, thay all behave simmilar, but in the implementation setAttributeNS there is call to parseQualifiedName, which is fine when the call comes directly from setAttributeNS DOM dunction, but is not OK when it comes from setAttribute DOM function. get/has/removeAttributeNS differ in that. Maybe you didn't understand it because there are few overloaded setAtrribute methods of ElementImpl. Try to follow execution flow for the setAttribute('bb:dddd', 'whatever'); DOM call. 1. ElementImpl::setAttribute(const DOMString &name, const DOMString &value, int &exception) is called with parametar name that virtualy equals to "bb:dddd" 2. ElementImpl::setAttributeNS(const DOMString &namespaceURI, const DOMString &qualifiedName, const DOMString &value, int &exception) is called with namespaceURI that virtualy equals to empty string and qualifiedName (which is here of type DOMString!) "bb:dddd" 2.a) within the body of method setAttributeNS, method DocumentImpl::parseQualifiedName is called, and this is the place where things go wrong. "bb:dddd" is split into prefix "bb" and localName "dddd". According to DOM specification that shouldn't happen, string should be treated just as it is full name (colomn is not considered separator, but just another character that is part of the localName). Implemetation of setAttribute DOM function in other browsers confirm this. 2.b) void ElementImpl::setAttribute(const QualifiedName& name, const DOMString &value) is called with name.prefix = "bb" name.localName = "dddd"... I am attaching nice test case that you can run in different browsers and see results of their implementation of get/set/has/removeAttribute and get/set/has/removeAttributeNS. Btw, I tried to include ChangeLog and the layout test case in the patch, but svn-create-patch script from WebKitTools/Scripts didn't include layout test. I guess because files are new and not yet in svn. Any further suggestion?
Boris Daljevi&#263;
Comment 12 2006-02-06 05:47:50 PST
Created attachment 6286 [details] test case to examine behavior of get/set/has/removeAttribute and get/set/has/removeNS Note that if you set attribute with setAttributeNS (that is if the first radiobutton in attached testcase is checked) according to DOM Level 2 (and 3), using get/has/removeAttribute functions can lead to unpredictable results. (Therefore differences in different browsers)
Alexey Proskuryakov
Comment 13 2006-02-06 06:33:01 PST
(In reply to comment #11) > Btw, I tried to include ChangeLog and the layout test case in the patch, but > svn-create-patch script from WebKitTools/Scripts didn't include layout test. I > guess because files are new and not yet in svn. Any further suggestion? With svn, it's possible to add files locally without having write access to the repository (svn add <filenames>). After this, svn-create-patch can create patches with new files.
Sjoerd Mulder
Comment 14 2006-02-09 23:51:14 PST
This bug is not fixed(tested in r12724). See the latest testcase the result, when setAttribute('bb:dddd') is that getAttribute('bb:dddd') works and not getAttribute('dddd')
Sjoerd Mulder
Comment 15 2006-02-10 04:44:20 PST
My fault... in current source(r12733) it works correctly!
Sjoerd Mulder
Comment 16 2006-03-08 08:50:10 PST
The getAttribute function still doesnt work probally when using the DOMParser <script type="text/javascript"> var sXml = '<div xmlns="http://www.w3.org/1999/xhtml" xmlns:b="http://www.backbase.com/b" b:name="stylesheet" />'; if(document.all){ var oXml = new ActiveXObject('Microsoft.XMLDOM'); oXml.loadXML(sXml); var oRes = oXml.documentElement; }else var oRes = new DOMParser().parseFromString(sXml, 'text/xml').documentElement; alert('attributes[2].name: '+oRes.attributes[2].name+' == attributes[2].value: '+oRes.attributes[2].value); alert('getAttribute(\'name\'): '+oRes.getAttribute('name')+' == getAttribute(\'b:name\'): '+oRes.getAttribute('b:name')); </script>
Darin Adler
Comment 17 2006-03-08 09:01:39 PST
(In reply to comment #16) > The getAttribute function still doesnt work probally when using the DOMParser > <script type="text/javascript"> > var sXml = '<div xmlns="http://www.w3.org/1999/xhtml" > xmlns:b="http://www.backbase.com/b" b:name="stylesheet" />'; > if(document.all){ > var oXml = new ActiveXObject('Microsoft.XMLDOM'); > oXml.loadXML(sXml); > var oRes = oXml.documentElement; > }else > var oRes = new DOMParser().parseFromString(sXml, > 'text/xml').documentElement; > alert('attributes[2].name: '+oRes.attributes[2].name+' == > attributes[2].value: '+oRes.attributes[2].value); > alert('getAttribute(\'name\'): '+oRes.getAttribute('name')+' == > getAttribute(\'b:name\'): '+oRes.getAttribute('b:name')); > </script> OK. Then please file a new bug with a suitable test case. Don't reopen this one. This one is something we already fixed, and landed with a regression test. It would be fine to reopen this one if this fix didn't work.
Sjoerd Mulder
Comment 18 2006-03-08 09:07:12 PST
Well apearantly this fix did work in certain circumstances, but when you transform a string to 'xml document' with the DOMParser and then request certain attributes the fix doesnt work, that's why i reopened this bug. This bug is now also In Radar <rdar://4470844>
Darin Adler
Comment 19 2006-03-08 10:40:54 PST
Reopening the same bug over and over again makes it much harder to keep track of it. The original bug didn't mention DOMParser at all. I think it makes it harder for people working to investigate and fix the bugs to keep things clear if you now add this DOMParser issue to the same bug report, even though it's a similar problem. For example, this shows up in the "bug with an approved patch" list. And the test cases attached to the bug all work, there's no test case for the new problem. It's *much*, *much* easier to fix the bug with a new bug report. Is there a reason you prefer reopening this one to writing a new one?
Sjoerd Mulder
Comment 20 2006-03-09 00:36:28 PST
Added: Bug 7677: getAttribute cant get attribute with a prefix when using the DOMParser
Lucas Forschler
Comment 21 2019-02-06 09:03:32 PST
Mass moving XML DOM bugs to the "DOM" Component.
Note You need to log in before you can comment on or make changes to this bug.