Opened 10 years ago
Closed 10 years ago
#14574 closed task (fixed)
consider using event delegation in _ListMouseMixin
Reported by: | bill | Owned by: | Douglas Hays |
---|---|---|---|
Priority: | high | Milestone: | 1.8 |
Component: | Dijit - Form | Version: | 1.7.1 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
_ListMouseMixin could leverage dojo/on's event delegation features to track events at the level of list items, without worrying about complications from sub-nodes within each list item: i.e. having to trace evt.target up the DOM tree, ignoring mouseout from a list item into a subnode of that same list item, etc.
I'm attaching a patch which seems to work but I don't understand the intricacies of the _ListMouseMixin code, in particular about the distinction between dragging and hovering.
Attachments (3)
Change History (11)
Changed 10 years ago by
Attachment: | listEventDelegation.patch added |
---|
comment:1 Changed 10 years ago by
Cc: | Kris Zyp ykami added |
---|
PS: the downside of this is the dojo.query() dependency. I filed #14575 to see if that can be worked around.
comment:2 Changed 10 years ago by
OK, Kris enhanced dojo/on so the dojo.query dependency is no longer necessary to do event delegation. But looking at _ListTouchMixin.js (and it's hard to test due to #14124), it doesn't seem like dojox/mobile needs event delegation at all.
Attaching a new patch.
If for some reason in dojox/mobile each list item has sub-nodes, and the click event might occur on a sub-node, then something like this should work:
var self = this, root = this.domNode; this._connects.push(on(root, on.selector(function(evt){ return evt.target.parentNode == root; }, "click"), function(evt){ self._onClick(this, evt); }));
Perhaps that code is there for the case when list items are rich text, and a click event could occur on <b>, <i>, etc.?
comment:3 Changed 10 years ago by
Bill, just a note, make sure your custom selector function actually returns a node, if it matches, as it is responsible for actually *finding* the node, not just indicating a boolean match (and perhaps my fix for #14575 was exactly what you intended).
comment:4 Changed 10 years ago by
@kzyp - By
*finding* the node
I assume you mean just returning the node, rather than tracing up the tree looking for a node?
It's true that the example in your test file returns a node:
on.selector(function(node){ return node.tagName == "BUTTON" && node; }, "click")
But I'm confused, I thought the custom selector function was supposed to match (no pun intended) the behavior of dojo.query.matches(), which returns a boolean:
query.matches = engine.match || function(node, selector, root){ // summary: // Test to see if a node matches a selector return query.filter([node], selector, root).length > 0; };
comment:6 Changed 10 years ago by
Cc: | Kris Zyp ykami removed |
---|---|
Milestone: | tbd → 1.8 |
Status: | new → assigned |
added patch for testing
comment:7 Changed 10 years ago by
I looked at it, and it seems to be working. A few things:
- I got very confused by the function name_onListItem(), which sounds like it's called every time an event happens (like _onClick()), but is actually a helper for setting up connections. I guess you should just call it something else, plus of course add a summary.
- The event.stop() calls look scary since I had that in _HasDropDown but had to change it to just preventDefault() instead. Another downside is that they introduce a dependency on dojo/_base/event.
- For some reason you added spurious spaces to some of the comments.
patch against [27509]