I have found it very handy while building widgets to be able to
connect custom signals on DOM elements to arbitrary handlers.
Unfortunately "signal( elem, signalName, param1, param2, ...)" didn't
work as expected.
I traced this down to the isDOM check in the connect function.
Signal.connect is assuming that if the src object has an
addEventListener or an attachEvent attribute then the handler wants to
be passed the MochiKit.Event object. For my handlers bound to custom
signals this is not the case, it wouldn't really matter except that I
also like to pass arguments via MochiKit.signal to these handlers.
My fix is based on the documented assumption that MochiKit assumes an
`on' prefix for all DOM event signals. I assume that MochiKit's goal
here is to homogonise the different events between browsers. For
example, MochiKit assumes `on...' and then removes it for browsers
that want to have `click' connected instead of 'onclick', etc.
Thus if in Signal.connect the isDOM calculation is strengthened to
make this assumption explicit then we get the ability to connect
custom signals to DOM elements and have Signal.signal pass any extra
parameters correctly. Eg;
var isDOM = (src.addEventListener || src.attachEvent) &&
(sig.substr(0,2) === 'on');
The only caveat is that custom signals cannot start with 'on' which I
think is acceptable as that is already a documented assumption by
MochiKit.
This allows for the following code to work;
var myHandler = function( one, two, three ) { log( one, two,
three ); };
connect( 'my-div', 'my-custom-event', myHandler );
signal ( 'my-div', 'my-custom-event', 1, 2, 3 );
this will log "1 2 3".
Below is a diff against SVN Revision 1368 including changes to the
test suite.
I have tested against FF2 and IE7 which is all I have access to, I
would appreciate it if others with access to other browsers could run
the test too.
- var isDOM = !!(src.addEventListener || src.attachEvent);
+ // The documentation states that DOM signals all start with
+ // 'on', so make this an explicit check, which has the bonus
+ // of allowing us to connect custom signals to elements and
+ // have our handler get called with the params passed to
+ // signal instead of the wrapped native Event object.
+ var isDOM = (src.addEventListener || src.attachEvent) &&
(sig.substr(0,2) === 'on');
+
if (isDOM && (sig === "onmouseenter" || sig ===
"onmouseleave")
&& !self._browserAlreadyHasMouseEnterAndLeave()) {
var listener = self._mouseEnterListener(src,
sig.substr(2), func, obj);
@@ -626,19 +632,20 @@
var listener = self._listener(src, sig, func, obj,
isDOM);
}
-
+
ident = connect('submit', 'onmousedown', eventTest);
triggerMouseEvent('submit', 'mousedown', false);
t.is(i, 3, 'Connecting an event to an HTML object and firing
a synthetic event');
@@ -107,39 +107,61 @@
triggerMouseEvent('submit', 'mousedown', false);
t.is(i, 3, 'Disconnecting an event to an HTML object and
firing a synthetic event');
+ // Custom Signals bound to thunks on DOM Elements do not get
+ // wrapped in an event object, when they shouldn't.
+ var customSubmitMethodParam1 = null;
+ var customSubmitMethodParam2 = null;
+ var customSubmitMethod = function( param1, param2 ){
+ // console.log( 'customSubmitMethod called with this bound
to:', this, 'params are', param1, param2 );
+ customSubmitMethodParam1 = param1;
+ customSubmitMethodParam2 = param2;
+ };
+ var customDiv = getElement('custom-signal-test');
+ update( customDiv, { dynamicMethod : customSubmitMethod } );
+ bindMethods( customDiv );
+ ident = connect( 'custom-signal-test', 'custom-event',
customDiv.dynamicMethod);
+ signal( 'custom-signal-test', 'custom-event', 'PARAM1',
'PARAM2' );
-
- }
+ t.ok( (customSubmitMethodParam1 === 'PARAM1' &&
+ customSubmitMethodParam2 === 'PARAM2'), 'checking that
methods bound to elements are correctly signaled' );
+
+ disconnect(ident);
+ ident = connect('custom-signal-test', 'custom-event',
customSubmitMethod);
+
...
I've also been bitten by this same issue, but didn't dig deeper into it before. Great analysis and nice with a patched solution!
Looking at the code in MochiKit.Signal, I have two comments:
1. connect(src, signal, dst, func) uses late binding to dst[func] for DOM signals, but uses early binding in other cases (with MochiKit.Base.bind). I think we should use late binding at all times instead.
2. I don't agree with the solution to the issue that signal() doesn't fulfill its contract (as per the documentation). Rather, I think a better solution would be to modify signal() to call ident.objOrFunc and ident.objOrFunc directly instead of using ident.listener. Then it would be fully up to the caller of signal() which arguments are to be specified and in which order. Using the "on" prefix seems a bit like an ad-hoc work-around to me.
Cheers,
/Per
On Mon, May 5, 2008 at 7:44 AM, simon.cus...@gmail.com
> I have found it very handy while building widgets to be able to > connect custom signals on DOM elements to arbitrary handlers. > Unfortunately "signal( elem, signalName, param1, param2, ...)" didn't > work as expected.
> I traced this down to the isDOM check in the connect function.
> Signal.connect is assuming that if the src object has an > addEventListener or an attachEvent attribute then the handler wants to > be passed the MochiKit.Event object. For my handlers bound to custom > signals this is not the case, it wouldn't really matter except that I > also like to pass arguments via MochiKit.signal to these handlers.
> My fix is based on the documented assumption that MochiKit assumes an > `on' prefix for all DOM event signals. I assume that MochiKit's goal > here is to homogonise the different events between browsers. For > example, MochiKit assumes `on...' and then removes it for browsers > that want to have `click' connected instead of 'onclick', etc.
> Thus if in Signal.connect the isDOM calculation is strengthened to > make this assumption explicit then we get the ability to connect > custom signals to DOM elements and have Signal.signal pass any extra > parameters correctly. Eg;
> The only caveat is that custom signals cannot start with 'on' which I > think is acceptable as that is already a documented assumption by > MochiKit.
> This allows for the following code to work;
> var myHandler = function( one, two, three ) { log( one, two, > three ); }; > connect( 'my-div', 'my-custom-event', myHandler ); > signal ( 'my-div', 'my-custom-event', 1, 2, 3 );
> this will log "1 2 3".
> Below is a diff against SVN Revision 1368 including changes to the > test suite.
> I have tested against FF2 and IE7 which is all I have access to, I > would appreciate it if others with access to other browsers could run > the test too.
> - var isDOM = !!(src.addEventListener || src.attachEvent); > + // The documentation states that DOM signals all start with > + // 'on', so make this an explicit check, which has the bonus > + // of allowing us to connect custom signals to elements and > + // have our handler get called with the params passed to > + // signal instead of the wrapped native Event object. > + var isDOM = (src.addEventListener || src.attachEvent) && > (sig.substr(0,2) === 'on'); > + > if (isDOM && (sig === "onmouseenter" || sig === > "onmouseleave") > && !self._browserAlreadyHasMouseEnterAndLeave()) { > var listener = self._mouseEnterListener(src, > sig.substr(2), func, obj); > @@ -626,19 +632,20 @@ > var listener = self._listener(src, sig, func, obj, > isDOM); > }
I am unsure how making it late bound helps, doesn't that just shift
the problem? How would that behave in the following two cases;
1. A request to connect "onclick" occurs.
Does the listener early bind a function to the native event onclick
which then calls the clients function late bound? If so what
advantage does this have?
2. A request to connect(someDomElement, "my-custom-signal",
myHandler); occurs.
When signal(someDomElement, "my-custom-signal", "param1", "param2")
occurs how does the late binding know whether to try and wrap a
native event to pass to myHandler or whether to pass the arguments
passed to signal? Wont signal() still need to look at the signal
name to determine if it is a DOM event handler or not?
My original point about the documentation was not so much that I
thought the contract wasn't being fullfilled but rather that in order
to implement cross browser DOM event handling MochiKit had already
decided to always expect the "on" prefix and to remove it for those
browsers that don't want it. (MochiKit could have gone the other way
and never accepted the "on" when registering events but then it would
have to add it for those browsers that do want it).
So seeing as MochiKit has already made the decision that all DOM Event
signals must start with "on" then we could take advantage of this
documented expectation to allow handling for custom signals on DOM
Elements without breaking any backwards compatibility.
On a side note, I am currently working on a patch for handling mouse
scroll events in a uniform way. Different browsers use completely
different names for these events I was planning on early binding
browser specific handlers that normalise the values, etc so I am keen
to solve this problem so that my next patch wont conflict.
Regards, Simon.
On May 6, 6:00 pm, "Per Cederberg" <cederb...@gmail.com> wrote:
> I've also been bitten by this same issue, but didn't dig deeper into
> it before. Great analysis and nice with a patched solution!
> Looking at the code in MochiKit.Signal, I have two comments:
> 1. connect(src, signal, dst, func) uses late binding to dst[func] for
> DOM signals, but uses early binding in other cases (with
> MochiKit.Base.bind). I think we should use late binding at all times
> instead.
> 2. I don't agree with the solution to the issue that signal() doesn't
> fulfill its contract (as per the documentation). Rather, I think a
> better solution would be to modify signal() to call ident.objOrFunc
> and ident.objOrFunc directly instead of using ident.listener. Then it
> would be fully up to the caller of signal() which arguments are to be
> specified and in which order. Using the "on" prefix seems a bit like
> an ad-hoc work-around to me.
> Cheers,
> /Per
> On Mon, May 5, 2008 at 7:44 AM, simon.cus...@gmail.com
> > I have found it very handy while building widgets to be able to
> > connect custom signals on DOM elements to arbitrary handlers.
> > Unfortunately "signal( elem, signalName, param1, param2, ...)" didn't
> > work as expected.
> > I traced this down to the isDOM check in the connect function.
> > Signal.connect is assuming that if the src object has an
> > addEventListener or an attachEvent attribute then the handler wants to
> > be passed the MochiKit.Event object. For my handlers bound to custom
> > signals this is not the case, it wouldn't really matter except that I
> > also like to pass arguments via MochiKit.signal to these handlers.
> > My fix is based on the documented assumption that MochiKit assumes an
> > `on' prefix for all DOM event signals. I assume that MochiKit's goal
> > here is to homogonise the different events between browsers. For
> > example, MochiKit assumes `on...' and then removes it for browsers
> > that want to have `click' connected instead of 'onclick', etc.
> > Thus if in Signal.connect the isDOM calculation is strengthened to
> > make this assumption explicit then we get the ability to connect
> > custom signals to DOM elements and have Signal.signal pass any extra
> > parameters correctly. Eg;
> > The only caveat is that custom signals cannot start with 'on' which I
> > think is acceptable as that is already a documented assumption by
> > MochiKit.
> > This allows for the following code to work;
> > var myHandler = function( one, two, three ) { log( one, two,
> > three ); };
> > connect( 'my-div', 'my-custom-event', myHandler );
> > signal ( 'my-div', 'my-custom-event', 1, 2, 3 );
> > this will log "1 2 3".
> > Below is a diff against SVN Revision 1368 including changes to the
> > test suite.
> > I have tested against FF2 and IE7 which is all I have access to, I
> > would appreciate it if others with access to other browsers could run
> > the test too.
> > - var isDOM = !!(src.addEventListener || src.attachEvent);
> > + // The documentation states that DOM signals all start with
> > + // 'on', so make this an explicit check, which has the bonus
> > + // of allowing us to connect custom signals to elements and
> > + // have our handler get called with the params passed to
> > + // signal instead of the wrapped native Event object.
> > + var isDOM = (src.addEventListener || src.attachEvent) &&
> > (sig.substr(0,2) === 'on');
> > +
> > if (isDOM && (sig === "onmouseenter" || sig ===
> > "onmouseleave")
> > && !self._browserAlreadyHasMouseEnterAndLeave()) {
> > var listener = self._mouseEnterListener(src,
> > sig.substr(2), func, obj);
> > @@ -626,19 +632,20 @@
> > var listener = self._listener(src, sig, func, obj,
> > isDOM);
> > }
Sorry, I was probably unclear in my previous post. In this comment:
>1. connect(src, signal, dst, func) uses late binding to dst[func] for >DOM signals, but uses early binding in other cases (with >MochiKit.Base.bind). I think we should use late binding at all times >instead.
I was not referring to your suggested patch. Rather that MochiKit sometimes does the dst[func] --> <function object> lookup when calling connect(), and sometimes only when the actual signals happen (late binding).
I think late binding is better, since then it will be possible to redefine functions, etc. In either way, I think the behavior should be consistent. Actually, thinking about this I registered a new Trac ticket:
Anyway, my main point was that MochiKit.Signal.signal(...) doesn't do what the documentation says. So I think it should be corrected, as the documented behavior seems reasonable to me. Think that is another Trac ticket... :-)
Cheers,
/Per
On Wed, May 14, 2008 at 9:03 AM, simon.cus...@gmail.com
> I am unsure how making it late bound helps, doesn't that just shift > the problem? How would that behave in the following two cases;
> 1. A request to connect "onclick" occurs.
> Does the listener early bind a function to the native event onclick > which then calls the clients function late bound? If so what > advantage does this have?
> 2. A request to connect(someDomElement, "my-custom-signal", > myHandler); occurs.
> When signal(someDomElement, "my-custom-signal", "param1", "param2") > occurs how does the late binding know whether to try and wrap a > native event to pass to myHandler or whether to pass the arguments > passed to signal? Wont signal() still need to look at the signal > name to determine if it is a DOM event handler or not?
> My original point about the documentation was not so much that I > thought the contract wasn't being fullfilled but rather that in order > to implement cross browser DOM event handling MochiKit had already > decided to always expect the "on" prefix and to remove it for those > browsers that don't want it. (MochiKit could have gone the other way > and never accepted the "on" when registering events but then it would > have to add it for those browsers that do want it).
> So seeing as MochiKit has already made the decision that all DOM Event > signals must start with "on" then we could take advantage of this > documented expectation to allow handling for custom signals on DOM > Elements without breaking any backwards compatibility.
> On a side note, I am currently working on a patch for handling mouse > scroll events in a uniform way. Different browsers use completely > different names for these events I was planning on early binding > browser specific handlers that normalise the values, etc so I am keen > to solve this problem so that my next patch wont conflict.
> Regards, Simon.
> On May 6, 6:00 pm, "Per Cederberg" <cederb...@gmail.com> wrote: >> I've also been bitten by this same issue, but didn't dig deeper into >> it before. Great analysis and nice with a patched solution!
>> Looking at the code in MochiKit.Signal, I have two comments:
>> 1. connect(src, signal, dst, func) uses late binding to dst[func] for >> DOM signals, but uses early binding in other cases (with >> MochiKit.Base.bind). I think we should use late binding at all times >> instead.
>> 2. I don't agree with the solution to the issue that signal() doesn't >> fulfill its contract (as per the documentation). Rather, I think a >> better solution would be to modify signal() to call ident.objOrFunc >> and ident.objOrFunc directly instead of using ident.listener. Then it >> would be fully up to the caller of signal() which arguments are to be >> specified and in which order. Using the "on" prefix seems a bit like >> an ad-hoc work-around to me.
>> Cheers,
>> /Per
>> On Mon, May 5, 2008 at 7:44 AM, simon.cus...@gmail.com
>> > I have found it very handy while building widgets to be able to >> > connect custom signals on DOM elements to arbitrary handlers. >> > Unfortunately "signal( elem, signalName, param1, param2, ...)" didn't >> > work as expected.
>> > I traced this down to the isDOM check in the connect function.
>> > Signal.connect is assuming that if the src object has an >> > addEventListener or an attachEvent attribute then the handler wants to >> > be passed the MochiKit.Event object. For my handlers bound to custom >> > signals this is not the case, it wouldn't really matter except that I >> > also like to pass arguments via MochiKit.signal to these handlers.
>> > My fix is based on the documented assumption that MochiKit assumes an >> > `on' prefix for all DOM event signals. I assume that MochiKit's goal >> > here is to homogonise the different events between browsers. For >> > example, MochiKit assumes `on...' and then removes it for browsers >> > that want to have `click' connected instead of 'onclick', etc.
>> > Thus if in Signal.connect the isDOM calculation is strengthened to >> > make this assumption explicit then we get the ability to connect >> > custom signals to DOM elements and have Signal.signal pass any extra >> > parameters correctly. Eg;
>> > The only caveat is that custom signals cannot start with 'on' which I >> > think is acceptable as that is already a documented assumption by >> > MochiKit.
>> > This allows for the following code to work;
>> > var myHandler = function( one, two, three ) { log( one, two, >> > three ); }; >> > connect( 'my-div', 'my-custom-event', myHandler ); >> > signal ( 'my-div', 'my-custom-event', 1, 2, 3 );
>> > this will log "1 2 3".
>> > Below is a diff against SVN Revision 1368 including changes to the >> > test suite.
>> > I have tested against FF2 and IE7 which is all I have access to, I >> > would appreciate it if others with access to other browsers could run >> > the test too.
>> > - var isDOM = !!(src.addEventListener || src.attachEvent); >> > + // The documentation states that DOM signals all start with >> > + // 'on', so make this an explicit check, which has the bonus >> > + // of allowing us to connect custom signals to elements and >> > + // have our handler get called with the params passed to >> > + // signal instead of the wrapped native Event object. >> > + var isDOM = (src.addEventListener || src.attachEvent) && >> > (sig.substr(0,2) === 'on'); >> > + >> > if (isDOM && (sig === "onmouseenter" || sig === >> > "onmouseleave") >> > && !self._browserAlreadyHasMouseEnterAndLeave()) { >> > var listener = self._mouseEnterListener(src, >> > sig.substr(2), func, obj); >> > @@ -626,19 +632,20 @@ >> > var listener = self._listener(src, sig, func, obj, >> > isDOM); >> > }
> I am unsure how making it late bound helps, doesn't that just shift > the problem? How would that behave in the following two cases;
> 1. A request to connect "onclick" occurs.
> Does the listener early bind a function to the native event onclick > which then calls the clients function late bound? If so what > advantage does this have?
> 2. A request to connect(someDomElement, "my-custom-signal", > myHandler); occurs.
> When signal(someDomElement, "my-custom-signal", "param1", "param2") > occurs how does the late binding know whether to try and wrap a > native event to pass to myHandler or whether to pass the arguments > passed to signal? Wont signal() still need to look at the signal > name to determine if it is a DOM event handler or not?
> My original point about the documentation was not so much that I > thought the contract wasn't being fullfilled but rather that in order > to implement cross browser DOM event handling MochiKit had already > decided to always expect the "on" prefix and to remove it for those > browsers that don't want it. (MochiKit could have gone the other way > and never accepted the "on" when registering events but then it would > have to add it for those browsers that do want it).
> So seeing as MochiKit has already made the decision that all DOM Event > signals must start with "on" then we could take advantage of this > documented expectation to allow handling for custom signals on DOM > Elements without breaking any backwards compatibility.
> On a side note, I am currently working on a patch for handling mouse > scroll events in a uniform way. Different browsers use completely > different names for these events I was planning on early binding > browser specific handlers that normalise the values, etc so I am keen > to solve this problem so that my next patch wont conflict.
> Regards, Simon.
> On May 6, 6:00 pm, "Per Cederberg" <cederb...@gmail.com> wrote: >> I've also been bitten by this same issue, but didn't dig deeper into >> it before. Great analysis and nice with a patched solution!
>> Looking at the code in MochiKit.Signal, I have two comments:
>> 1. connect(src, signal, dst, func) uses late binding to dst[func] for >> DOM signals, but uses early binding in other cases (with >> MochiKit.Base.bind). I think we should use late binding at all times >> instead.
>> 2. I don't agree with the solution to the issue that signal() doesn't >> fulfill its contract (as per the documentation). Rather, I think a >> better solution would be to modify signal() to call ident.objOrFunc >> and ident.objOrFunc directly instead of using ident.listener. Then it >> would be fully up to the caller of signal() which arguments are to be >> specified and in which order. Using the "on" prefix seems a bit like >> an ad-hoc work-around to me.
>> Cheers,
>> /Per
>> On Mon, May 5, 2008 at 7:44 AM, simon.cus...@gmail.com
>> > I have found it very handy while building widgets to be able to >> > connect custom signals on DOM elements to arbitrary handlers. >> > Unfortunately "signal( elem, signalName, param1, param2, ...)" didn't >> > work as expected.
>> > I traced this down to the isDOM check in the connect function.
>> > Signal.connect is assuming that if the src object has an >> > addEventListener or an attachEvent attribute then the handler wants to >> > be passed the MochiKit.Event object. For my handlers bound to custom >> > signals this is not the case, it wouldn't really matter except that I >> > also like to pass arguments via MochiKit.signal to these handlers.
>> > My fix is based on the documented assumption that MochiKit assumes an >> > `on' prefix for all DOM event signals. I assume that MochiKit's goal >> > here is to homogonise the different events between browsers. For >> > example, MochiKit assumes `on...' and then removes it for browsers >> > that want to have `click' connected instead of 'onclick', etc.
>> > Thus if in Signal.connect the isDOM calculation is strengthened to >> > make this assumption explicit then we get the ability to connect >> > custom signals to DOM elements and have Signal.signal pass any extra >> > parameters correctly. Eg;
>> > The only caveat is that custom signals cannot start with 'on' which I >> > think is acceptable as that is already a documented assumption by >> > MochiKit.
>> > This allows for the following code to work;
>> > var myHandler = function( one, two, three ) { log( one, two, >> > three ); }; >> > connect( 'my-div', 'my-custom-event', myHandler ); >> > signal ( 'my-div', 'my-custom-event', 1, 2, 3 );
>> > this will log "1 2 3".
>> > Below is a diff against SVN Revision 1368 including changes to the >> > test suite.
>> > I have tested against FF2 and IE7 which is all I have access to, I >> > would appreciate it if others with access to other browsers could run >> > the test too.
>> > - var isDOM = !!(src.addEventListener || src.attachEvent); >> > + // The documentation states that DOM signals all start with >> > + // 'on', so make this an explicit check, which has the bonus >> > + // of allowing us to connect custom signals to elements and >> > + // have our handler get called with the params passed to >> > + // signal instead of the wrapped native Event object. >> > + var isDOM = (src.addEventListener || src.attachEvent) && >> > (sig.substr(0,2) === 'on'); >> > + >> > if (isDOM && (sig === "onmouseenter" || sig === >> > "onmouseleave") >> > && !self._browserAlreadyHasMouseEnterAndLeave()) { >> > var listener = self._mouseEnterListener(src, >> > sig.substr(2), func, obj); >> > @@ -626,19 +632,20 @@ >> > var listener = self._listener(src, sig, func, obj, >> > isDOM); >> > }
