Re: Add getElementsByClassName support

[j...@google.com]

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).

http://codereview.appspot.com/44041

[Ray Cromwell]

On Tue, May 12, 2009 at 1:26 PM, <j...@google.com> wrote:
>  - 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.

The problem is, firefox3 has this method, but firefox2 needs to use
document.evaluate, so you either have to add a deferred binding just
for this, or you do a runtime test and cache the result, something
like

public native void initGetElementsByClassName() /*-{
this.getElementsByClassName = nativeTest ?
this.@com.google.gwt.user.client.DOMImpl::getElementsByClassNameNative(Ljava/lang/String;)
: this.@com.google.gwt.user.client.DOMImpl::getElementsByClassNameXPath(Ljava/lang/String;);
}-*/;

> - 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.

Yes, you can use querySelectorAll for this, although short-circuiting
to getElementsByClassName() if it's native is faster on some browsers.
Again, the IE6 vs IE8 issue will crop up requiring either another
deferred binding, or a runtime-test.

There will be some overlap between this and the Gwt Query based
selector stuff too.

-Ray

[Joel Webber]

On Tue, May 12, 2009 at 4:38 PM, Ray Cromwell <cromw...@gmail.com> wrote:

On Tue, May 12, 2009 at 1:26 PM,  <j...@google.com> wrote:
>  - 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.

 The problem is, firefox3 has this method, but firefox2 needs to use
document.evaluate, so you either have to add a deferred binding just
for this, or you do a runtime test and cache the result, something
like

public native void initGetElementsByClassName() /*-{
  this.getElementsByClassName = nativeTest ?
this.@com.google.gwt.user.client.DOMImpl::getElementsByClassNameNative(Ljava/lang/String;)
: this.@com.google.gwt.user.client.DOMImpl::getElementsByClassNameXPath(Ljava/lang/String;);
}-*/;

I see. That makes sense. In this case, though, it would be nice to move that logic into the DOMImplMozilla so that it doesn't clutter up the standard case.

> - 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.

Yes, you can use querySelectorAll for this, although short-circuiting
to getElementsByClassName() if it's native is faster on some browsers.
 Again, the IE6 vs IE8 issue will crop up requiring either another
deferred binding, or a runtime-test.

There will be some overlap between this and the Gwt Query based
selector stuff too.

Good point. If you guys could coordinate to make sure there's at least a chance of GwtQuery using this interface (to the extent it needs getElementsByClassName() at all), that would be great.

-Ray

[Vitali Lovich]

On Tue, May 12, 2009 at 4:49 PM, Joel Webber <j...@google.com> wrote:

On Tue, May 12, 2009 at 4:38 PM, Ray Cromwell <cromw...@gmail.com> wrote:

On Tue, May 12, 2009 at 1:26 PM,  <j...@google.com> wrote:
>  - 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.

 The problem is, firefox3 has this method, but firefox2 needs to use
document.evaluate, so you either have to add a deferred binding just
for this, or you do a runtime test and cache the result, something
like

public native void initGetElementsByClassName() /*-{
  this.getElementsByClassName = nativeTest ?
this.@com.google.gwt.user.client.DOMImpl::getElementsByClassNameNative(Ljava/lang/String;)
: this.@com.google.gwt.user.client.DOMImpl::getElementsByClassNameXPath(Ljava/lang/String;);
}-*/;

I see. That makes sense. In this case, though, it would be nice to move that logic into the DOMImplMozilla so that it doesn't clutter up the standard case.

> - 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.

I haven't looked at IE8 yet.  It's fine to use querySelectorAll to implement getElementsByClassName as a faster way for IE8.  Eventually, querySelectorAll needs to make it into the API by itself, although that will require more effort probably since the Javascript emulation of that one is probably far more complicated.

Yes, you can use querySelectorAll for this, although short-circuiting
to getElementsByClassName() if it's native is faster on some browsers.
 Again, the IE6 vs IE8 issue will crop up requiring either another
deferred binding, or a runtime-test.

querySelectorAll would only make sense with IE8 no?  I think it's the only browser that didn't implement both at the same time (or in any case, getElementsByClassName probably came first).

[Vitali Lovich]

On Tue, May 12, 2009 at 4:26 PM, <j...@google.com> wrote:

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.

This was so that the overhead was as low as possible.
  1) Situation 1 - Program doesn't use getElementsByClassName.  This whole code path will be stripped out by the compiler (not a reason for the inner class, just my thought process when I was writing this).
  2) Situation 2 - Program does use getElementsByClassName.  2 ways to implement.  a) initialize the prototypes ahead of time with the appropriate implementation.  b) determine the appropriate function dynamically on every invocation.

The inner class was the only way I could think of to satisfy 2a while supporting 1 so that there's some minimal startup code to pick the right implementation, but the invocation is as fast as can possibly be.

Optionally, I could have made the startup code common to everyone, but then that just adds overhead for people who will never use this function.  Or I could have done something like

if (e.getElementsByClassName) {
   return e.getElementsByClassName(class name);
}
Element.prototype.getElementsByClassName = function() { //whatever };
document.getElementsByClassName = function() { // whatever };

 - 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

Considering the native versions added getElementsByClassName to the document object, I don't see a potential conflict.  All the libraries I looked at were fixed a while back too because they clobbered the native version with the emulated code.  This code also checks for document.getElementsByClassName first.  That means if a user wants to use the JS libraries version of it (I wouldn't know why since the GWT version would be faster & smaller), he would just have to make sure that it gets registered first (the implementation I wrote doesn't clobber the existing getElementsByClassName).

     (remember you can add static fields to the DOMImpl classes if you
need to).

what is this in reference 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.

As Ray pointed out, FF2 breaks this for us.  Just to expand, so does every standards compliant browser at some point (a while back I was asking about supported browser versions for particularly this reason - all the versions had, as far as I could tell, non-XPath, followed by XPath, followed by native getElementsByClassName, hence the reason I made it standard.

- 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.

Yes - I should have written this down to begin with.  I'll have to look it up again.
From what I can remember I believe it went like this:
*< IE8 => DOM
IE8 => XPath through querySelectorAll (implementation still needs to be written)
< FF2 => DOM
< FF3 => XPath through document.evaluate
*>= FF3 => native
Safari 3 => XPath
*>= Safari 3.1 => native
Safari 2 => not clear about this - might be XPath, might be DOM.  Can't find clear documentation on this & don't have a Mac.

Opera => todo.  They don't seem to have a good changelog (at least that I can find) of when features like this were added.

But don't hold me to any of this - it's all from memory (except for those marked *).

- 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.

Yup.  Might pose a problem with quirks mode, but no probably no more so than any of the other XPath methods.

- 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 :)

Yes that would be nice.  I've been on vacation for a while - this is on my todo list.

 - If you need a hand running tests across browsers, send me patches
and I'll be glad to do it here.

Sweet :D.  I think first we need to come up with a matrix of browsers versions.  My list:
IE6, IE8 (IE7 optional - should be functionally equivalent to IE6)
Firefox 1.5, Firefox 2, Firefox 3
Safari 2, Safari 3, Safari 4 (Safari 3.1 is optional, but should be functionally equivalent to 4)
Opera 8.5 (first free version & about the same age as FF 1.5), Opera 9.0, Opera 9.5
Bonus: Chrome, Konqueror

Is that a reasonable approximation?  Too much?  Too little?  Wrong versions?

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).

Response forthcoming via appspot.

http://codereview.appspot.com/44041

[vlo...@gmail.com]

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

http://codereview.appspot.com/44041

[cromw...@gmail.com]

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.

http://codereview.appspot.com/44041

[vlo...@gmail.com]

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.

http://codereview.appspot.com/44041