Last Comment Bug 357450 - Implement getElementsByClassName
: Implement getElementsByClassName
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- enhancement with 1 vote (vote)
: mozilla1.9
Assigned To: Robert Sayre
: Hixie (not reading bugmail)
Mentors:
http://whatwg.org/specs/web-apps/curr...
Depends on: 390411 395915 399334 iWeb 421015 433833
Blocks:
  Show dependency treegraph
 
Reported: 2006-10-20 16:05 PDT by Robert Sayre
Modified: 2016-03-17 16:05 PDT (History)
23 users (show)
sayrer: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP - Unpack the Array in order to call nsContentList (14.65 KB, patch)
2006-12-05 15:24 PST, Robert Sayre
no flags Details | Diff | Splinter Review
WIP - Refactor nsContentList matching functions (34.97 KB, patch)
2006-12-05 20:31 PST, Robert Sayre
no flags Details | Diff | Splinter Review
WIP - Fix bugs bz spotted (35.72 KB, patch)
2006-12-06 09:25 PST, Robert Sayre
no flags Details | Diff | Splinter Review
WIP - works on HTML elements, added mochitest (37.37 KB, patch)
2006-12-06 12:32 PST, Robert Sayre
no flags Details | Diff | Splinter Review
WIP - helper class style (46.87 KB, patch)
2006-12-07 20:21 PST, Robert Sayre
no flags Details | Diff | Splinter Review
WIP - helper class style, with new files (52.84 KB, patch)
2006-12-07 21:19 PST, Robert Sayre
no flags Details | Diff | Splinter Review
WIP - fix build bustage in TestXMLExtras.cpp (50.49 KB, patch)
2006-12-08 08:43 PST, Robert Sayre
no flags Details | Diff | Splinter Review
WIP - works. now for cleanup. (59.77 KB, patch)
2006-12-08 11:28 PST, Robert Sayre
no flags Details | Diff | Splinter Review
getElementsByClassName (62.12 KB, patch)
2006-12-08 13:32 PST, Robert Sayre
no flags Details | Diff | Splinter Review
getElementsByClassName (62.11 KB, patch)
2006-12-08 13:42 PST, Robert Sayre
no flags Details | Diff | Splinter Review
getElementsByClassName (64.44 KB, patch)
2006-12-11 11:33 PST, Robert Sayre
no flags Details | Diff | Splinter Review
everything in the tearoff (64.71 KB, patch)
2006-12-11 16:08 PST, Robert Sayre
no flags Details | Diff | Splinter Review
getElementsByClassName (61.12 KB, patch)
2006-12-11 18:00 PST, Robert Sayre
jst: review-
Details | Diff | Splinter Review
getElementsByClassName, string argument only (47.91 KB, patch)
2007-01-16 14:45 PST, Robert Sayre
no flags Details | Diff | Splinter Review
nits fixed (47.93 KB, patch)
2007-01-16 14:56 PST, Robert Sayre
no flags Details | Diff | Splinter Review
fixed flubbed null check (47.93 KB, patch)
2007-01-16 15:55 PST, Robert Sayre
no flags Details | Diff | Splinter Review
do the idl files right (50.79 KB, patch)
2007-01-16 21:16 PST, Robert Sayre
jst: review+
Details | Diff | Splinter Review
address jst's comments (50.89 KB, patch)
2007-01-22 20:53 PST, Robert Sayre
peterv: superreview+
Details | Diff | Splinter Review
patch to check in (42.46 KB, patch)
2007-02-01 07:07 PST, Robert Sayre
no flags Details | Diff | Splinter Review

Description Robert Sayre 2006-10-20 16:05:49 PDT
getElementsByClassName would be nice to have for our own use. 

getElementsByCSSSelector would be the more expressive alternative that would require me to write another language inside a string literal. This is not that bug.
Comment 1 Robert Sayre 2006-10-20 16:22:53 PDT
Use cases: 

http://www.google.com/search?q=getElementsByClassName
Results 1 - 10 of about 430,000 for getElementsByClassName.
Comment 2 Myk Melez [:myk] [@mykmelez] 2006-10-20 16:41:09 PDT
One could use this to extract microformats from a page with a single function call, f.e. this call, which grabs all hCards:

document.getElementsByClassName("vcard");
Comment 3 Robert Sayre 2006-10-20 16:51:44 PDT
Turns out we have a standard for this too.

http://whatwg.org/specs/web-apps/current-work/#getelementsbyclassname

Sweet.
Comment 4 Robert Sayre 2006-10-20 17:08:04 PDT
In the spec, Hixie says:

"There is an open issue on whether we should use multiple arguments or just one argument that needs to be split on spaces."

I think we should allow both, sort of. The first (and only?) argument should be allowed to be a string or an array. If it's a string, we'll call split() for you.

Thoughts?
Comment 5 Hixie (not reading bugmail) 2006-10-20 17:34:11 PDT
Both seem reasonable, yeah. There's been a lot of discussion on this on the WHATWG list.
Comment 6 Hixie (not reading bugmail) 2006-10-23 13:09:30 PDT
At the moment the WHATWG spec defines a single method that takes a single argument, which must be an array. Other arguments would cause an exception to be thrown. I recommend implementing that for now; if, during implementation and initial deployment, we find that Web authors frequently don't understand this, then we can change to some more suitable model. Please keep the WHATWG list apprised of your implementation experience. Thanks!
Comment 7 Robert Sayre 2006-10-23 13:13:10 PDT
I'll take this. It'll be fun.
Comment 8 Robert Sayre 2006-12-05 15:24:42 PST
Created attachment 247592 [details] [diff] [review]
WIP - Unpack the Array in order to call nsContentList
Comment 9 Robert Sayre 2006-12-05 15:27:39 PST
This is the first DOM method I know of that takes an array, so I had to unpack it by hand. Kind of irritating, but should also work in PyXPCOM, etc.
Comment 10 Boris Zbarsky [:bz] 2006-12-05 15:33:15 PST
fwiw, it may be worth using nsTArray<nsString> instead of nsStringArray.  Though might now matter much, and eventually we'd like to make nsStringArray just be nsTArray<nsString>...
Comment 12 Robert Sayre 2006-12-05 20:31:44 PST
Created attachment 247628 [details] [diff] [review]
WIP - Refactor nsContentList matching functions
Comment 13 Boris Zbarsky [:bz] 2006-12-05 20:43:34 PST
fwiw, that could use some OOM check and the delete calls should happen on the right pointers (cast to the actual class you implement, not some superclass), no?
Comment 14 Robert Sayre 2006-12-06 09:25:44 PST
Created attachment 247690 [details] [diff] [review]
WIP - Fix bugs bz spotted
Comment 15 Robert Sayre 2006-12-06 12:32:19 PST
Created attachment 247712 [details] [diff] [review]
WIP - works on HTML elements, added mochitest

but the methods live in the wrong place and I haven't done document.getElementsByClassName yet.
Comment 16 Robert Sayre 2006-12-06 20:38:11 PST
jst says:

"the least overhead would probably be to write a non-scriptable method that takes an nsTArray<nsAString> and write JS glue in nsDOMClassInfo.cpp to make it work with either a single string or an array of strings"
Comment 17 Boris Zbarsky [:bz] 2006-12-06 20:49:36 PST
You'd need nsTArray<nsString>, but I agree that that's the way to go.
Comment 18 Robert Sayre 2006-12-07 20:21:23 PST
Created attachment 247923 [details] [diff] [review]
WIP - helper class style
Comment 19 Robert Sayre 2006-12-07 21:19:49 PST
Created attachment 247924 [details] [diff] [review]
WIP - helper class style, with new files
Comment 20 Robert Sayre 2006-12-08 08:43:04 PST
Created attachment 247984 [details] [diff] [review]
WIP - fix build bustage in TestXMLExtras.cpp
Comment 21 Robert Sayre 2006-12-08 11:28:11 PST
Created attachment 247999 [details] [diff] [review]
WIP - works. now for cleanup.

Works on documents and elements, unit tests cover HTML, XHTML, XHTML/SVG, and XUL/XHTML/SVG.
Comment 22 Robert Sayre 2006-12-08 13:32:16 PST
Created attachment 248004 [details] [diff] [review]
getElementsByClassName
Comment 23 Robert Sayre 2006-12-08 13:42:01 PST
Created attachment 248007 [details] [diff] [review]
getElementsByClassName
Comment 24 Robert Sayre 2006-12-08 14:04:54 PST
hmm, maybe I should change this to return a node list if it gets an argument. That's what getElementsByTagName seems to do.
Comment 25 Hixie (not reading bugmail) 2006-12-08 14:07:00 PST
That's what the spec says to do...
Comment 26 Robert Sayre 2006-12-08 14:18:28 PST
(In reply to comment #25)
> That's what the spec says to do...

Actually, you told me to throw an exception for something like

> document.getElementsByClassName({})

or

> document.getElementsByClassName([[]])

but browsers don't seem to do that.


<html>
<body>
 <p id="null">WebKit finds this</p>
 <p id="undefined">Firefox and WebKit find this</p>
 <p id=",">Firefox and WebKit find this</p>
 
 <script type="text/javascript">
 document.write(document.getElementById(null) + "<br>" + 
                document.getElementById(undefined) + "<br>" + 
                document.getElementById(["",""]))
 </script>
   
</body>
</html>
Comment 27 Robert Sayre 2006-12-08 14:22:25 PST
(In reply to comment #24)
> hmm, maybe I should change this to return a node list if it gets an argument.
> That's what getElementsByTagName seems to do.
> 

ah, this should have said "if it gets a *bogus* argument"... getElementsByTagName seems to return a node list no matter what.
Comment 28 Hixie (not reading bugmail) 2006-12-08 14:25:27 PST
Oh, a bogus argument. Yeah, do whatever you think is right for that case.
Comment 29 Robert Sayre 2006-12-08 16:06:34 PST
Comment on attachment 248007 [details] [diff] [review]
getElementsByClassName

"A call to getElementsByClassName(['ccc', 'bbb']) would only return one node, however, namely p3. A call to document.getElementById('example').getElementsByClassName('ccc bbb') would return the same thing."

Oops, I missed the string tokenization requirement.
Comment 30 Anne (:annevk) 2006-12-09 03:58:12 PST
Robert, that's in one of the non normative examples. I'm pretty sure that's a bug in the spec. (Although personally I think having it accept a single string might make more sense than an array given how .className works.)
Comment 31 Robert Sayre 2006-12-11 11:33:57 PST
Created attachment 248289 [details] [diff] [review]
getElementsByClassName

OK, this one seems to be compliant, and I added unit tests for all of the examples in the WHATWG doc. However, things got messy again. I think I want to create an interface called nsIDOMDocumentOrElement (not a node). Since I already had to write a tearoff for nsGenericElement, I'm guessing I can use it in nsDocument.
Comment 32 Robert Sayre 2006-12-11 16:08:29 PST
Created attachment 248314 [details] [diff] [review]
everything in the tearoff
Comment 33 Robert Sayre 2006-12-11 18:00:09 PST
Created attachment 248333 [details] [diff] [review]
getElementsByClassName
Comment 34 Robert Sayre 2006-12-11 18:05:12 PST
Comment on attachment 248333 [details] [diff] [review]
getElementsByClassName

there's a little bit of duplication in nsIDOMClassInfo, between the two helper classes. I could probably make that go away with a new helper they would both inherit from. Not sure if that's a good idea.
Comment 35 Johnny Stenback (:jst, jst@mozilla.com) 2007-01-02 18:20:54 PST
Comment on attachment 248333 [details] [diff] [review]
getElementsByClassName

+	nsIDOMNSDocumentOrElement.idl		\

I'd much rather to see this new method be added to nsIDOMNSDocument and a new nsIDOMNSElement interface than a new combined document&element interface.

+interface nsIDOMNSDocumentOrElement : nsISupports
+{
+  // Web Applications 1.0
+  [noscript] nsIDOMNodeList GetElementsByClassNameTArray(in StringArray classes);
+  nsIDOMNodeList getElementsByClassName(in DOMString classesString);

This is something that really should be discussed in the WhatWG group, but wouldn't this be easier to implement and just as easy to use if the argument to this method was a string that lets you specify more than one class name by separating them with whitespace, rather than to go into this whole mess with arrays (which has so far ben avoided completely in the DOM APIs)?

- In nsElementSH::NewResolve():

+    JSString *str = JSVAL_TO_STRING(id);
+    JSAutoRequest ar(cx);
+    JSFunction *cfnc =
+      ::JS_DefineFunction(cx, obj, ::JS_GetStringBytes(str),
+                          GetElementsByClassName, 0, 0);

Using JS_DefineUCFunction(cx, obj, ::JS_GetStringChars(str), ::JS_GetStringLength(str), ...) here would potentially save us from needing to keep the single byte version of the function name around. I know we do this in other places in nsDOMClassInfo.cpp (due to some of the code predating JS_DefineUCFunction), but the same applies to those places as well.

- In doGetElementsByClassName():

The isDoc argument looks unused.

+  } else if (JSVAL_IS_OBJECT(argv[0]) &&
+             JS_ValueToObject(cx, argv[0], &arrObj) &&
+             JS_IsArrayObject(cx, arrObj)) {

You don't need JS_ValueToObject() here, just use the JSVAL_TO_OBJECT() macro directly.

+    jsval elemVal;

This variable is only used inside the below loop, move the declaration there.

+      for (jsuint i=0; i < arrayLen; i++) {
+        jsstr = nsnull;
+        if (JS_GetElement(cx, arrObj, i, &elemVal) &&
+            JSVAL_IS_PRIMITIVE(elemVal)) {
+          jsstr = JS_ValueToString(cx, elemVal);
+        }

Why allow non-string primitive values in the array but not objects? I'd say we either allow any type of object as long as it converts to a string that's a valid class name, or we only allow string values. If we choose to permit only string values, use the macro JSVAL_TO_STRING() here and not the more expensive JS_ValueToString() call.

+    rv = element->GetElementsByClassNameTArray(&classes,
+                                               getter_AddRefs(nodeList));

Seems like the C++-only method in this interface should take a reference to a nsTArray, not a pointer.

+  if (NS_FAILED(rv)) {
+    nsDOMClassInfo::ThrowJSException(cx, rv);
+    return JS_FALSE;
+  }
+
+  return NS_SUCCEEDED(rv);

The return JS_FALSE inside the if check is not necessary, fall through and you'll get the same result.

+JSBool JS_DLL_CALLBACK
+nsDocumentSH::GetElementsByClassName(JSContext *cx, JSObject *obj, uintN argc,
+                                    jsval *argv, jsval *rval)

This looks *identical* to nsElementSH::GetElementsByClassName() if you remove the unused isDoc argument to doGetElementsByClassName(). Maybe make this a non-class static and call the same method from both places?

- In nsContentList::~nsContentList():

+  if (mDestroyFunc) {
+    // Clean up mData
+    (*mDestroyFunc)(mData);

Maybe only call this if mData is non-null to prevent ever needing to null check the data in the destroy function implementation?

- In content/base/src/nsDocument.cpp:

+// NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIDOMNSDocumentOrElement, 
+//                               new nsDocumentOrElement(this->mRootContent));
+

Don't need this.

+nsresult
+nsDocumentOrElement::GetElementsByAtomArray(nsIContent* aContent,
+                                            nsCOMArray<nsIAtom>* aClasses,
+                                            nsIDOMNodeList** aReturn)
+{
+  if (aClasses->Count() > 0 && aContent) {
+    nsContentList* elements =
+      new nsContentList(aContent, MatchClassNames,
+                        DestroyClassNameArray, aClasses);
+    NS_ENSURE_TRUE(elements, NS_ERROR_OUT_OF_MEMORY);

This'll leak aClasses...

+    *aReturn = elements;
+  } else {
+    *aReturn = new nsBaseContentList();
+    if (!*aReturn) {
+      return NS_ERROR_OUT_OF_MEMORY;

and so will this.

+    }
+  }

And the above duplicates the null check, you could move that out here. And "elements" in the if() case above is not needed, assign directly into *aReturn there too.

+  NS_IF_ADDREF(*aReturn);

*aReturn is already null-checked, so use NS_ADDREF().

- In nsHTMLDocument.cpp:

+nsHTMLDocument::DestroyNameAttribute(void* aData)
+{
+  nsString* elementName = NS_STATIC_CAST(nsString*, aData);
+  delete elementName;
 }

There's a few of these around. Maybe stick one of these in nsContentUtils or nsContentList and let call-sites that use nsString data pass in that shared nsString destroying method?

And all the match functions (or call sites that create the nsContentLists) would now need to null-check the data argument in case we're OOM.

Other than that this looks good, but r- until we figure out the above...
Comment 36 Robert Sayre 2007-01-02 18:28:44 PST
>
> This is something that really should be discussed in the WhatWG group, but
> wouldn't this be easier to implement and just as easy to use if the argument to
> this method was a string that lets you specify more than one class name by
> separating them with whitespace, rather than to go into this whole mess with
> arrays (which has so far ben avoided completely in the DOM APIs)?

iirc, this has been discussed to death on the WhatWG list--I don't have a preference either way.
Comment 37 Robert Sayre 2007-01-16 14:45:35 PST
Created attachment 251691 [details] [diff] [review]
getElementsByClassName, string argument only
Comment 38 Robert Sayre 2007-01-16 14:56:23 PST
Created attachment 251694 [details] [diff] [review]
nits fixed

#whatwg says we can change to string argument only. Besides, we can always add the array goop later if consensus shifts.
Comment 39 Boris Zbarsky [:bz] 2007-01-16 15:19:01 PST
I assume the quirks-mode difference between this code and CSS matching is purposeful?  In quirks mode, class names are case-insensitive for matching purposes...
Comment 40 Robert Sayre 2007-01-16 15:55:13 PST
Created attachment 251701 [details] [diff] [review]
fixed flubbed null check
Comment 41 Jeff Walden [:Waldo] (remove +bmo to email) 2007-01-16 18:16:06 PST
Comment on attachment 251701 [details] [diff] [review]
fixed flubbed null check

>? dom/public/idl/core/nsIDOMNSElement.idl

You forgot to add this to the patch.


>Index: dom/public/idl/core/nsIDOMNSDocument.idl

>+  nsIDOMNodeList getElementsByClassName(in DOMString classes);

Need to bump the IID here.
Comment 42 Robert Sayre 2007-01-16 21:16:35 PST
Created attachment 251724 [details] [diff] [review]
do the idl files right

good catch, thanks
Comment 43 Hixie (not reading bugmail) 2007-01-16 23:24:07 PST
(In reply to comment #39)
> I assume the quirks-mode difference between this code and CSS 
> matching is purposeful?  In quirks mode, class names are 
> case-insensitive for matching purposes...

Quirks mode is just for legacy pages; since there are no legacy pages that use this API, no quirks are needed, IMHO.
Comment 44 Johnny Stenback (:jst, jst@mozilla.com) 2007-01-22 16:26:48 PST
Comment on attachment 251724 [details] [diff] [review]
do the idl files right

- In nsDocument::DestroyClassNameArray(void* aData):

+  if (aData) {
+    nsCOMArray<nsIAtom>* classes = NS_STATIC_CAST(nsCOMArray<nsIAtom>*, aData);
+    delete classes;

The if (aData) check isn't needed, delete is null safe.

- In content/base/src/nsDocument.h:

+  /**
+   * Utility methods for getElementsByClassName
+   */
+  static nsresult GetElementsByClassNameHelper(nsIContent* aContent,
+                                               const nsAString& aClasses,
+                                               nsIDOMNodeList** aReturn);
+   
+  // nsContentList match functions
+  static PRBool MatchClassNames(nsIContent* aContent, PRInt32 aNamespaceID,
+                                nsIAtom* aAtom, void* aData);
+  static void DestroyClassNameArray(void* aData);

Since the above are used by both nsDocument and nsGenericElement one could argue that the above belongs in nsContentUtils rather than in nsDocument. Thoughts?

And it looks like the MatchClassNames() and DestroyClassNameArray() functions could be non-class statics, or at least protected static methods as they're only ever used internally by GetElementsByClassNameHelper().

+NS_INTERFACE_MAP_BEGIN(nsNSElementTearoff)
+ NS_INTERFACE_MAP_ENTRY(nsIDOMNSElement)

Add a space there to match surrounding indentation.

r=jst with that.
Comment 45 Robert Sayre 2007-01-22 16:53:29 PST
(In reply to comment #44)
> 
> Since the above are used by both nsDocument and nsGenericElement one could
> argue that the above belongs in nsContentUtils rather than in nsDocument.
> Thoughts?

I can't think of a reason they would be useful elsewhere, though. I followed the pattern used in nsXULElement and nsXULDocument, which share match functions that live in nsXULDocument. Either location is OK with me.

Comment 46 Robert Sayre 2007-01-22 20:53:30 PST
Created attachment 252419 [details] [diff] [review]
address jst's comments
Comment 47 Boris Zbarsky [:bz] 2007-01-25 21:57:39 PST
It might take me a week or two to get to this.  :(
Comment 48 Peter Van der Beken [:peterv] 2007-01-31 16:16:50 PST
Comment on attachment 252419 [details] [diff] [review]
address jst's comments

>Index: testing/mochitest/tests/test_bug357450.js
>===================================================================

>+  var nodes = root.getElementsByClassName("f");

>+  is(nodes.length, 0, "empty string should get an empty nodelist");

Didn't you just pass in "f" instead of the empty string?

>+  nodes = root.getElementsByClassName([1]);

So we still support arrays?

>+  nodes = root.getElementsByClassName({});

>+  is(nodes.length, 0, "bogus arg should get an empty nodelist");

I'm guessing we don't do that if the object passed in provides a toString?

>Index: testing/mochitest/tests/test_bug357450.xul
>===================================================================

>\ No newline at end of file

Fix this.

>Index: testing/mochitest/tests/test_bug357450_svg.xhtml
>===================================================================

>\ No newline at end of file

>Index: content/base/src/nsDocument.cpp
>===================================================================

>+nsDocument::MatchClassNames(nsIContent* aContent,

>+  PRInt32 length = classes->Count();
>+  for (PRInt32 i=0; i < length; i++) {

Nits: declare i outside of the loop, spaces around = and ++i.

>Index: content/base/src/nsGenericElement.h
>===================================================================

>+  virtual ~nsNSElementTearoff() {};

Do we even need this?

>Index: content/html/document/src/nsHTMLDocument.cpp
>===================================================================

> nsHTMLDocument::MatchNameAttribute(nsIContent* aContent, PRInt32 aNamespaceID,

>+  nsAString* elementName = NS_STATIC_CAST(nsAString*, aData);

I'd cast to nsString*.

>Index: content/xul/document/src/nsXULDocument.cpp
>===================================================================

> nsXULDocument::MatchAttribute(nsIContent* aContent,

>+    nsAString* attrValue = NS_STATIC_CAST(nsAString*, aData);

Ditto.

>Index: dom/public/idl/core/nsIDOMNSElement.idl
>===================================================================

>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2000
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Vidur Apparao <vidur@netscape.com> (original author)
>+ *   Johnny Stenback <jst@netscape.com>

Heh, haven't seen Vidur in a while. ;-)

>+  nsIDOMNodeList getElementsByClassName(in DOMString classes);

Want to add a comment describing this, maybe with a link to the spec?

Looks great, thanks.
Comment 49 Robert Sayre 2007-02-01 06:28:46 PST
(In reply to comment #48)
> 
> >+  nodes = root.getElementsByClassName([1]);
> 
> So we still support arrays?

No, we coerce that to the string "1". see comment #26.
Comment 50 Robert Sayre 2007-02-01 07:07:44 PST
Created attachment 253608 [details] [diff] [review]
patch to check in
Comment 51 Robert Sayre 2007-02-01 08:37:23 PST
Checking in dom/public/idl/core/Makefile.in;
/cvsroot/mozilla/dom/public/idl/core/Makefile.in,v  <--  Makefile.in
new revision: 1.13; previous revision: 1.12
done
Checking in dom/public/idl/core/nsIDOMNSDocument.idl;
/cvsroot/mozilla/dom/public/idl/core/nsIDOMNSDocument.idl,v  <--  nsIDOMNSDocument.idl
new revision: 1.14; previous revision: 1.13
done
RCS file: /cvsroot/mozilla/dom/public/idl/core/nsIDOMNSElement.idl,v
done
Checking in dom/public/idl/core/nsIDOMNSElement.idl;
/cvsroot/mozilla/dom/public/idl/core/nsIDOMNSElement.idl,v  <--  nsIDOMNSElement.idl
initial revision: 1.1
done
Checking in dom/src/base/nsDOMClassInfo.cpp;
/cvsroot/mozilla/dom/src/base/nsDOMClassInfo.cpp,v  <--  nsDOMClassInfo.cpp
new revision: 1.425; previous revision: 1.424
done
Checking in content/base/public/nsContentUtils.h;
/cvsroot/mozilla/content/base/public/nsContentUtils.h,v  <--  nsContentUtils.h
new revision: 1.117; previous revision: 1.116
done
Checking in content/base/src/nsContentList.cpp;
/cvsroot/mozilla/content/base/src/nsContentList.cpp,v  <--  nsContentList.cpp
new revision: 3.102; previous revision: 3.101
done
Checking in content/base/src/nsContentList.h;
/cvsroot/mozilla/content/base/src/nsContentList.h,v  <--  nsContentList.h
new revision: 3.68; previous revision: 3.67
done
Checking in content/base/src/nsDocument.cpp;
/cvsroot/mozilla/content/base/src/nsDocument.cpp,v  <--  nsDocument.cpp
new revision: 3.703; previous revision: 3.702
done
Checking in content/base/src/nsDocument.h;
/cvsroot/mozilla/content/base/src/nsDocument.h,v  <--  nsDocument.h
new revision: 3.325; previous revision: 3.324
done
Checking in content/base/src/nsGenericElement.cpp;
/cvsroot/mozilla/content/base/src/nsGenericElement.cpp,v  <--  nsGenericElement.cpp
new revision: 3.529; previous revision: 3.528
done
Checking in content/base/src/nsGenericElement.h;
/cvsroot/mozilla/content/base/src/nsGenericElement.h,v  <--  nsGenericElement.h
new revision: 3.241; previous revision: 3.240
done
Checking in content/html/content/src/nsHTMLTableRowElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLTableRowElement.cpp,v  <--  nsHTMLTableRowElement.cpp
new revision: 1.112; previous revision: 1.111
done
Checking in content/html/document/src/nsHTMLDocument.cpp;
/cvsroot/mozilla/content/html/document/src/nsHTMLDocument.cpp,v  <--  nsHTMLDocument.cpp
new revision: 3.710; previous revision: 3.709
done
Checking in content/html/document/src/nsHTMLDocument.h;
/cvsroot/mozilla/content/html/document/src/nsHTMLDocument.h,v  <--  nsHTMLDocument.h
new revision: 3.204; previous revision: 3.203
done
Checking in content/xul/content/src/nsXULElement.cpp;
/cvsroot/mozilla/content/xul/content/src/nsXULElement.cpp,v  <--  nsXULElement.cpp
new revision: 1.682; previous revision: 1.681
done
Checking in content/xul/document/src/nsXULDocument.cpp;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v  <--  nsXULDocument.cpp
new revision: 1.747; previous revision: 1.746
done
Checking in content/xul/document/src/nsXULDocument.h;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.h,v  <--  nsXULDocument.h
new revision: 1.191; previous revision: 1.190
done
Comment 52 Anne (:annevk) 2007-09-08 05:53:55 PDT
Hi, you guys might want to have a look at the testcases that fail here: http://tc.labs.opera.com/apis/getElementsByClassName/
Comment 53 Robert Sayre 2007-09-08 15:16:57 PDT
(In reply to comment #52)
> Hi, you guys might want to have a look at the testcases that fail here:
> http://tc.labs.opera.com/apis/getElementsByClassName/
> 

You might, too. ;)

See comment 43 for details on "quirks mode" behavior.
Comment 54 Robert Sayre 2007-09-08 15:48:07 PDT
Most of these failures look like our nodelist code missing the class attribute on the <html> element. The tokenization of the parameter and dynamic updates seem fine.
Comment 55 Anne (:annevk) 2007-09-08 16:53:07 PDT
I think the quirks mode test fails because of the <html> element problem. One other thing that seems go wrong is not recognizing \f as space character (007 and 008). HTML5 currently defines that as space character and I've assumed that this API will simply reuse that definition.
Comment 56 Boris Zbarsky [:bz] 2007-09-12 09:27:13 PDT
> Most of these failures look like our nodelist code missing the class attribute
> on the <html> element.

It's not missing anything. It's being told to ignore the root by the caller.  That is, nsDocument::GetElementsByClassName is just buggy here.
Comment 57 Boris Zbarsky [:bz] 2007-09-12 09:54:18 PDT
Filed bug 395915.

The whitespace issue should get a bug once that part of the API is stable, I guess.
Comment 58 Boris Zbarsky [:bz] 2007-09-12 12:20:58 PDT
Note that I'm not sure the 011.xml test is correct.  Since there is no <x> element defined by SVG, I'm not sure the "class" attribute applies to it.  Someone should check with the SVG working group, and I would suggest using valid markup for the test, unless you mean to be testing SVG error handling (which is not really defined on the DOM level, so I'm not sure what you'd be testing).
Comment 59 Anne (:annevk) 2007-09-12 13:11:00 PDT
I have assumed that each element in the SVG namespace supports the global attributes much like each element in the HTML namespace does. I suppose this might not be what the SVG draft says, but it seems the right thing to do...
Comment 60 Boris Zbarsky [:bz] 2007-09-12 13:55:16 PDT
It's not clear that it is the right thing, per SVG spec.  As a simple example, per the spec schema the "class" attribute is only allowed on some SVG elements (the ones implementing SVGStylableElement) and not others.  We're actually violating that for existing SVG elements in the interests of having this API work better on valid SVG documents.  But for invalid ones, I don't think we really care to make it work.
Comment 61 chris hofmann 2008-03-12 18:46:06 PDT
can folks that worked on this comment on the possible side effect detected in bug 421015

-thanks, chofmann
Comment 62 Volkmar Kostka 2008-04-10 15:15:48 PDT
Speaking of side effects this one is also a possible candidate: bug #428293

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