Closed Bug 206053 Opened 21 years ago Closed 18 years ago

[FIX]document.getElementsByTagName('tagname') with XML document wrongly includes elements with namespace prefix in the tag name

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: martin.honnen, Assigned: bzbarsky)

References

Details

(Keywords: dev-doc-complete)

Attachments

(6 files, 2 obsolete files)

When calling
  xmlDocument.getElementsByTagName('tagname')
Mozilla DOM implementation includes elements in the resulting NodeList which
have a prefix in the nodeName, e.g. elements alike
  <prefix:tagname xmlns:prefix="someURI" />
are included in the NodeList.
That is wrong in my view, and is also not the case in the Java DOM
implementation that comes with Sun JDK 1.4.
I will upload a test case consisting of an example XML documents that uses
namespaces and an HTML page that loads the XML documents and checks for certain
elements with xmlDocument.getElementsByTagName.
Attached file test case (obsolete) —
Explanation of test case output: for the XML

<gods xmlns="http://vatican.va/2003/gods">  
  <gods:god xmlns:gods="http://vatican.va/2003/gods">Kibo</gods:god>
  <god>Maho</god>
</gods>

the output is

xmlDocument.getElementsByTagName("god").length: 2
xmlDocument.getElementsByTagNameNS("http://vatican.va/2003/gods", "god").length: 2

while there is only one element with tag name "god", the other element has a tag
name of "gods:god".

For the XML

<gods xmlns="http://vatican.va/2003/gods">
  <gods:god xmlns:gods="http://vatican.va/2003/gods">Kibo</gods:god>
  <god>Maho</god>
  <god>Jaffo</god>
</gods>

the output is

xmlDocument.getElementsByTagName("god").length: 3
xmlDocument.getElementsByTagNameNS("http://vatican.va/2003/gods", "god").length: 3

while there are only two elements with tag name "god".

For the XML

<gods xmlns="http://vatican.va/2003/gods">
  <gods:god xmlns:gods="http://vatican.va/2003/gods">Kibo</gods:god>
  <god>Maho</god>
  <god>Jaffo</god>
  <aPrefix:god xmlns:aPrefix="http://vatican.va/2003/devils">Xibo</aPrefix:god>
</gods>

the output is

xmlDocument.getElementsByTagName("god").length: 4
xmlDocument.getElementsByTagNameNS("http://vatican.va/2003/gods", "god").length: 3

while there are only two elements with tag name "god", the other elements having
a tag name "gods:god" respectively "aPrefix:god".
This Java program is comparable to the HTML/JavaScript test case and shows the
difference in the implementation of xmlDocument.getElementsByTagName of Mozilla
and the Sun Java JDK 1.4. Both pretend to implement the W3C DOM Level 2 Core
and thus the results should be the same. I think the Java implementation is
correct and the Mozilla implementation needs to be fixed.

Here is the output of the Java program when run on the console with Win 98:

xmlDocument.getElementsByTagName("god").getLength(): 1
xmlDocument.getElementsByTagNameNS("http://vatican.va/2003/gods",
"god").getLeng
th(): 2
xmlDocument.getElementsByTagName("god").getLength(): 2
xmlDocument.getElementsByTagNameNS("http://vatican.va/2003/gods",
"god").getLeng
th(): 3
xmlDocument.getElementsByTagName("god").getLength(): 2
xmlDocument.getElementsByTagNameNS("http://vatican.va/2003/gods",
"god").getLeng
th(): 3
The test case
http://bugzilla.mozilla.org/attachment.cgi?id=123587&action=view
shows that the problem with getElementsByTagName is probably caused by a wrong
value for the tagName property in Mozilla's DOM implementation. The W3C DOM
Level 2 Core specification in the description of createElementNS says that the
tagName should be the qualified name, it seems Mozilla wrongly sets the local
name as the tagName.
I have updated the bug summary to reflect this.
Summary: document.getElementsByTagName('tagname') with XML document wrongly includes elements with namespace prefix → document.getElementsByTagName('tagname') with XML document wrongly includes elements with namespace prefix in the tag name as the tagName property only contains the local name and not the qualified name
OS: Windows 98 → All
Hardware: PC → All
bz, this is due to nsContentList not knowing what prefix to match against, we'll
need some lowlevel nsContentListKey hacking here. Got some cycles?
Well, it's sounding like getElementsByTagName should just never find anything
that's not in the null namespace in XML docs... is that the case?  If so, this
should be pretty simple to do...
No, there's more to it than that. document.getElementsByTagName() should find
elements whose nodeName (e.g. "foo:bar") matches what's passed in, regardless of
the elements namespace (prefix or no prexix). I don't think this can be done w/o
holding on to a prefix one way or anyother in nsContentListKey.
Ah, I see what you mean.

nsContentListKey already holds on to a namespace id... We could simply change
the GetElementsByTagName code to look for a ':' in the tagname and convert it to
a namespace id and a localname if that happens, no?  If there is no ':', we use
the null namespace (HTML elements (not XHTML) report themselves as being in the
null namespace, right?)
No, unfortunately that won't work. The prefix can map to different namespaces in
different parts of the tree (XML namespaces can be redeclared). We need to know
the prefix and the namespace in nsContentListKey, but never both at the same
time (that I can think of). And yes, HTML elements are in the null namespace
(but make sure you ask the elements nodeinfo, not nsIContent::GetNameSpaceID()).
Oh, right...  Bleh.

Well, I won't have time to do this anytime soon (esp. what with the fact that
I'm still trying to be on vacation), but feel free to assign to me...
Flags: blocking1.9a2?
Attachment #123587 - Attachment is obsolete: true
Attached patch FixSplinter Review
The tagName testcase already passes; this makes us pass the other testcase.  Note that this does change the behavior of getElementsByTagName (to follow spec).  Also note that this makes getElementsByTagName slower if it has to deal with elements with non-empty prefixes.  I think that's ok, myself.
Assignee: general → bzbarsky
Status: NEW → ASSIGNED
Attachment #218840 - Flags: superreview?(jst)
Attachment #218840 - Flags: review?(peterv)
Priority: -- → P2
Summary: document.getElementsByTagName('tagname') with XML document wrongly includes elements with namespace prefix in the tag name as the tagName property only contains the local name and not the qualified name → [FIX]document.getElementsByTagName('tagname') with XML document wrongly includes elements with namespace prefix in the tag name
Target Milestone: --- → mozilla1.9alpha
Attachment #218840 - Flags: review?(peterv) → review+
Comment on attachment 218840 [details] [diff] [review]
Fix

sr=jst
Attachment #218840 - Flags: superreview?(jst) → superreview+
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
searchWidgets.xml uses getElementsByTagName to iterate over anonymous xul:menuitem elements, should this now use the NS version?
Yeah.  Sorry I didn't catch that when I looked at in-tree uses.  :(
bz or anyone else, feel free to comment.
Attachment #223159 - Flags: superreview?(mscott)
Attachment #223159 - Flags: review?
Comment on attachment 223159 [details] [diff] [review]
Fix for searchWidgets.xml

thanks a lot for fixing this Neil.
Attachment #223159 - Flags: superreview?(mscott) → superreview+
Comment on attachment 223159 [details] [diff] [review]
Fix for searchWidgets.xml

Looks ok, but I'd probably just hardcode the XUL namespace, in case this binding gets applied to a random non-XUL element...
Attachment #223159 - Flags: review? → review+
(In reply to comment #22)
>(From update of attachment 223159 [details] [diff] [review])
>Looks ok, but I'd probably just hardcode the XUL namespace, in case this
>binding gets applied to a random non-XUL element...
In the unlikely event whomever is responsible can just deal :-P
How, exactly?  The point is, the code is conceptually wrong.  We really do want the XUL namespace, not the bound node namespace, esp. given xbl:extends.  Please make the change I asked for; the review is conditional on it.
Flags: blocking1.9a2?
Depends on: 343307
This caused bug 343307.
This has come up on the mailing list, so I mentioned this change on http://developer.mozilla.org/en/docs/DOM:element.getElementsByTagName

Probably worth a mention on the Firefox 3 page as well.
Flags: in-testsuite?
Keywords: dev-doc-needed
Do the tests in http://lxr.mozilla.org/seamonkey/source/content/test/unit/test_nodelist.js cover this being in a testsuite?
Added a reference to this issue to the Firefox 3 for developers page; tagging as doc complete.
Component: DOM: Core → DOM: Core & HTML
QA Contact: desale → general
You need to log in before you can comment on or make changes to this bug.