49017 - RFE: EventListener objects not accepted by addEventListener

Status: VERIFIED FIXED

(Core :: DOM: Events, enhancement, P3)

Description

[Joe Hewitt]

The DOM Level 2 spec intended for event listeners to be objects, not static
functions.  This is made clear by the definition of the EventListener interface
whose only method is handleEvent(in Event evt).

Currently, only static functions are accepted as listeners. For me, this is not
practical, as my listeners are often pertinent to a specific object instance and
this object needs to be notified directly when the event fires.  Having only
static functions as listeners forces me to resort to ugly trickery to make sure
the right object is notified.

This is not something I consider an enhancement.  It's a crucial part of the DOM
Level 2 events architecture and needs to be implemented.  Imagine this syntax:

object.addEventListener("click", { related: this, handleEvent: function(aEvent)
{ alert(this.related + ' is listening.' } }, false);

Comment 1

[joki (gone)]

The DOM level 2 spec is based on interfaces.  Since ECMAScript does not have 
interfaces the ECMAScript language binding in the DOM level 2 spec states:

Object EventListener - This is an ECMAScript function reference. This method 
returns a void. The parameter is of type Event. 

This is how it is implemented in mozilla.

Comment 2

[Joe Hewitt]

Since ECMAScript doesn't allow defining of interfaces, it would be super 
convenient if we could instantiate the DOM EventListener interface via 
XPConnect or something and override the handleEvent function... the bottom line 
is that functions as listeners don't allow contextual information to be 
referenced which distinguishes that listener from other listeners.  Event if I 
add properties to the function object they can't be determined from inside the 
function when it is called.

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

joki's assessment that this bug is invalid is correct. However, I changed the 
bug to say this is an enhancement request. I will also Future it, meaning we 
will not tackle this now but will reconsider sometime after NS 6.0.

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration.

Comment 4

[Joe Hewitt (gone)]

I think this should get priority in the near future.  Anyone writing
object-oriented JS _needs_ this.  Case in point:  I add an event listener to the
document object for the "mousedown" event.  I want a specific object to receive
notification of this specific mousedown event.  How is that currently possible?
It's not, without ugly hacks.  Static function listeners just won't do.

It should be possible to add an object as a listener when that object has a
HandleEvent method.  HandleEvent should be invoked as a method so that "this"
points back to the object.  XPConnect-based events support this functionality --
why not DOM events too? 

Comment 5

[Johnny Stenback (:jst)]

If this is still needed/wanted I think this could be fixed with not too much
XPConnect hacking, right now when the event listener calls an event handler it
does that by calling HandleEvent() on an nsIDOMEventListener pointer, and when
XPConnect sees this call (if the event listener is a JS function/object) it
checks if the JSOjbect (i.e. the event handler) is a function (since
nsIDOMEventListener is a special interface that's marked as a [function]
interface) and if it is XPConnect calls that function. If the event handler
JSObject is not a function XPConnect could check if the JSObject has a
handleEvent() function on it and call that, doesn't sound to complicated. Cc:ing
dbradley and jband for their comments.

Comment 6

[Joe Hewitt (gone)]

Yes, this is still very much wanted by me! ;)

Comment 7

[John Bandhauer]

I don't like things that require special hardcoded hacks to xpconnect. Why is it 
that you can't use closures in your JS code? You can produce a JS function that 
has any instance data of its own that you want.

Comment 8

[John Bandhauer]

function F(id) {this.id = id}

F.prototype =  {
    getEventHandler : function() {
        var self = this;
        return function(aVar) {return self.handleEvent(aVar);};
    },

    handleEvent : function (aVar) {
        dump("handleEvent called on object with id = "+this.id+
             " and with aVar = "+aVar+"\n");    
    }
};

var f = new F("aID");

var handler = f.getEventHandler();

handler("aString");

Comment 9

[John Bandhauer]

And if you don't want to build the closure creation function into each JS class 
then a simple one line function can be used to do the same generic delegation 
that jst is suggesting - but in your JS code, not hacked into xpconnect...

function buildEventDispatcher(obj) {
    return function(aVar) {return obj.handleEvent(aVar);};
}

function F(id) {this.id = id}
F.prototype =  {
    handleEvent : function (aVar) {
        dump("handleEvent called on object with id = "+this.id+
             " and with aVar = "+aVar+"\n");    
    }
};

var f = new F("aID");

var handler = buildEventDispatcher(f);

handler("aString");

Comment 10

[John Bandhauer]

One more comment...

The scheme jst suggested has the flaw (that I forgot about above) that when we 
call JS event handlers we do so with the 'this' set to the object on who's 
behalf the event is being sent - a form element or whatever. So, the call really 
has two objects that are communicated: the 'this' of the element and the event 
itself. If we do what he suggests we lose that 'this'.

The code below shows how you could do a one line closure to arrange for your 
(custom) event handling methods to receive all three references: the element 
'this, the event, and your handler object's 'this'...


// This event dispatcher closure will send the this of the object sending 
// the event as the first param and the event object as the second param.
function buildEventDispather(obj) {
    return function(aEvent) {return obj.handleEvent(this, aEvent);};
}

function F(id) {this.id = id}

F.prototype =  {
    handleEvent : function (aSource, aEvent) {
        dump("handleEvent called on object with id = "+this.id+
             " and with aSource.id = "+aSource.id+
             " and with aEvent = "+aEvent+"\n");    
    }
};

var f = new F("aID");

var handler = buildEventDispather(f);

// To demonstrate....
// Simulate the way that the 'this' is set in nornal event handler calls
// by making the handler into a member and calling it as a method...

var eventSource = {id : "eventSourceId", handleEvent : handler }

eventSource.handleEvent("aString");

/////////////////////////////////////////////////////////////////////////////

I think the critical point that hewitt missed in filing this bug is that 
JavaScript != C++ in that in JS there are not *only* static functions and object 
methods. You can attach any info you want to a function object - using closures 
as above, or simply by adding named properties to the function object.

I think it would be a big mistake to build in more (non-standard) mappings for 
sending DOM events into JS for mozilla. So, I'm setting this bug back to 
INVALID. Reopen it if you want to further argue your position :)

Comment 11

[Johnny Stenback (:jst)]

IMO if someone creates an object in JS and chooses to implement the
nsIDOMEventListener interface in JS and register that as an event listener, then
it's pretty obvious that it's going to work differently than if they do go with
the callback function route.

IMO XPConnect wouldn't need to (and couldn't?) do the "convert 'this'" dance at
all if the event handler is an object that has the handleEvent() function on it,
the 'this' would simply be the object that was registerd as the event handler
and XPConnect would just call the handleEvent() function on it. It's then up to
the event handler to know how to get at the information it needs (since it can't
rely on 'this' being the event target), this lines up exactly whth how event
handlers work in C++. I don't see how this would be any more hardcoding in
XPConnect, it would simply be to fall through into the normal path for calling a
function throught an interface implemented by a JSObject if the object (that's
registerd in this case as an event handler) is not a function object. IMO
there's nothing special about nsIDOMEventListener in the case where the event
listener is *not* a function.

Reopening, I don't think this needs to be high priority, but I do think this
makes sense.

Comment 12

[John Bandhauer]

jst: What if the JSObject being called is a function *and* has a function 
property with the named methods? Then the named methods should 'win' no? Do you 
want xpconnect to pay the cost of checking for the named property every time a 
[function] interface is thrown? Or, do you just want to call that a pathological 
case and forget about it?

[sorry, I forgot that 'handleEvent' was actually the declared name in the 
interface - I stupidly had it in my mind that you were talking about hardcoding 
that into xpconnect. Doh!]

Comment 13

[John Bandhauer]

Attachment: patch that I think will do what jst proposes

Comment 14

[John Bandhauer]

I'm luke warm about this suggestion. But, here's a patch that I think (without 
testing!) will do what Johnny says he wants. The xpidl [function] scheme is 
generic, but really only there for this use (and for the currently broken 
OnError handler support!). You guys can decide among yourselves if this is 
really the Right Thing to do. I'd be swayed if *other* JS mappings of the DOM 
did the same thing. Divergence from the defacto standard seems generally bad to 
me - especially when a reasonable alternative (using closures) is availabe. 
Whatever. Y'all should do some testing too if you want to land this patch :)

Comment 15

[Johnny Stenback (:jst)]

Jband, your patch does indeed make this work, and so does the patch I hacked up
that did more or less the same, in a more hacky way :-)

I'm in favor of supporting this, it makes alot of sense to me, and it doesn't
break anything, except for some pathological cases.

Brendan, do you have feelings either way about this?

Comment 16

[Brendan Eich [:brendan]]

jband: why call JS_ValueToFunction, which might convert and return an entirely
different JSFunction*, instead of testing whether JS_TypeOfValue(cx, v) ==
JSTYPE_FUNCTION, which indicates a callable object?  Or would typeof do the
wrong thing with an interface wrapper?

Other than that, this patch seems ok to me.  I had forgotten about the xpidl
[function] attribute.  Is it documented anywhere?  A quick grep in my
mozilla.org website content tree didn't find anything.

/be

Comment 17

[John Bandhauer]

brendan: I was trying to cover all the bases. When I later try to call the 
objects won't the engine try to do whatever it can to convert the object to a 
function before giving up? I don't mind doing the simpler (non-failing!) test. 
Should I just rely on that?

[function] is in the newer typelib spec: 
http://mozilla.org/scriptable/typelib_file.html#is_function
I didn't even look into hacking mang's xpidl reference yet.

I appreciate the commentary on the code correctness, but the main issue (I 
thought) was one of opinion: should we add this named method JS mapping of the 
event sink interfaces?

Comment 18

[John Bandhauer]

Attachment: The simplification of the previous patch using JS_TypeOfValue

Comment 19

[Brendan Eich [:brendan]]

My point was not code nit-picking: there is a difference between a callable
object and a function (conversion).  To call the object, you do *not* need to
convert it to a function -- so why do that in predicate logic leading to the
object call?

I needed to see docs, or something, reminding me of what [function] was for in
order to give an informed opinion on this proposed extension.  The paragraph in
the doc you cited didn't really help!  In reading the xpcwrappedjsclass.cpp:650
code, it seems that [function] causes the 'this' parameter, the object whose
method is being called, to be invoked (so long as it is callable), and the
method name to be ignored.  Right?

I'm slow today: where is [function] used in existing IDL?  I understood jst's
comments dated 2001-05-13 16:22 to say that we should support any JS object as
an nsIDOMEventListener so long as it has the right methods.  Have we hacked
things somehow so that a JS function object is being used as a [function]-tagged
wrapped JS class, in order to satisfy a DOM event listener API, and that somehow
precludes us from using a non-function (non-callable) object alternatively to
satisfy the same interface requirements?

If I've got that right, then I agree.  But I need to see where [function] is
used, and lxr is not cooperating.

/be

Comment 20

[John Bandhauer]

brendan: I was not discounting what you'd already said.

try:
http://lxr.mozilla.org/seamonkey/search?string=%5Bscriptable%2C+function

Yes. We use [function] to mean: "Treat the object that is claiming to implement 
this interface as a function. Any method calls on this interface will be routed 
as function calls on that object." In order to support DOM event 'this' setting 
rules we added a generic 'thisTranslator' scheme to support setting the 
appropriate 'this' via JS specific helper code (so the native object calling 
into one of these interface need not do anything special).

What my code was trying to do in the IsCallableObject function was to perform 
the same conversion that the JS engine would later attempt (is this not true?). 
I was using the failure of that conversion to indicate that the object simply is 
not going to be callable. I'm fine with the simpler JS_TypeOfValue test instead.

As you'll see in the lxr link above the only place we use this (except my simple 
test) is to support 'legacy' DOM event callback semantics. The JS caller sets a 
function (or text we compile into a function) as an event handler. We do that 
'set' by building a wrappedJS and handing the native proxy part of that 
wrappedJS to the code that manages the later dispatching of events.

Later when an event is to be sent, that native code simply does an xpcom method 
call like: foo->HandleEvent(SomeEvent). the code in xpcwrappedjsclass.cpp 
discovers that the interface being called is marked as [function] so it attempts 
to call the wrapped JSObject as a function rather than lookup the named method 
on it. We added the 'thisTranslator' as mentioned above to get the semantics of 
the 'this' that the executing JS event handler expects to work correctly (jst's 
code registers with xpconnect a thisHandler for the given interface iid which 
extracts the correct 'this' from the event param, asks xpconnect to build a 
wrappedNative wrapper around it, and returns the JSObject of that wrapper as the 
'this' that should be used in the call to the JS event handler).

So, when it comes down to it... The *generic* extension being discussed is to 
extend the rules for [function] to be something like: "Treat the object that is 
claiming to implement this interface as a function. Any method calls on this 
interface will be routed as function calls on that object. UNLESS the object is 
not a function, then the normal named method lookup and dispatch will occur."

The specific behavior extension for DOM events is that if a non-function object 
is set as an event handler then we will do a lookup for its "handleEvent" method 
and call it with the event as a param (and the handler is on its own in terms of 
figuring out the 'source' of the event). 

It is this non-standard extension to DOM event handling that I'm concerned with. 
I agree that it better fits the DOM interface declarations. I just wonder if it 
is a proprietary extension that we really want. This is not my area and I'm 
happy to defer to the consensus of others.

Hope this helps. Thanks for taking the time to look at this.

Comment 21

[Brendan Eich [:brendan]]

As I said, converting to a function is different from calling an object.  A call
to an object will not convert that object to a function except in some weird
case that no one claims to understand any longer (js_Invoke, jsinterp.c:660).  I
hope to clean that up and see what breaks soon; I'll report the news in a bug.

Do you rely on that convert call that no own understands?  I hope not.

I'm ok with this extension.  It seems better to let JS objects implement
nsIDOMEventListener and be passed to the obvious DOM methods (addEventListener)
than to require closures or functions with ad-hoc properties to convey extra
arguments.  In fact, does the w3c DOM specify a JS binding that dictates *only*
function objects as parameters to such functions?  I would be surprised.

/be

Comment 22

[John Bandhauer]

> Do you rely on that convert call that no own understands?  I hope not.

I would not say I "rely" on that. All xpconnect does when calling wrapped 
JSObject is push the jsval for the JSObject (and the 'this' and params, of 
course) and call js_Invoke. I see that js_Invoke has code paths that do 
conversions if the JSObject is not of class js_FunctionClass. I was just trying 
to write correct code. 

'Normal' xpconnect wrappers are not callable as functions. But, it is possible 
to use nsIXPCScriptable to produce callable JSObjects that are not of class 
js_FunctionClass. Of course, JS_TypeOfValue will deal with that case. I know of 
no code that implements a convert callback to yield a callable object for 
objects that would not already be identifiable as callable using JS_TypeOfValue.

You might notice that I already posted a patch that does as you suggested.

Comment 23

[Brendan Eich [:brendan]]

Sorry, I saw the patch -- looks great, r/sr=[e-mail].  I was still
trying to get to the bottom of [function], what the DOM spec says about the JS
binding, if anything, and whether XPConnect implemeneted JSClass.convert but not
JSClass.call.

/be

Comment 24

[Johnny Stenback (:jst)]

The ECMA bindings in the DOM Event spec does say that the EventListener argument
to addEventListener() n' friends should be a function reference, but since we
don't break that I'm ok with this extension.

http://www.w3.org/TR/2000/REC-DOM-Level-2-Events-20001113/ecma-script-binding.html

Comment 25

[Johnny Stenback (:jst)]

Jband, I discussed this with Vidur and we both agree that we sohuld support both
ways of registering event listeners. Please go ahead and check this in ASAP
(today if possible), r/sr=jst

Comment 26

[John Bandhauer]

fix checked in.

Comment 27

[Vladimir Ermakov]

QA Contact Update

Comment 28

[Vladimir Ermakov]

verifying on build 2002-03-06-04