The default bug view has changed. See this FAQ.

getElementsByTagName returns HTMLCollection instead of NodeList

RESOLVED FIXED in mozilla1.6beta

Status

()

Core
DOM: Core & HTML
P5
normal
RESOLVED FIXED
15 years ago
7 years ago

People

(Reporter: Dan Rosen, Assigned: peterv)

Tracking

({dom1, dom2, testcase})

Trunk
mozilla1.6beta
dom1, dom2, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

559 bytes, text/html
Details
v1
2.96 KB, patch
Christopher Aillon (sabbatical, not receiving bugmail)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
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.
(Reporter)

Comment 1

15 years ago
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.
(Reporter)

Comment 2

15 years ago
Also note that this happens with XML documents as well.
Keywords: dom1, dom2, testcase
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.
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.
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.
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...
Assignee: jst → bzbarsky
Depends on: 143191
Priority: -- → P5
Target Milestone: --- → Future
(Reporter)

Comment 7

15 years ago
> 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.
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

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

Comment 10

14 years ago
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.
(Assignee)

Comment 11

14 years ago
Taking.
Assignee: bzbarsky → peterv
(Assignee)

Updated

14 years ago
Target Milestone: Future → mozilla1.6beta
(Assignee)

Comment 12

14 years ago
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...).
(Assignee)

Updated

14 years ago
Attachment #133196 - Flags: superreview?(jst)
Attachment #133196 - Flags: review?(bzbarsky)
Aarrgh.  Please don't hijack my bugs, ok, Neil?

Comment 14

14 years ago
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 :-(
(Assignee)

Comment 15

14 years ago
Created attachment 133259 [details] [diff] [review]
v1

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

Updated

14 years ago
Attachment #133196 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #133259 - Flags: superreview?(jst)
Attachment #133259 - Flags: review?(bzbarsky)
(Assignee)

Updated

14 years ago
Attachment #133196 - Flags: superreview?(jst)
Attachment #133196 - Flags: review?(bzbarsky)

Updated

14 years ago
Blocks: 221988
Comment on attachment 133259 [details] [diff] [review]
v1

caillon may know something about this code; I do not...
Attachment #133259 - Flags: review?(bzbarsky) → review?(caillon)
Comment on attachment 133259 [details] [diff] [review]
v1

sr=jst
Attachment #133259 - Flags: superreview?(jst) → superreview+
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
Attachment #133259 - Flags: review?(caillon) → review+
(Assignee)

Comment 19

14 years ago
Attachment 133259 [details] [diff] was checked in. Bz, do you want this bug back?
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.
(Assignee)

Comment 21

14 years ago
I'm going to mark this fixed. I think any remaining work (if needed) is covered
in bug 143191.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 22

12 years ago
(fixed typo in summary so I can find this bug next time)
Summary: getElementByTagName returns HTMLCollections instead of NodeLists → getElementsByTagName returns HTMLCollection instead of NodeList

Updated

9 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: stummala → general

Comment 23

7 years ago
I think this is broken again, or still broken, or something.  See bug 14869.
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?
You need to log in before you can comment on or make changes to this bug.