event argument is not provided to onTap consistently

[JedWatson]

I've seen an error where the onTap event is not provided the event argument... it would seem that the React synthetic event is consistently passed through, but this bears review and testing.

[KeKs0r]

I just ran into the same issue. I think it has to do with events being cleaned in React.
The relevant code is:
https://github.com/JedWatson/react-tappable/blob/master/src/Tappable.js#L286
In that line the event still exists, because it has not been cleaned by react.

On the other hand in this line:
https://github.com/JedWatson/react-tappable/blob/master/src/Tappable.js#L293
(which is in a callback) the event has already been cleaned by react.

I think if we would add an event.persist() in L287, the event would persist through the callback. I don't know the exact performance impact though.

[JedWatson]

I don't know the exact performance impact though.

Just checked and it should be fine, I'll publish this update shortly. Thanks @KeKs0r!

[slorber]

@KeKs0r I think this is the same issue as #46

[KeKs0r]

It might be the same, but my PR already fixed the issue for me. Have you tried reproducing your issue with my PR #40 merged?

[slorber]

Also I would advice not using persist but instead make the onTap call always synchronous to the triggering event (touchEnd or other) and let the client call persist if he wants too.

In iOS mobile for example if you want to give focus to a text input after a tap, the keyboard only shows up if the focus has been given synchronously to the user action, so giving focus to an element with a setTimeout won't display the keyboard. This is another reason to make all the Tappable events always synchronous to the real dom event.

This PR/Commit solves the issue without persist: slorber@b07db97

[slorber]

@KeKs0r sure it does solve the issue but still I think my PR is more appropriate. See the focus example I give above, and know it's not the only problem that could happen when firing the tap event asynchronously. Often this is related to security implementations in browsers. Like opening a link in another domain in a blank target can't be done async, otherwise other websites could open a lot of new spam browser tabs when you surf on them. Some actions have to absolutly be sync or the browser won't let you do them.

Using setState(newState,fireTapEvent) like it is currently makes the event async.

[KeKs0r]

I have not given it that much thought. I am cool for your PR over mine if @JedWatson agrees.

[slorber]

@JedWatson Here's a reference to the problems I mention:

http://stackoverflow.com/questions/18885676/open-new-tab-without-popup-blocker-after-ajax-call-on-user-click

jquery-archive/jquery-mobile#3016

[JedWatson]

@slorber I'm happy to go with your PR if it solves the issue. I'll merge & publish an update now.

Thanks for your work on this!

[JedWatson]

Pubilshed as v0.5.5, this should be resolved now.

[slorber]

np

arf @JedWatson just run into an issue with my PR :( In some cases the element is active and no callback is provided so trying to execute it throws an error.

See PR to fix it here #51

[JedWatson]

merged & released as v0.5.6