Opened 8 years ago

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

listEventDelegation.patch (6.6 KB) - added by bill 8 years ago.
patch against [27509]
listEventDelegation2.patch (6.0 KB) - added by bill 8 years ago.
patch against [27513]
14574.patch (7.5 KB) - added by Douglas Hays 7 years ago.
revised patch w/o query dep

Download all attachments as: .zip

Change History (11)

Changed 8 years ago by bill

Attachment: listEventDelegation.patch added

patch against [27509]

comment:1 Changed 8 years ago by bill

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 8 years ago by bill

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

Last edited 8 years ago by bill (previous) (diff)

Changed 8 years ago by bill

Attachment: listEventDelegation2.patch added

patch against [27513]

comment:3 Changed 8 years ago by Kris Zyp

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 8 years ago by bill

@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:5 Changed 7 years ago by bill

Reopened #14575 as it isn't working as I expected.

Changed 7 years ago by Douglas Hays

Attachment: 14574.patch added

revised patch w/o query dep

comment:6 Changed 7 years ago by Douglas Hays

Cc: Kris Zyp ykami removed
Milestone: tbd1.8
Status: newassigned

added patch for testing

comment:7 Changed 7 years ago by bill

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.

comment:8 Changed 7 years ago by Douglas Hays

Resolution: fixed
Status: assignedclosed

In [28829]:

Fixes #14574. Change this.connect to this.own(on(... and add list item target as second parameter to event handler callbacks.`

Note: See TracTickets for help on using tickets.