Add getElementsByClassName support

Enhancement request to track progress of adding getElementsByClassName
support to Document & Elements.

Ideally, it would follow HTML5 semantics as closely as possible, although
this may be difficult for browsers that need use the XPath implementation
for performance.

There is also potentially a difference in behaviour if the lowest-overhead
implementation:
Safari XPath & browsers that implement getElementsByClassName natively will
return a live NodeList.
All other browsers will most likely have to return an array cast as a NodeList.

[cromwellian, 2009-04-21 02:26:30]

http://codereview.appspot.com/44041/diff/1/2
File user/src/com/google/gwt/dom/client/DOMImpl.java (right):

http://codereview.appspot.com/44041/diff/1/2#newcode194
Line 194: for (j = regexps.length - 1; j >= 0 && regexps[j].test(e); j--) {}
Do you really mean test(e) or test(e.className) ?

http://codereview.appspot.com/44041/diff/1/2#newcode198
Line 198: return result;
IIRC, getElementsByClassName() is supposed to return unique, non-repeated
elements.

[vlovich, 2009-04-21 04:38:21]

In the last response I meant to say getElementsByTagName returns a unique set,
not getElementsByClassName (otherwise the sentance reads like it's from the
redundant desk in the office of redundant redundancies).

http://codereview.appspot.com/44041/diff/1/2
File user/src/com/google/gwt/dom/client/DOMImpl.java (right):

http://codereview.appspot.com/44041/diff/1/2#newcode184
Line 184: var i, j, e, c, classes = className.split(" "), elements = parent.all
|| parent.getElementsByTagName("*"), regexps = [], result = [];
Just noticed that it should go the other way I think
parent.geltElementsByTagName("*") || parent.all.  It's not really an issue since
this code will generally only run on IE & the other browsers I believe implement
.all anyways.  IE since 5.5 (at least 6) supports getElementsByTagName.

http://codereview.appspot.com/44041/diff/1/2#newcode194
Line 194: for (j = regexps.length - 1; j >= 0 && regexps[j].test(e); j--) {}
On 2009/04/21 02:26:30, cromwellian wrote:
> Do you really mean test(e) or test(e.className) ?
> 

Sorry - yes.  e.className.  I knew I missed it somewhere :D.  Actually, I think
that it should be

ec = e.className;
test(ec)

Should be faster if there are multiple classes.

http://codereview.appspot.com/44041/diff/1/2#newcode198
Line 198: return result;
On 2009/04/21 02:26:30, cromwellian wrote:
> IIRC, getElementsByClassName() is supposed to return unique, non-repeated
> elements.

Doesn't this? Am I missing something?  getElementsByClassName returns a set of
elements as well doesn't it?  A subset of a set should still be a set.

[vlovich, 2009-04-21 05:11:14]

http://codereview.appspot.com/44041/diff/1/4
File user/src/com/google/gwt/dom/client/DOMImplStandard.java (right):

http://codereview.appspot.com/44041/diff/1/4#newcode129
Line 129: var r, i, x = document.evaluate(xPath, parent, null, 0, null);
There's a bug here I think - r needs to be initialized with [].  Otherwise, push
is undefined when it is accessed later.

[jgw, 2009-05-12 20:26:56]

Thanks for digging into this -- it'll be helpful for a lot of developers.
And sorry I've taken so long to get around to reviewing -- I've been pretty
swamped lately.

I'm going to ask Ray to come back and review some of the DOM details a bit more,
but I'd like to address a couple of broader structural questions myself.

- It's not entirely clear to me why DOMImplStandard.ElementsByClassName is its
own inner class.
  - I also don't think it should be adding methods to the "document" object.
    - It runs the risk of conflicting with other js libs, and there's almost
always a better way to hang onto those values
      (remember you can add static fields to the DOMImpl classes if you need
to).
  - This class also appears to be doing runtime detection of
(document.evaluate). I think we should be able to make this
    solely dependent upon the browser overload and ditch the runtime switch.

- It would be nice to have a brief doc (even if it's just on the contrib list),
detailing which approaches work on which browsers.

- Don't forget IE8 support (I slipped it into trunk as a new user-agent value
recently). They added support for querySelectorAll(), which is nice.

- This could really use some tests. There is a lot of complex behavior here, and
it would be nice to have some confirmation that it works :)
  - If you need a hand running tests across browsers, send me patches and I'll
be glad to do it here.

http://codereview.appspot.com/44041/diff/1/5
File user/src/com/google/gwt/dom/client/Document.java (right):

http://codereview.appspot.com/44041/diff/1/5#newcode1148
Line 1148: return DOMImpl.impl.getElementsByClassName(this.<Element>cast(),
className);
The Document is actually *not* an Element (one of those weird DOM things...).
Either DOMImpl.getElementsByClassName() needs two overloads, or it should be
loosened up to accept Node (I probably favor the former, but it depends upon the
implementation details within that method).

[vlovich, 2009-05-12 21:26:11]

http://codereview.appspot.com/44041/diff/1/5
File user/src/com/google/gwt/dom/client/Document.java (right):

http://codereview.appspot.com/44041/diff/1/5#newcode1148
Line 1148: return DOMImpl.impl.getElementsByClassName(this.<Element>cast(),
className);
On 2009/05/12 20:26:57, jgw wrote:
> The Document is actually *not* an Element (one of those weird DOM things...).
> Either DOMImpl.getElementsByClassName() needs two overloads, or it should be
> loosened up to accept Node (I probably favor the former, but it depends upon
the
> implementation details within that method).
I was just writing as it would be done in Javascript.  Sure you could write two
seperate methods, but at some point, unless you just want to duplicate the JSNI
for both, you have to cast one to the other.  I know they're not the same.  Now
that you mention it though, could this cause a problem in hosted mode?  I
haven't had time to test any of this.  I dislike Node because Node doesn't
actually expose the method

[cromwellian, 2009-05-14 18:50:42]

Per Joel's request.

http://codereview.appspot.com/44041/diff/1/3
File user/src/com/google/gwt/dom/client/DOMImplSafari.java (right):

http://codereview.appspot.com/44041/diff/1/3#newcode173
Line 173: public native NodeList<Element> getElementsByXPath(Element parent,
String xPath) /*-{
Where is this _getElementsByXPath function? Is this code copied from
prototype.js? Also, if this is a function you've inserted on document, you
should probably use $doc

http://codereview.appspot.com/44041/diff/1/4
File user/src/com/google/gwt/dom/client/DOMImplStandard.java (right):

http://codereview.appspot.com/44041/diff/1/4#newcode159
Line 159: document.getElementsByClassName =
Element.prototype.getElementsByClassName = xpathImpl;
This code by default will run in an IFRAME that is different than the host HTML
page, so I'm not sure this is doing what you want, since you've overriding
methods on the document object and Element.prototype of the IFRAME which loaded
the GWT code, not the one that has the elements you want to search.  $doc refers
to the document object of the host page.

http://codereview.appspot.com/44041/diff/1/4#newcode169
Line 169: if (!document.getElementsByClassName) {
The checks on document here are ok because another module or included JS library
might stomp on $doc.getElementsByClassName, so checking the private one in the
IFRAME is better. However, if you truly want to check if a native implementation
exists, you can't just test for the existence of the function.

[vlovich, 2009-05-14 20:38:11]

I'm going to start working on version 2 trying to incorporate all the feedback
so far.

http://codereview.appspot.com/44041/diff/1/3
File user/src/com/google/gwt/dom/client/DOMImplSafari.java (right):

http://codereview.appspot.com/44041/diff/1/3#newcode173
Line 173: public native NodeList<Element> getElementsByXPath(Element parent,
String xPath) /*-{
On 2009/05/14 18:50:42, cromwellian wrote:
> Where is this _getElementsByXPath function? Is this code copied from
> prototype.js? Also, if this is a function you've inserted on document, you
> should probably use $doc

Sorry - my bad.  Didn't read the code from the
http://webkit.org/blog/153/webkit-gets-native-getelementsbyclassname/ carefully
enough.  This function can be removed - the one provided in DOMImplStandard is
the one that should be used.

http://codereview.appspot.com/44041/diff/1/4
File user/src/com/google/gwt/dom/client/DOMImplStandard.java (right):

http://codereview.appspot.com/44041/diff/1/4#newcode159
Line 159: document.getElementsByClassName =
Element.prototype.getElementsByClassName = xpathImpl;
On 2009/05/14 18:50:42, cromwellian wrote:
> This code by default will run in an IFRAME that is different than the host
HTML
> page, so I'm not sure this is doing what you want, since you've overriding
> methods on the document object and Element.prototype of the IFRAME which
loaded
> the GWT code, not the one that has the elements you want to search.  $doc
refers
> to the document object of the host page.
> 

You are correct.  I should set them for the parent page, but please look at the
comment below for my proposed correction (this portion of code remains
unchanged).

http://codereview.appspot.com/44041/diff/1/4#newcode169
Line 169: if (!document.getElementsByClassName) {
On 2009/05/14 18:50:42, cromwellian wrote:
> The checks on document here are ok because another module or included JS
library
> might stomp on $doc.getElementsByClassName, so checking the private one in the
> IFRAME is better. However, if you truly want to check if a native
implementation
> exists, you can't just test for the existence of the function.

Actually, AFAIK, that's exactly the test to use to determine if the native
implementation exists (or in the more general case, if any implementation,
native or otherwise, of a JS function exists).

As for the stomping, I agree, but please comment on by reasoning in the added
snippet below.

http://codereview.appspot.com/44041/diff/1/4#newcode175
Line 175: }
I think I'm missing this code here to make it work:
  if (!$doc.getElementsByClassName) {
    $doc.getElementsByClassName = document.getElementsByClassName;
  }

  if (!$wnd.Element.prototype.getElementsByClassName) {
    $wnd.Element.prototype.getElementsByClassName =
Element.prototype.getElementsByClassName;
  }

In a little non-GWT snippet I wrote,
window.parent.Element.prototype.getElementsByClassName seemed to work from
within the iFrame on FF 3.5 in Linux.  Would this be an appropriate generic
solution?

Also, to detect stomping, I think, if the functions are already defined in $doc
but not document, we should give the developer some kind of warning message that
there might be a conflict with one of the libraries he/she uses.  What do you
think?  A call to GWT.log should probably suffice.