search

JedWatson / react-tappable

Tappable component for React
http://jedwatson.github.io/react-tappable/
MIT License
862 stars 90 forks source link

event argument is not provided to onTap consistently #39

Closed JedWatson closed 9 years ago

JedWatson commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

@KeKs0r I think this is the same issue as https://github.com/JedWatson/react-tappable/issues/46

KeKs0r commented 9 years ago

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 commented 9 years ago

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: https://github.com/slorber/react-tappable/commit/b07db97891840e8e32ac3115d44ecb3a105e609d

slorber commented 9 years ago

@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 commented 9 years ago

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

slorber commented 9 years ago

@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

https://github.com/jquery/jquery-mobile/issues/3016

JedWatson commented 9 years ago

@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 commented 9 years ago

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

slorber commented 9 years ago

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 https://github.com/JedWatson/react-tappable/pull/51

JedWatson commented 9 years ago

merged & released as v0.5.6