#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, [email protected]…, ben hockey, dante, Rawld Gill | |
Blocked By: | Blocking: |
Description (last modified by )
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
, andonmousenter
) 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 forkeypress
,onmouseleave
, andonmousenter
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 todojo/_base/keypress.js
as a custom extension/emulation event.mouseenter
/mouseleave
handling - moved todojo/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 tolisten()
function -dojo.query(".class").on("some-event", callback);
- Return value from
connect()
andlisten()
/on()
is an object with three methods:cancel()
,pause()
, andresume()
dojo.disconnect(handle)
still works, but can dohandle.cancel()
instead (as well as pause and resume) dojo/listen
provides an Evented base class that can be extended for event emitting objects, providingon()
andemit()
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)
Change History (84)
comment:1 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:2 Changed 10 years ago by
Cc: | Eugene Lazutkin added |
---|---|
Description: | modified (diff) |
comment:3 Changed 10 years ago by
Milestone: | tbd → 1.7 |
---|
comment:5 Changed 10 years ago by
Cc: | [email protected]… added |
---|
comment:6 Changed 10 years ago by
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 10 years ago by
Attachment: | events.diff added |
---|
The same patch version with https://github.com/kriszyp/dojo/commit/0a82d5042a9e5aaaead563099e0ab016f9e6f9bb fixed
comment:7 Changed 10 years ago by
comment:9 Changed 10 years ago by
Cc: | ben hockey added |
---|
comment:11 Changed 10 years ago by
(In [24545]) Allow undefined argument for backwards compatibility, but don't document it (yet)
comment:12 Changed 10 years ago by
(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 10 years ago by
For reference, [24471] was the first checkin, trac didn't notice it.
comment:14 Changed 10 years ago by
comment:15 Changed 10 years ago by
comment:16 Changed 10 years ago by
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 10 years ago by
comment:19 Changed 10 years ago by
comment:21 Changed 10 years ago by
comment:22 follow-up: 24 Changed 10 years ago by
comment:23 Changed 10 years ago by
comment:24 Changed 10 years ago by
comment:26 Changed 10 years ago by
comment:27 Changed 10 years ago by
comment:28 Changed 10 years ago by
comment:29 follow-up: 32 Changed 10 years ago by
comment:30 Changed 10 years ago by
comment:31 Changed 10 years ago by
comment:32 Changed 10 years ago by
comment:33 Changed 10 years ago by
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; 276 276 event = newEvent; 277 277 } 278 event = event || {}; 278 279 do{ 279 280 // 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) 280 281 target[method] && target[method].apply(target, args);
comment:35 Changed 10 years ago by
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 10 years ago by
comment:39 Changed 10 years ago by
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 10 years ago by
Cc: | dante added; phiggins removed |
---|
comment:41 follow-up: 45 Changed 10 years ago by
comment:42 Changed 10 years ago by
comment:43 Changed 10 years ago by
comment:44 follow-up: 47 Changed 10 years ago by
comment:45 Changed 10 years ago by
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 10 years ago by
comment:47 Changed 10 years ago by
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 10 years ago by
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 10 years ago by
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 10 years ago by
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:15 Changed 9 years ago by
Replying to bill:
In [26493]:
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:22 follow-up: 24 Changed 9 years ago by
Cc: | Rawld Gill added |
---|---|
Priority: | normal → high |
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:24 Changed 9 years ago by
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:26 Changed 9 years ago by
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:28 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:32 Changed 9 years ago by
Replying to bill:
In [27173]:
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.
tried to fix formatting of description