Opened 4 years ago

Closed 4 years ago

#18355 closed enhancement (fixed)

Add target of selector from on.selector as property of event object

Reported by: Bryan Forbes Owned by:
Priority: high Milestone: 1.11
Component: Events Version: 1.10.2
Keywords: Cc:
Blocked By: Blocking:

Description

Currently, on.selector() (and the 'selector:eventType' syntax of on()) set the target of the selector to the context (this) of the listener. For instance, imagine the following:

<ul id="parent">
    <li class="clickable" data-id="0"><span>Foo</span></li>
    <li data-id="1"><span>Bar</span></li>
</ul>

on(dom.byId('parent'), 'li.clickable:click', function (event) {
    // If the user clicks on the text "Foo", `event.target` will be
    // the <span> within the <li> so it won't have the `data-id`
    // attribute on it. To get the `data-id` attribute, one must use
    // the context of the function:
    var id = this.getAttribute('data-id');
});

This is fine and dandy until you want to bind the context of your listener. This is something that is done quite frequently in widgets:

on(this.containerNode, 'li.clickable:click', lang.hitch(this, 'onRowClick'));

In this scenario, the context of the listener is set to the widget and the target of the selector is lost. It would be nice if the event object passed to listeners would add a property set to the target of the selector:

on(node, 'selector:eventType', function (event) {
    event.selectorTarget === this
});

I'd like to get feedback on this before continuing with a patch.

Change History (12)

comment:1 Changed 4 years ago by ido

@BryanForbes?: What about re-assigning event.currentTarget in case of event delegation ? In you example, with the current dojo/on event.currentTarget will be 'node'. Maybe it can be changed to be selectorTarget.

Or maybe we could override a non standard property like https://developer.mozilla.org/en-US/docs/Web/API/event.originalTarget

If a new property is preferred, I propose 'matchTarget' as internally the dojo/on function is 'on.matches'

Anyhow, I am available and would be happy to work on a patch :)

comment:2 Changed 4 years ago by Bryan Forbes

I think we should use a new property rather than overwrite a property that a browser already uses. I've seen too many cases in browsers (Chrome, IE) where overwriting properties on events causes the browser to throw errors. That being said, I prefer selectorTarget, but matchTarget is also a good name (since the node is the matching node of the selector) and I would not be opposed to it.

comment:3 Changed 4 years ago by ido

@BryanForbes? : I have created pull request : https://github.com/dojo/dojo/pull/120 I named the property "matchTarget" because my patch returns it for any event on dom object. I though it would be more consistent to have this extra property for delegation as well as for direct events. "seletorTarget" is weird in case of non delegated event.

What do you think? Should I change the pull request to add the property only for delegated event?

comment:4 Changed 4 years ago by Bryan Forbes

My preference is to only add matchTarget when there is a delegated event. The biggest reason is that even though you're adding it for most DOM events, matchTarget still won't show up in the general case (non-DOM events, events with a non-object as their first argument, etc.) and you're adding an extra function execution for every listener that goes through on() for a specialized property. I'd like to hear others weigh in on this, but it just seems like extra overhead for a property that may or may not be there anyway.

comment:5 Changed 4 years ago by ido

You're right for the extra function, and I am mitigated as well... It is there for avoiding mistakes more than for technical reason. Actually the patch I propose is similar to what we have in our dojo version since quite long time. Our first patch was adding it only in case of event delegation. But it turns out that developers were always confused... It's a known fact that non-dom events are totally different from dom event object however people were expecting to have always the same type of event object with dom element. At some point we decided to add the property for every dom events to avoid this confusion. Since then, no more defect about undefined property.

So yeah, I totally agree with you about the overhead but it feels more convenient... I am 50/50...

I hope we will have other's opinion...

comment:6 Changed 4 years ago by Kris Zyp

I would vote for the name selectorTarget, and only assigning on event delegation (and then assigning it on.selector, where it would just be a single line assignment, since you already have the reference).

comment:7 Changed 4 years ago by bill

With event delegation in native javascript, the actual target is reported in evt.target. At least according to http://davidwalsh.name/event-delegate.

Why doesn't that approach work with dojo/on?

comment:8 Changed 4 years ago by Bryan Forbes

The example that David shows is very simple: there is only text in the <li>s in the list which is why event.target is the <li>. From MDN's event.target reference:

This property of event objects is the object the event was dispatched on. It is different than event.currentTarget when the event handler is called in bubbling or capturing phase of the event.

If you add a more complex structure, David's example wouldn't work because the event is dispatched on the thing that is actually clicked on. As he says later in his article:

For example, if the A element had a SPAN tag in it, the SPAN tag would be the target element. In that case, we'd need to walk up the DOM tree to find out if it contained an A.classA element we were looking for.

You can see that here: http://jsfiddle.net/gva1mnwm/ by clicking on "click for span" and looking at the console. The extension event that on.selector produces walks up the DOM tree from event.target checking to see if any of the nodes between event.target and the node that the handler was attached to match the selector (just like David suggests).

The approach that David uses works with dojo/on and behaves exactly the same as using addEventListener, however since it's a simple example, it doesn't show the problem that this ticket is covering.

comment:9 Changed 4 years ago by bill

Ah right, makes sense. So, having a new property name sounds good to me. (I have no preference about the name.)

comment:10 Changed 4 years ago by ido

I changed the pull request according to comments. now "selectorTarget" is added only for delegated events

Last edited 4 years ago by ido (previous) (diff)

comment:11 Changed 4 years ago by dylan

Milestone: tbd1.11
Priority: undecidedhigh

comment:12 Changed 4 years ago by dylan

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.