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 [email protected]
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
-~----------~----~----~----~------~----~------~--~---
