alexgibson / tap.js

DEPRECATED - A custom 'tap' event JavaScript plugin for touch devices
Other
349 stars 65 forks source link

Event Called Multiple Times on Target #19

Closed joshuabambrick closed 10 years ago

joshuabambrick commented 10 years ago

This is a similar, but (as far as I can tell) unrelated issue to that I posted about before in #18. When I bind separate handlers to child and parent elements, the child element's handlers appear to get triggered twice when using a mouse click.

I am not familiar with custom events, but looking at line blob/master/tap.js#L64, I think calling dispatchEvent on e.target is potentially incorrect, or at least you need to communicate that this is not necessarily the element to which the event was bound.

I am not convinced that simply replacing e.target with this.element works, at least I seem to have had mixed results when using jquery's selector param.

alexgibson commented 10 years ago

but looking at line blob/master/tap.js#L64, I think calling dispatchEvent on e.target is potentially incorrect

Sorry if I'm missing what you're saying here, but can you please outline what you think the correct behavior should be instead. What else would it get called on if not what was tapped?

I'm also not quite clear on why you would want to bind tap event handlers to both a parent and child node. Why not just bind a single tap event on the parent and use event delegation to find which child was tapped?

joshuabambrick commented 10 years ago

Well, if you bind a handler to the body element, and then a separate one to a div inside it, you don't expect the div's event handler to get called twice when you click it. However, since the div is the target of both events, tap.js calls dispatchEvent on it twice - once for each handler.

Also, whilst it's not really relevant, you might bind two handlers if the two pieces of code were independent components which are not aware of each other. In my specific example, the two components are both Backbone views, with events attached using the events parameter.

alexgibson commented 10 years ago

Well, if you bind a handler to the body element, and then a separate one to a div inside it, you don't expect the div's event handler to get called twice when you click it.

As per the previous issue you opened, is this not fixed just by calling e.preventDefault(); in your event handler?

joshuabambrick commented 10 years ago

Sorry, I realise that I forgot to include the word 'twice' in my original post.

joshuabambrick commented 10 years ago

Unfortunately not, calling e.preventDefault did not fix the issue.

alexgibson commented 10 years ago

Ah ok, I'll see if I can build a test case to look at the issue. Fwiw, I still think this is an edge case and binding tap events to both a parent and child node is more a pattern you should be avoiding.

joshuabambrick commented 10 years ago

Might be worth mentioning that I am using the jquery plugin, but I am not sure that the issue is limited to that.

joshuabambrick commented 10 years ago

I think the issue might be solvable by using this.element, as I mentioned, and then defining add in your special jquery event so that you can do something with the selector property (see http://benalman.com/news/2010/03/jquery-special-events/).

alexgibson commented 10 years ago

We can't replace e.target with this.element when dispatching the event, as this would prevent things like event delegation from working (which is what you should really be using instead of adding tap events to both parent and child nodes).

If you can provide a reduced test case and identify if this is an issue when only using the jQuery plugin, that would be very helpful.

joshuabambrick commented 10 years ago

You're right, you have to call dispatchEvent on e.target. I think the issue is that you end up dispatching multiple events which can actually be in response to the same pointer event. This means that it is not limited to the jquery plugin.

Here is a jsfiddle of the issue: http://jsfiddle.net/sAuhsoj/nkf8vw4b/

alexgibson commented 10 years ago

Can you please tell me if adding e.stopPropagation(); immediately after Line 66 fixes your issue?

joshuabambrick commented 10 years ago

I think it does if the user calls, event.preventDefault

alexgibson commented 10 years ago

Great, this should fix the issue here. I will see to adding this in a commit, thanks for filing!

alexgibson commented 10 years ago

Actually, you're right - I think it is safe to call this even if preventDefault() is not used in the tap event handler.

alexgibson commented 10 years ago

What you do also still need to do, is make sure you still call e.stopPropagation(); in your tap event handler as well, else the child tap event will bubble up to the parent (just as any native or custom event should do).