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

Status: RESOLVED FIXED

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

Description

[Martin Honnen]

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.

Comment 1

[Martin Honnen]

Attachment: example XML document needed for test case

Comment 2

[Martin Honnen]

Attachment: test case

Comment 3

[Martin Honnen]

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".

Comment 4

[Martin Honnen]

Attachment: comparable Java test case

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

Comment 5

[Martin Honnen]

Attachment: test case that checks tagName property of element with namespace prefix in the tag name

Comment 6

[Martin Honnen]

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.

Comment 7

[Johnny Stenback (:jst)]

bz, this is due to nsContentList not knowing what prefix to match against, we'll
need some lowlevel nsContentListKey hacking here. Got some cycles?

Comment 8

[Boris Zbarsky [:bzbarsky]]

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...

Comment 9

[Johnny Stenback (:jst)]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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?)

Comment 11

[Johnny Stenback (:jst)]

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()).

Comment 12

[Boris Zbarsky [:bzbarsky]]

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...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated version of "test case" that should work with the new bugzilla URI

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Update version of "tagName" testcase

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

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.

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 218840 [details] [diff] [review]
Fix

sr=jst

Comment 17

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 18

[[e-mail]]

searchWidgets.xml uses getElementsByTagName to iterate over anonymous xul:menuitem elements, should this now use the NS version?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Yeah.  Sorry I didn't catch that when I looked at in-tree uses.  :(

Comment 20

[[e-mail]]

Attachment: Fix for searchWidgets.xml

bz or anyone else, feel free to comment.

Comment 21

[Scott MacGregor]

Comment on attachment 223159 [details] [diff] [review]
Fix for searchWidgets.xml

thanks a lot for fixing this Neil.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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...

Comment 23

[[e-mail]]

(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

Comment 24

[Boris Zbarsky [:bzbarsky]]

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.

Comment 25

[Boris Zbarsky [:bzbarsky]]

This caused bug 343307.

Comment 26

[Nickolay_Ponomarev]

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.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Do the tests in http://lxr.mozilla.org/seamonkey/source/content/test/unit/test_nodelist.js cover this being in a testsuite?

Comment 28

[Nickolay_Ponomarev]

I think they do, yes: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/test/unit/test_nodelist.js&rev=&cvsroot=/cvsroot&mark=34-35,42-43#34

Comment 29

[Eric Shepherd [:sheppy]]

Added a reference to this issue to the Firefox 3 for developers page; tagging as doc complete.