Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#12451 closed enhancement (fixed)

Event handling improvements

Reported by: Kris Zyp Owned by: Kris Zyp
Priority: high Milestone: 1.7
Component: Events Version: 1.6.0
Keywords: Cc: Eugene Lazutkin, ccmitchellusa@…, ben hockey, dante, Rawld Gill
Blocked By: Blocking:

Description (last modified by Eugene Lazutkin)

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.

Attachments (1)

events.diff (53.8 KB) - added by evan 8 years ago.
The same patch version with https://github.com/kriszyp/dojo/commit/0a82d5042a9e5aaaead563099e0ab016f9e6f9bb fixed

Download all attachments as: .zip

Change History (84)

comment:1 Changed 8 years ago by ben hockey

Description: modified (diff)

tried to fix formatting of description

comment:2 Changed 8 years ago by Eugene Lazutkin

Cc: Eugene Lazutkin added
Description: modified (diff)

comment:3 Changed 8 years ago by Kris Zyp

Milestone: tbd1.7

comment:4 Changed 8 years ago by Chris Mitchell

patch appears to be out of date with trunk.

comment:5 Changed 8 years ago by Chris Mitchell

Cc: ccmitchellusa@… added

comment:6 Changed 8 years ago by 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.

Changed 8 years ago by evan

Attachment: events.diff added

comment:7 Changed 8 years ago by 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.

comment:8 Changed 8 years ago by Kris Zyp

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

comment:9 Changed 8 years ago by ben hockey

Cc: ben hockey added

comment:10 Changed 8 years ago by bill

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

comment:11 Changed 8 years ago by Adam Peller

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

comment:12 Changed 8 years ago by 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

comment:13 Changed 8 years ago by bill

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

comment:14 Changed 8 years ago by bill

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

comment:15 Changed 8 years ago by Kris Zyp

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

comment:16 Changed 8 years ago by 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

comment:17 Changed 8 years ago by 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

comment:18 Changed 8 years ago by Kris Zyp

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

comment:19 Changed 8 years ago by Kris Zyp

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

comment:20 Changed 8 years ago by Kris Zyp

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

comment:21 Changed 8 years ago by Kris Zyp

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

comment:22 Changed 8 years ago by 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.

comment:23 Changed 8 years ago by Kris Zyp

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

comment:24 Changed 8 years ago by Kris Zyp

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

comment:25 Changed 8 years ago by Kris Zyp

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

comment:26 Changed 8 years ago by bill

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

comment:27 Changed 8 years ago by bill

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

comment:28 Changed 8 years ago by bill

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

comment:29 Changed 8 years ago by bill

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

comment:30 Changed 8 years ago by Kris Zyp

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

comment:31 Changed 8 years ago by 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

comment:32 Changed 8 years ago by 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

comment:33 Changed 8 years ago by 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

     
    275275                        newEvent.type = type;
    276276                        event = newEvent;
    277277                }
     278                event = event || {};
    278279                do{
    279280                        // 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)
    280281                        target[method] && target[method].apply(target, args);

comment:34 Changed 8 years ago by Kris Zyp

How does it currently fail?

comment:35 Changed 8 years ago by 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.

comment:36 Changed 8 years ago by Kris Zyp

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

comment:37 Changed 8 years ago by ben hockey

thanks

comment:38 Changed 8 years ago by Kris Zyp

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

comment:39 Changed 8 years ago by bill

Cc: phiggins added

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.

comment:40 Changed 8 years ago by bill

Cc: dante added; phiggins removed

comment:41 Changed 8 years ago by 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

comment:42 Changed 8 years ago by Kris Zyp

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

comment:43 Changed 8 years ago by bill

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

comment:44 Changed 8 years ago by Rawld Gill

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

comment:45 in reply to:  41 Changed 8 years ago by 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?

comment:46 Changed 8 years ago by Kris Zyp

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

comment:47 in reply to:  44 Changed 8 years ago by 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?

comment:48 Changed 8 years ago by 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?

comment:49 Changed 8 years ago by 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.

comment:50 Changed 8 years ago by 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.

comment:51 Changed 8 years ago by bill

In [26058]:

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

comment:52 Changed 8 years ago by 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.

comment:53 Changed 8 years ago by 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.

comment:54 Changed 8 years ago by Kris Zyp

In [26489]:

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

comment:55 Changed 8 years ago by 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.

comment:56 Changed 8 years ago by 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.

comment:14 Changed 8 years ago by bill

In [26494]:

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

comment:15 in reply to:  56 Changed 8 years ago by 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);

comment:16 Changed 8 years ago by bill

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

comment:17 Changed 8 years ago by 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.

comment:15 Changed 8 years ago by Kris Zyp

In [26498]:

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

comment:16 Changed 8 years ago by 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.

comment:17 Changed 8 years ago by bill

In [26508]:

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

comment:16 Changed 8 years ago by bill

In [26518]:

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

comment:17 Changed 8 years ago by Kris Zyp

In [26733]:

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

comment:18 Changed 8 years ago by Kris Zyp

In [26734]:

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

comment:19 Changed 8 years ago by cjolif

In [26748]:

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

comment:20 Changed 8 years ago by cjolif

In [26750]:

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

comment:21 Changed 8 years ago by Kenneth G. Franqueiro

In [26768]:

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

comment:22 Changed 8 years ago by ben hockey

Cc: Rawld Gill added
Priority: normalhigh

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).

comment:23 Changed 8 years ago by Kris Zyp

In [26864]:

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

comment:24 in reply to:  22 Changed 8 years ago by 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

comment:25 Changed 8 years ago by Kris Zyp

In [26865]:

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

comment:26 Changed 8 years ago by 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?

comment:27 Changed 8 years ago by ben hockey

all my related issues have been moved to other tickets.

comment:28 Changed 8 years ago by Kris Zyp

Resolution: fixed
Status: newclosed

comment:29 Changed 8 years ago by 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.

comment:30 Changed 8 years ago by 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.

comment:31 Changed 8 years ago by 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.

comment:32 in reply to:  29 Changed 8 years ago by 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.

comment:33 Changed 8 years ago by bill

In [27986]:

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

comment:34 Changed 7 years ago by bill

In [28620]:

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

comment:35 Changed 7 years ago by bill

In [30271]:

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

Note: See TracTickets for help on using tickets.