Event handling improvements

[Kris Zyp]

This is a proposal for some rework of event handling. Here are the main improvements/ideas:

• Uses has() branching
• Function event types, allowing you to create new custom/extension events (like dojo/touch.press) in a safe extensible way. Special handling of certain key events (keypress, onmouseleave, and onmousenter) are implemented using extension events. For example:

  dojo.connect(node, dojo.touch.press, listener);
  

• Introduce a new lightweight dojo/listen module/function that can be used sans the large blocks of corrective code for keypress, onmouseleave, and onmousenter emulation (particular useful to avoid for mobile apps).
• Modularization - Trying to improve the modularity of our event handling: There is several distinct pieces of functionality in dojo/_base/event.js that is broken out:
  • keypress handling - this is moved out to dojo/_base/keypress.js as a custom extension/emulation event.
  • mouseenter/mouseleave handling - moved to dojo/mouse.js as a custom extension/emulation event.
  • aop - Used for listening to regular object's methods, can be used on its own, but leveraged by listen module (dojo/aop)
  • listen - The main listening component, this does not do include special code for specific events, but it does include the IE event normalization code.
  • connect - Back-compatible delegation to the other modules, if you look in connect.js you will see how it does the magical mapping of named events to these custom events

Note that some of these ideas for aop/listening have been suggested for 2.0. However, this patch adds this functionality without breaking compatibility, so it seems like it might be viable in 1.x.

• NodeList.prototype.on() maps to listen() function -

  dojo.query(".class").on("some-event", callback);
  

• Return value from connect() and listen()/on() is an object with three methods: cancel(), pause(), and resume() dojo.disconnect(handle) still works, but can do handle.cancel() instead (as well as pause and resume)
• dojo/listen provides an Evented base class that can be extended for event emitting objects, providing on() and emit() methods (could be used by a future widget class hierarchy)
• listen provides separation of events and methods. You can have a "start" method that conditionally triggers a "start" event (mapped to "onstart"), for example.

[ben hockey]

tried to fix formatting of description

[Chris Mitchell]

patch appears to be out of date with trunk.

[Kris Zyp]

I put the updated event handling code in ​https://github.com/kriszyp/dojo/tree/has-modularize so it is easier to checkout/clone and update until it is committed.

[evan]

The same patch version with ​https://github.com/kriszyp/dojo/commit/0a82d5042a9e5aaaead563099e0ab016f9e6f9bb fixed

[bill]

(In [24406]) rudimentary event tests for existing event functionality (pre-refactor). currently eventMouseRobot.html fails on IE6 because mousedown isn't bubbling. refs #12451.

[Kris Zyp]

(In [24473]) Fix global reference setup, refs #12451 !strict

[bill]

(In [24503]) fix stray global, refs #12451 !strict

[Adam Peller]

(In [24545]) Allow undefined argument for backwards compatibility, but don't document it (yet)

[Kris Zyp]

(In [24547]) Just still within current window for reference redirection (apparently that doesn't cause a leak) to fix the issue with BackForward? failure, refs #12451 !strict

[bill]

For reference, [24471] was the first checkin, trac didn't notice it.

[bill]

(In [24556]) remove unused variable "undef" and fix duplicate declaration of "signal", refs #12451 !strict

[Kris Zyp]

(In [24559]) Keep a reference to corrected touch event so it can be re-used, refs #12451 !strict

[ben hockey]

with reference to r24616, require.has is not guaranteed. this breaks when dojo is loaded via another AMD loader that does not define require.has

[Kris Zyp]

(In [24729]) Suppress errors on setting getter-only properties of native events so "target" property works the same on FF, refs #12451 !strict

[Kris Zyp]

(In [24861]) rename dispatch to emit, refs #12451 !strict

[Kris Zyp]

(In [24862]) Include mouseButtons in dojo/mouse export, refs #12451 !strict

[Kris Zyp]

(In [24873]) rename dispatch to emit, refs #12451 !strict

[Kris Zyp]

(In [25013]) Alias on() to listen() for NodeList? so that listen can be used consistently, #refs #12451 !strict

[bill]

(In [25033]) Refactor widget deferred connect code to leverage new event modules. As a fringe benefit this adds an on() method to _WidgetBase, so code can do myWidget.on("click", func) instead of dojo.connect(myWidget, "onClick", ...).

Refs #5417, #12451. Fixes #12986 !strict.

[Kris Zyp]

(In [25095]) Rename dojo/listen.js to dojo/on.js, refs #12451 !strict

[Kris Zyp]

(In [25096]) Rename dojo/listen.js to dojo/on.js, refs #12451 !strict

[Kris Zyp]

(In [25202]) Rename cancel() to remove(), refs #12451 !strict

[bill]

(In [25209]) missed spot in dojo/core renaming cancel() to remove() for event handles, refs #12451 !strict

[bill]

(In [25211]) renaming cancel() to remove() for event handles in dijit, refs #12451 !strict

[bill]

(In [25255]) renaming cancel() to remove() for event handles in dijit, missed a spot in first checkin, refs #12451 !strict

[bill]

(In [25308]) Shave some bytes by using this._connects[] for both connects() and subscribes(). Refs #12451 !strict.

[Kris Zyp]

(In [25466]) Fix issue with apply advice to instances after applying advice to prototype, refs #12451 !strict

[Kris Zyp]

(In [25783]) Move dojo.keys to own module, move event geometry normalization to dom-geometry (covering Opera case there too), use on.emit to publish, add multiple arg support to on.emit, remove on.publish, add support for listening to window events in two arg on(), fixes #13436, refs #12451 !strict

[Bryan Forbes]

(In [25786]) Added ./_base/sniff to the dependency list for dojo/keys and also use has instead of dojo.is*. refs #12451 !strict

[ben hockey]

i don't know if i just stumbled on this or if a recent change introduced it. i think it used to be possible to emit an event that is null or undefined? if its supported, something like this patch should be applied

on.js

@@ -275 +275 @@
                         newEvent.type = type;
                         event = newEvent;
                 }
+                event = event || {};
                 do{
                         // call any node which has a handler (note that ideally we would try/catch to simulate normal event propagation but that causes too much pain for debugging)
                         target[method] && target[method].apply(target, args);

[Kris Zyp]

How does it currently fail?

[ben hockey]

the condition in the while of the do and the return value.

btw, this patch would convert 0 and false to be objects so i'm not sure its what should be the final patch but that depends on what's allowed to be an event.

[Kris Zyp]

(In [25790]) Handle missing event object properly, refs #12451 !strict

[ben hockey]

thanks

[Kris Zyp]

(In [25838]) Return all exported methods, refs #12451 !strict

[bill]

For some reason [24838] completely breaks the API doc parsing, preview.php says:

Died parsing statements

Line 320, character 5: Expected ':' but got .:'.'

Maybe Pete knows what that means, it doesn't look like the actual problem is on line 320. Anyway you should fix that and make sure that dojo.connect etc. is still showing up in preview.php.

[Kris Zyp]

(In [25860]) Add support for normalization of focusin/focusout so focus events can be used with event delegation. Also use in operator instead of try catch to determine which properties to copy in emit, refs #12451 !strict

[Kris Zyp]

(In [25861]) Deal with Opera's inability to handle repeated keydown events, refs #12451 !strict

[bill]

(In [25867]) try to fix breakages from connect.js module return value change in [25838], refs #12451 !strict

[Rawld Gill]

(In [25873]) fixed docs to work with doc parser; refs #12451; !strict

[ben hockey]

Replying to kzyp:

(In [25860]) Add support for normalization of focusin/focusout so focus events can be used with event delegation. Also use in operator instead of try catch to determine which properties to copy in emit, refs #12451 !strict

this revision has made firefox 5.0 complain with an error "Not enough arguments". i guess the 3rd param to removeListener is not optional in ff5?

[Kris Zyp]

(In [25877]) Readded capture arg on removeEventListener, refs #12451 !strict

[ben hockey]

Replying to rcgill:

(In [25873]) fixed docs to work with doc parser; refs #12451; !strict

is this reference to the dojo global intentional? ​http://bugs.dojotoolkit.org/browser/dojo/dojo/trunk/_base/connect.js?rev=25873#L206

would kernel work instead?

[Sam Foster]

dojo/on is so close to being baseless and dojo-dependency-free. There's just the reference to dojo.isAndroid, and dojo.query. I don't follow the isAndroid thing - that would seem to be a candidate for feature testing and a call to has()? (though I don't know if its is actually feature-testable). The dojo.query reference is used to verify an element matches the selector, so there's an expectation of a specific API, but not necessarily of dojo's.

has and aspect are dependency-free. I would love to be able to drop in dojo/on in a non-dojo project, with a minimum of shimming and package mapping shenanigans. Is that feasible and reasonable do you think?

[Kris Zyp]

Fixed the android test in [25992]. The dojo.query call is actually intentionally done that way to allow for light baseless use, as it allows you to use dojo/on without loading the entirety of dojo/query and dojo/NodeList (which is quite sizable), if you can live without event delegation. It would be nice if we could just use the selector engine rather than the full dojo/NodeList, but that would be probably be a more involved change.

[bill]

I see what you are going for with the dojo.query call, although that trick won't work for long; eventually the dojo variable will disappear completely.

[bill]

In [26058]:

Shave a few more bytes bytes, thanks PEM. Refs #12451 !strict.

[bill]

In [26063]:

Make isLeft() etc. available directly from mouse.js module return value, refs #12451 !strict. I didn't add the other stuff from mouseButtons because it doesn't seem necessary, but we can add it later if someone needs to access it.

[bill]

In [26368]:

For dijit, convert most direct usages of dojo.connect() to use dojo/on, dojo/aspect, or widget.on(). Still leaves _WidgetBase.connect() using dojo.connect(). Plus there's apparently no equivalent of dojo.isCopyKey() so using dojo/_base/connect.js for that too. Refs #12451 !strict.

[Kris Zyp]

In [26489]:

Remove dojo/aspect as a dep from dojo/on and create Evented and topic modules, refs #12451 !strict

[bill]

In [26490]:

Fix references to Evented class (from on.Evented --> Evented), refs #12451 !strict. Not sure how to test the change to socket.js though, I don't see any test case for it.

[bill]

In [26493]:

Since dojo.connect() no longer calls _Widget.on(), need to be more explicit to get deferred connect code to work. Refs #5417, #12451 !strict.

[bill]

In [26494]:

Fix topic publishing/subscribing code to use dojo/topic not dojo/on, refs #12451 !strict.

[ben hockey]

Replying to bill:

In [26493]:

Since dojo.connect() no longer calls _Widget.on(), need to be more explicit to get deferred connect code to work. Refs #5417, #12451 !strict.

is there a reason you didn't leverage the originalConnect that will be passed to the advice?

also, should you be using has or testing for kernel.connect to selectively wrap kernel.connect since it is selectively mixed into kernel? if kernel.connect does not exist, aspect.around will add kernel.connect which would be misleading if someone was testing for the existence of kernel.connect to infer has("dojo-1x-base"). realistically, this will probably never happen and probably isn't a problem but more just for completeness.

function aroundAdvice(originalConnect) {
  // ...
}
aspect.around(connect, "connect", aroundAdvice);
has("dojo-1x-base") &&  aspect.around(kernel, "connect", aroundAdvice);
// alternatively
kernel.connect && aspect.around(kernel, "connect", aroundAdvice);

[bill]

Ah you are right, I will update it like that.

[bill]

In [26497]:

Using originalHandler passed to aroundAdvice, and guard against if dojo.connect() isn't defined because of has("dojo-1x-base") flag. Refs #5417, #12451 !strict.

[Kris Zyp]

In [26498]:

Fix parser test to use Evented object, refs #12451 !strict

[bill]

In [26504]:

Update test file: for the deferred-connect around advice on dojo.connect() to trigger, need to use the "dojo" inside the iframe, not the "dojo" of the test runner. Refs #12520, #12451 !strict.

[bill]

In [26508]:

Safeguard against null first argument to dojo.connect(), refs #5417, #12451 !strict.

[bill]

In [26518]:

avoid error on destroy if this._ownconnects not set, refs #12451 !strict

[Kris Zyp]

In [26733]:

Rename topic methods to publish and subscribe, refs #12451 !strict

[Kris Zyp]

In [26734]:

Rename topic methods to publish and subscribe, refs #12451 !strict

[cjolif]

In [26748]:

Adapt code to follow topic.on to publish.subscribe renaming, refs #12451.

[cjolif]

In [26750]:

Adapt code to follow topic.on to publish.subscribe renaming, refs #12451. !strict.

[Kenneth G. Franqueiro]

In [26768]:

Update pubsub test for dojo/topic. Refs #12451 !strict

[ben hockey]

r24476 has a note about deciding if dojo/_base/connect will go back into non-browser environments. currently it is not included in non-browser environments and as a result we have #13956 and #13957 related to this issue.

r24476 didn't reference a ticket but referenced r24471 which references this ticket - hence the comment here. (adding rawld to this ticket since he committed r24476 and adding the ticket to the hot list for 1.7).

[Kris Zyp]

In [26864]:

Cache events so that events can be mutated in IE like other browsers and improver performance, refs #12451 !strict

[ben hockey]

Replying to neonstalwart:

r24476 has a note about deciding if dojo/_base/connect will go back into non-browser environments. currently it is not included in non-browser environments and as a result we have #13956 and #13957 related to this issue.

r24476 didn't reference a ticket but referenced r24471 which references this ticket - hence the comment here. (adding rawld to this ticket since he committed r24476 and adding the ticket to the hot list for 1.7).

using #13957 to track this issue

[Kris Zyp]

In [26865]:

Revert attempt to cache event, doesn't work in IE6, refs #12451 !strict

[Adam Peller]

What, if any, are the remaining issues for 1.7 in this ticket? At this point, can we close this and create new tickets as needed?

[ben hockey]

all my related issues have been moved to other tickets.

[bill]

In [27173]:

Fix regression from [26368], exception using DropDownButton to open a TooltipDialog with an href, fixes #14363 and refs #12451 on trunk/, !strict.

[bill]

In [27174]:

Fix regression from [26368], exception using DropDownButton to open a TooltipDialog with an href, fixes #14363 and refs #12451 on 1.7/ branch, !strict.

[bill]

In [27207]:

Fix spelling and formatting of API doc, although it doesn't matter since module level doc doesn't show up in the API reference. Refs #12451 !strict.

[Karl Tiedt]

Replying to bill:

In [27173]:

Fix regression from [26368], exception using DropDownButton to open a TooltipDialog with an href, fixes #14363 and refs #12451 on trunk/, !strict.

Just for posterity sake, in our recent testing of 1.7+ this change actually breaks destroyRecursive() in widgets that create Grid + HeaderMenu? via a template where the menu was defined below the Grid in the template... reversing the order by changing dfe() to pop() causes the headerMenu to be destroyed first, and then the grid tries to reference it via its unReplace() code.

Granted this is probably very much an edge case, posting for future searches that may stumble across similar behaviour.

[bill]

In [27986]:

Make better tests for new dojo/query module, refs #12451, #14874, #14875, #14876, #14877, #14879, #14880.

[bill]

In [28620]:

Mark dojo/behavior as deprecated since dojo/on with on.selector() works better, refs #12451.

[bill]

In [30271]:

convert test to AMD, and fix lint error, refs #12451