fn.on delegation misbehaviour

[netfusion]

In #188 this was fixed but I noticed that the value of event.currentTarget is different from jQuery.
You can see the current behaviour here.
event.currentTarget returns always the outer div even when you click the inner.

The expected behaviour is this.
When you click the inner div event.currentTarget should return the inner div.

It also looks like cash triggers both events (on the outer and inner div) whereas jQuery only triggers the second one.

[fabiospampinato]

I see 2 problems here:

• event.currentTarget doesn't point to the right element when using event delegation.

• return false doesn't seem to be respected.

Thanks for the bug report, I'll try to get this fixed in a timely manner.

[fabiospampinato]

The issue with return false is interesting. Technically it seems that we are doing the right thing, you are attaching 2 handlers to the same element and the first handler calls stopPropagation, so the event isn't propagated up the tree, but it will still trigger the other handler since it was attached to the same element.

When using event delegation in jQuery, ie. $('foo').on ( 'click', 'bar', () => {} ), even though they are also attaching the actual event handler to foo they treat this as if they were attaching it to bar.

Luckily it seems that we can use event.cancelBubble to detect when event.stopPropagation is called. jQuery instead is kind of rewriting the entire event system, and they use their own isPropagationStopped method.

It might be good to mention that we don't support things like isDefaultPrevented or isPropagationStopped in the migration guide 👍.

[fabiospampinato]

I'm afraid both problems can't be fixed, not without any major drawbacks. I've added a section to the migration guide about this, you might want to check that out: https://github.com/kenwheeler/cash/blob/master/docs/migration_guide.md/#events

Here there's an explanation why they can't be fixed without major drawbacks:

event.currentTarget doesn't point to the right element when using event delegation.

event.currentTarget is not writable, so:

event.currentTarget = 123;
event.currentTarget === 123; // => false

Possibile solutions that come to mind:

1. Wrap all events in custom event objects, this is what jQuery is doing. It would require a considerable amount of bytes and slow down pretty much everything related to event handling.

2. Wrap events in a Proxy. It would also slow down things considerably, besides Proxy is not available on IE10.

Both possible solutions are unaccetable, so just use this instead.

return false doesn't seem to be respected.

We are listening for events on the target element, not the delegate elements. That's basically the whole point of event delegation. So when you call stopPropagation from a delegate handler we can't stop other events attached to the target from executing too.

Possibile solutions that come to mind:

1. Rewrite the event system from scratch, so that we have more control over it, basically what jQuery is doing. This alone would probably require more bytes that the entirety of Cash today.

2. When a delegated event handler is attached we could remove all the previous non-delegate handlers and reattach them, so that our delegate handlers are executed first. Then we could modify the event object and add some extra logic to basically handle this use case.

Both possible solutions are unaccetable, and there isn't a workaround that works for every situation, in many cases you could just use event.stopImmediatePropagation to achieve the same effect though.

I'm closing this issue as it doesn't look like there's a way for us to fix it that doesn't come with huge downsides.

[netfusion]

Thanks for the quick response.

Using this instead of event.currentTarget does not work if you are using ES6 arrow functions. I use them inside class methods and would highly prefer to keep this returning my current class instance.

Until the best solution is determined it may be good to use an additional property on event as a temporary workaround.

I noticed another problem during testing today. Some events (e.g. mouseenter) do not trigger when delegation is used. This makes sense because technically the actual target does not get entered (its a child element which does).

[fabiospampinato]

I don't want to add custom properties that don't have a jQuery counterpart to events. I feel your pain, and you can already do that yourself by patching cash's on method, something like this (I haven't actually tested this code):

const onPrev = $.fn.on;
$.fn.on = function on ( event, selector, callback, _one ) {
  if ( $.isString ( eventFullName ) ) {
    if ( isFunction ( selector ) ) {
      callback = selector;
      selector = '';
    }
    const callbackPrev = callback;
    callback = function callback ( event, data ) {
      event.myCustomProperty = this;
      return callbackPrev.call ( this, event, data );
    }
  }
  return onPrev.call ( this, event, selector, callback, _one );
};

I noticed another problem during testing today. Some events (e.g. mouseenter) do not trigger when delegation is used. This makes sense because technically the actual target does not get entered (its a child element which does).

Can you make a separate issue about this?

[fabiospampinato]

Reopening as these issues might be fixable without any major drawbacks.

event.currentTarget doesn't point to the right element when using event delegation.

We only have to clone the event object when using event delegation. Hopefully cloning it would be fast and all those defaultPrevented etc. properties won't be too much of an hassle to work with.

return false doesn't seem to be respected.

We can call stopImmediatePropagation instead of just stopPropagation on the last delegate handler. The issue here is that if somebody instead of doing return false does event.stopPropagation () we can't inject our custom logic, unless we override the default stopPropagation method.

Basically in order to fix both of these issues we need to have writable events.

[fabiospampinato]

event.currentTarget doesn't point to the right element when using event delegation.

Fixed in 7d0884f.

Object.create saved the day, we are now using it for extending the non-writable event object, it's fast and elegant:

event = Object.create ( event, { currentTarget: { value: currentTarget } } );

[fabiospampinato]

event = Object.create ( event, { currentTarget: { value: currentTarget } } );

It seems that there are some unexpected errors when using this 🤔 I'll investigate this further tomorrow.

[fabiospampinato]

Since part of this issue has been resolved I'm closing this in favor of #238.