Last Comment Bug 162927 - getElementsByTagName returns HTMLCollection instead of NodeList
: getElementsByTagName returns HTMLCollection instead of NodeList
Status: RESOLVED FIXED
: dom1, dom2, testcase
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: P5 normal (vote)
: mozilla1.6beta
Assigned To: Peter Van der Beken [:peterv]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 143191
Blocks: 221988
  Show dependency treegraph
 
Reported: 2002-08-15 14:58 PDT by Dan Rosen
Modified: 2010-01-02 08:42 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (559 bytes, text/html)
2002-08-15 15:01 PDT, Dan Rosen
no flags Details
v1 (2.96 KB, patch)
2003-10-13 08:44 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
v1 (2.96 KB, patch)
2003-10-14 02:27 PDT, Peter Van der Beken [:peterv]
caillon: review+
jst: superreview+
Details | Diff | Splinter Review

Description Dan Rosen 2002-08-15 14:58:26 PDT
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 2002-08-15 15:01:41 PDT
Created attachment 95455 [details]
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 2002-08-15 15:04:43 PDT
Also note that this happens with XML documents as well.
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2002-08-15 15:49:25 PDT
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 [:bz] (still a bit busy) 2002-08-15 16:26:44 PDT
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, jst@mozilla.com) 2002-08-15 16:36:59 PDT
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 [:bz] (still a bit busy) 2002-08-15 16:56:28 PDT
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 2002-08-15 21:39:53 PDT
> 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 [:bz] (still a bit busy) 2003-03-12 20:57:01 PST
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 neil@parkwaycc.co.uk 2003-10-13 03:51:43 PDT
Note that instanceof does work with full interface names i.e.
alert(divs instanceof Components.interfaces.nsIDOMNodeList);
Comment 10 neil@parkwaycc.co.uk 2003-10-13 05:21:51 PDT
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] 2003-10-13 08:37:05 PDT
Taking.
Comment 12 Peter Van der Beken [:peterv] 2003-10-13 08:44:17 PDT
Created attachment 133196 [details] [diff] [review]
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 [:bz] (still a bit busy) 2003-10-13 22:55:30 PDT
Aarrgh.  Please don't hijack my bugs, ok, Neil?
Comment 14 neil@parkwaycc.co.uk 2003-10-14 01:24:49 PDT
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] 2003-10-14 02:27:31 PDT
Created attachment 133259 [details] [diff] [review]
v1

Didn't rediff before attaching so it missed a crucial change (&& -> ||). This
one works.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2003-10-15 01:20:34 PDT
Comment on attachment 133259 [details] [diff] [review]
v1

caillon may know something about this code; I do not...
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2003-10-17 10:29:30 PDT
Comment on attachment 133259 [details] [diff] [review]
v1

sr=jst
Comment 18 Christopher Aillon (sabbatical, not receiving bugmail) 2003-10-17 15:30:28 PDT
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] 2003-10-21 05:52:08 PDT
Attachment 133259 [details] [diff] was checked in. Bz, do you want this bug back?
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2003-10-21 08:53:57 PDT
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] 2003-10-29 08:47:11 PST
I'm going to mark this fixed. I think any remaining work (if needed) is covered
in bug 143191.
Comment 22 Sjoerd Visscher 2005-07-18 09:13:04 PDT
(fixed typo in summary so I can find this bug next time)
Comment 23 Jesse Ruderman 2010-01-01 22:16:48 PST
I think this is broken again, or still broken, or something.  See bug 14869.
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2010-01-02 08:42:11 PST
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?

Note You need to log in before you can comment on or make changes to this bug.