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: http://trac.mochikit.com/ticket/307 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, [EMAIL PROTECTED] <[EMAIL PROTECTED]> wrote: > > Hi Per, > > Thanks for the feedback. > > 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" <[EMAIL PROTECTED]> 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, [EMAIL PROTECTED] >> >> <[EMAIL PROTECTED]> wrote: >> >> > Hi all, >> >> > 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. >> >> > Regards, Simon Cusack. >> >> > diff -r ae903235ba94 MochiKit/Signal.js >> > --- a/MochiKit/Signal.js Mon May 05 13:37:27 2008 +1000 >> > +++ b/MochiKit/Signal.js Mon May 05 15:39:44 2008 +1000 >> > @@ -63,7 +63,7 @@ >> > str += '}'; >> > } >> > } >> > - if (this.type() == 'mouseover' || this.type() == 'mouseout' >> > || >> > + if (this.type() == 'mouseover' || this.type() == 'mouseout' >> > || >> > this.type() == 'mouseenter' || this.type() == >> > 'mouseleave') { >> > str += ', relatedTarget(): ' + >> > repr(this.relatedTarget()); >> > } >> > @@ -516,10 +516,10 @@ >> > if (sig === 'onload' || sig === 'onunload') { >> > return function (nativeEvent) { >> > obj[func].apply(obj, [new E(src, nativeEvent)]); >> > - >> > + >> > var ident = new MochiKit.Signal.Ident({ >> > source: src, signal: sig, objOrFunc: obj, >> > funcOrStr: func}); >> > - >> > + >> > MochiKit.Signal._disconnect(ident); >> > }; >> > } else { >> > @@ -531,10 +531,10 @@ >> > if (sig === 'onload' || sig === 'onunload') { >> > return function (nativeEvent) { >> > func.apply(obj, [new E(src, nativeEvent)]); >> > - >> > + >> > var ident = new MochiKit.Signal.Ident({ >> > source: src, signal: sig, objOrFunc: func}); >> > - >> > + >> > MochiKit.Signal._disconnect(ident); >> > }; >> > } else { >> > @@ -613,7 +613,13 @@ >> > obj = src; >> > } >> >> > - 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); >> > } >> >> > - if (src.addEventListener) { >> > - src.addEventListener(sig.substr(2), listener, false); >> > - } else if (src.attachEvent) { >> > - src.attachEvent(sig, listener); // useCapture unsupported >> > + if (isDOM) { >> > + if (src.addEventListener) { >> > + src.addEventListener(sig.substr(2), listener, false); >> > + } else if (src.attachEvent) { >> > + src.attachEvent(sig, listener); // useCapture >> > unsupported >> > + } >> > } >> > - >> > var ident = new MochiKit.Signal.Ident({ >> > - source: src, >> > - signal: sig, >> > - listener: listener, >> > - isDOM: isDOM, >> > - objOrFunc: objOrFunc, >> > - funcOrStr: funcOrStr, >> > + source: src, >> > + signal: sig, >> > + listener: listener, >> > + isDOM: isDOM, >> > + objOrFunc: objOrFunc, >> > + funcOrStr: funcOrStr, >> > connected: true >> > }); >> > self._observers.push(ident); >> > diff -r ae903235ba94 tests/test_MochiKit-Signal.html >> > --- a/tests/test_MochiKit-Signal.html Mon May 05 13:37:27 2008 +1000 >> > +++ b/tests/test_MochiKit-Signal.html Mon May 05 15:39:44 2008 +1000 >> > @@ -4,26 +4,29 @@ >> > <script type="text/javascript" src="../MochiKit/Iter.js"></ >> > script> >> > <script type="text/javascript" src="../MochiKit/DOM.js"></script> >> > <script type="text/javascript" src="../MochiKit/Style.js"></ >> > script> >> > - <script type="text/javascript" src="../MochiKit/Signal.js"></ >> > script> >> > - <script type="text/javascript" src="../MochiKit/Logging.js"></ >> > script> >> > - <script type="text/javascript" src="SimpleTest/SimpleTest.js"></ >> > script> >> > + <script type="text/javascript" src="../MochiKit/Signal.js"></ >> > script> >> > + <script type="text/javascript" src="../MochiKit/Logging.js"></ >> > script> >> > + <script type="text/javascript" src="SimpleTest/SimpleTest.js"></ >> > script> >> > <link rel="stylesheet" type="text/css" href="SimpleTest/ >> > test.css"> >> >> > </head> >> > <body> >> >> > Please ignore this button: <input type="submit" id="submit" /><br /> >> > +Please ignore this div: <br /> >> > +<div id="custom-signal-test" /> >> > +<br /> >> >> > <pre id="test"> >> > <script type="text/javascript" src="test_Signal.js"></script> >> > <script type="text/javascript"> >> > try { >> > - >> > + >> > tests.test_Signal({ok:ok, is:is}); >> > ok(true, "test suite finished!"); >> > - >> > + >> > } catch (err) { >> > - >> > + >> > var s = "test suite failure!\n"; >> > var o = {}; >> > var k = null; >> > diff -r ae903235ba94 tests/test_Signal.js >> > --- a/tests/test_Signal.js Mon May 05 13:37:27 2008 +1000 >> > +++ b/tests/test_Signal.js Mon May 05 15:39:44 2008 +1000 >> > @@ -3,7 +3,7 @@ >> > if (typeof(tests) == 'undefined') { tests = {}; } >> >> > tests.test_Signal = function (t) { >> > - >> > + >> > var submit = MochiKit.DOM.getElement('submit'); >> > var ident = null; >> > var i = 0; >> > @@ -14,7 +14,7 @@ >> > i += this.someVar; >> > } >> > }; >> > - >> > + >> > var aObject = {}; >> > aObject.aMethod = function() { >> > t.ok(this === aObject, "aMethod should have 'this' as >> > aObject"); >> > @@ -37,19 +37,19 @@ >> >> > disconnect(ident); >> > submit.click(); >> > - t.is(i, 2, '...and then disconnected'); >> > - >> > - if (MochiKit.DOM.getElement('submit').fireEvent || >> > - (document.createEvent && >> > + t.is(i, 2, '...and then disconnected'); >> > + >> > + if (MochiKit.DOM.getElement('submit').fireEvent || >> > + (document.createEvent && >> > typeof(document.createEvent('MouseEvents').initMouseEvent) == >> > 'function')) { >> > - >> > - /* >> > - >> > - Adapted from: >> > + >> > + /* >> > + >> > + Adapted from: >> > >> > http://www.devdaily.com/java/jwarehouse/jforum/tests/selenium/javascr... >> > License: Apache >> > Copyright: Copyright 2004 ThoughtWorks, Inc >> > - >> > + >> > */ >> > var triggerMouseEvent = function(element, eventType, >> > canBubble) { >> > element = MochiKit.DOM.getElement(element); >> > @@ -98,7 +98,7 @@ >> > t.ok((typeof(e.key()) === 'undefined'), 'checking that >> > key() is undefined'); >> > }; >> >> > - >> > + >> > ident = connect('submit', 'onmousedown', eventTest); >> > triggerMouseEvent('submit', 'mousedown', false); >> > t.is(i, 3, 'Connecting an event to an HTML object and firing >> >> ... >> >> read more ยป > > > > --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "MochiKit" group. To post to this group, send email to mochikit@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/mochikit?hl=en -~----------~----~----~----~------~----~------~--~---
