162927 - getElementsByTagName returns HTMLCollection instead of NodeList

Status: RESOLVED FIXED

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

Description

[Dan Rosen]

The |getElementByTagName| method of DOM |Document| returns objects that report
themselves to XPConnect as |HTMLCollection|s rather than the |NodeList|s they're
supposed to be.

Not sure where this bug goes (DOM or XPConnect), and I'm not entirely positive
this is a bug -- but it is unexpected behavior.

Oddly, the IDL for these is correct, but if I query the objects in JavaScript
using |instanceof| I get the broken results. I'll attach a testcase in a second.

Comment 1

[Dan Rosen]

Attachment: Testcase

This testcase has two <div>s and two buttons. Clicking one button will get the
divs using |getElementsByTagName| and see if the list returned is an instanceof
NodeList. Clicking the other will get them and see if the list returned is an
instanceof HTMLCollection.

The NodeList button should show "true" and the HTMLCollection button should
show "false," but the opposite is currently the case.

Comment 2

[Dan Rosen]

Also note that this happens with XML documents as well.

Comment 3

[Johnny Stenback (:jst)]

This really doesn't matter since the objects we return, even if their JS class
name is HTMLCollection, do implement the nsIDOMNodeList interface so we're not
breaking any standards here. I wouldn't fix this, but if bz wants to fix this he
can take the bug, if not, please mark this WONTFIX.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So...  The problem is that the resulting collection _must_ implement
HTMLCollection for compat with older versions of Mozilla and with IE.  The
object also implements NodeList, since the functionality on this interface is a
subset of that on HTMLCollection (and this would all be a non-issue if DOM1 HTML
had made HTMLCollection extend NodeList).

I get "false" for both buttons in that testcase, by the way, so I don't even
have a way to test possible "easy fixes" usefully.... if the fix for this is not
easy, I would be in favor of not bothering.

Comment 5

[Johnny Stenback (:jst)]

We'd need to split up nsContentList to fix this and even then we'd probably only
want to fix this for XML documents (where the fix even kinda almost matters, at
least a little bit). But no, there's no one-line fix for this.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK.  I'll take this for now, and if something good happens as I fix bug 143191,
so be it.  But I wouldn't hold out too much hope...

Comment 7

[Dan Rosen]

> This really doesn't matter since the objects we return, even if their JS class
> name is HTMLCollection, do implement the nsIDOMNodeList interface

My understanding of JS (or at least JS1.5) is that the |instanceof| operator
will return true for any type implemented by an object, not just the principal type:

http://devedge.netscape.com/library/manuals/2000/javascript/1.5/reference/ops.html#1055015

For example:

  var a = new Array();
  a instanceof Array   ==> true
  a instanceof Object  ==> true

If I'm not mistaken (and again, I might be -- my knowledge of how XPConnect
plays with XPCOM is pretty slim), the same should go for interfaces.

So I guess I'm saying that regardless of how nsContentList is split up, or what
classes happen to implement what interfaces, I want to be able to use instanceof
in the general case to query an interface. Perhaps I should just be using QI :)

> we'd probably only want to fix this for XML documents (where the fix even
> kinda almost matters, at least a little bit).

From a persnickety point of view, DOM HTML's getElementsByTagName also returns
NodeLists, not HTMLCollections... Just to pick nits...

> I get "false" for both buttons in that testcase, by the way, so I don't even
> have a way to test possible "easy fixes" usefully....

Weird... I observe this in 1.0 release and 1.1alpha. I don't have a current
build at the moment.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So... jst?  We have nsContentList being QIable to nsIDOMNodeList (by inheriting
QI from nsBaseContentList).  It has classinfo for nsIDOMNodeList.  Shouldn't
that make instanceof do the right thing?  Maybe what we should really do is fix
instanceof to handle this right....

Comment 9

[[e-mail]]

Note that instanceof does work with full interface names i.e.
alert(divs instanceof Components.interfaces.nsIDOMNodeList);

Comment 10

[[e-mail]]

peterv claims this is a separate bug but I'll let you gurus decide...

document.body instanceof Node
document.body instanceof Components.interfaces.nsIDOMNode
document.body instanceof Components.interfaces.nsIDOMElement

all return true, but

document.body instanceof Element

returns false.

Comment 11

[Peter Van der Beken [:peterv]]

Taking.

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: v1

This fixes instanceof for a bunch of DOM objects. Note that with this patch the
testcase shows true for both |instanceof HTMLCollection| and |instanceof
NodeList| so this bug isn't really fixed (but since Neil refuses to open a new
bug...).

Comment 13

[Boris Zbarsky [:bzbarsky]]

Aarrgh.  Please don't hijack my bugs, ok, Neil?

Comment 14

[[e-mail]]

Comment on attachment 133196 [details] [diff] [review]
v1

Do you want the good news first or the bad news first?

The good news is that document.body instanceof Element now returns true :-)

The bad news is that document instanceof Document and document.body instanceof
Node now return false :-(

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: v1

Didn't rediff before attaching so it missed a crucial change (&& -> ||). This
one works.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133259 [details] [diff] [review]
v1

caillon may know something about this code; I do not...

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 133259 [details] [diff] [review]
v1

sr=jst

Comment 18

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 133259 [details] [diff] [review]
v1

>+  const nsIID *class_iid;
>+  if (class_name_struct->mType == nsGlobalNameStruct::eTypeInterface ||
>+      class_name_struct->mType == nsGlobalNameStruct::eTypeClassProto) {
>+    class_iid = &class_name_struct->mIID;
>+  } else if (class_name_struct->mType == nsGlobalNameStruct::eTypeClassConstructor) {
>+    class_iid =
>+      sClassInfoData[class_name_struct->mDOMClassInfoID].mProtoChainInterface;
>+  } else if (class_name_struct->mType == nsGlobalNameStruct::eTypeExternalClassInfo) {
>+    class_iid = class_name_struct->mData->mProtoChainInterface;
>+  } else if (class_name_struct->mType == nsGlobalNameStruct::eTypeExternalConstructorAlias) {
>+    const nsGlobalNameStruct* alias_struct =
>+      gNameSpaceManager->GetConstructorProto(class_name_struct);
>+    if (!alias_struct) {
>+      NS_ERROR("Couldn't get constructor prototype.");
>+      nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_UNEXPECTED);
>+
>+      return JS_FALSE;
>+    }
>+
>+    if (alias_struct->mType == nsGlobalNameStruct::eTypeClassConstructor) {
>+      class_iid =
>+        sClassInfoData[alias_struct->mDOMClassInfoID].mProtoChainInterface;
>+    } else if (alias_struct->mType == nsGlobalNameStruct::eTypeExternalClassInfo) {
>+      class_iid = alias_struct->mData->mProtoChainInterface;
>+    } else {
>+      NS_ERROR("Expected eTypeClassConstructor or eTypeExternalClassInfo.");
>+      nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_UNEXPECTED);
>+
>+      return JS_FALSE;
>+    }
>+  } else {
>+    *bp = JS_FALSE;


Cool.  This will probably be easier to read with switch statements though
instead of if/elseif/elseif/elseif/else fun. It might even save some code size
to do so.  (I'm not sure that all compilers we use are smart enough to figure
out that it only needs to dereference class_name_struct->mType and
alias_struct->mType once each)

r=caillon

Comment 19

[Peter Van der Beken [:peterv]]

Attachment 133259 [details] [diff] was checked in. Bz, do you want this bug back?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Not at all, since I think that the way it now works is correct -- the object is
indeed an instance of both.  There is no reason for it not to be.

Comment 21

[Peter Van der Beken [:peterv]]

I'm going to mark this fixed. I think any remaining work (if needed) is covered
in bug 143191.

Comment 22

[Sjoerd Visscher]

(fixed typo in summary so I can find this bug next time)

Comment 23

[Jesse Ruderman]

I think this is broken again, or still broken, or something.  See bug 14869.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Presumably broken again; this worked as of when Peter checked in the patch, using the testcase in this bug.  Want to hunt down when it broke?