New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React 15-rc.1 regression: onTouchTap not working #6221

Closed
appsforartists opened this Issue Mar 8, 2016 · 11 comments

Comments

Projects
None yet
7 participants
@appsforartists

appsforartists commented Mar 8, 2016

onTouchTap stops working in React 15. I've prepared this reduction.

The only difference between pane A and pane B is which version of React is used. If you open it on a touchscreen device (or in DevTools with emulation on), you'll see that onTouchTap worked in React 14, but ceases to work in React 15.

@appsforartists

This comment has been minimized.

Show comment
Hide comment
@appsforartists

appsforartists Mar 8, 2016

Someone proposed that onTouchTap might now be integrated. It's not

appsforartists commented Mar 8, 2016

Someone proposed that onTouchTap might now be integrated. It's not

@zpao zpao added this to the 15.0 milestone Mar 8, 2016

@saumya

This comment has been minimized.

Show comment
Hide comment
@saumya

saumya Mar 9, 2016

Finally! Thanks for the heads up.
After banging on for 2 days and trying to fix on my side, thinking everything is wrong on my setup, commented here yesterday.

I thought its MaterialUI. Will give it a try on React 14.

saumya commented Mar 9, 2016

Finally! Thanks for the heads up.
After banging on for 2 days and trying to fix on my side, thinking everything is wrong on my setup, commented here yesterday.

I thought its MaterialUI. Will give it a try on React 14.

@saumya

This comment has been minimized.

Show comment
Hide comment
@saumya

saumya Mar 9, 2016

I have now tested it with "react": "^0.14.7" and "react-dom": "^0.14.7". It works fine, without even the touch emulator.

saumya commented Mar 9, 2016

I have now tested it with "react": "^0.14.7" and "react-dom": "^0.14.7". It works fine, without even the touch emulator.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 1, 2016

Member

This is not a regression.

If you look at the history of TapEventPlugin in the React codebase you will see that it was affected by one of the recent refactorings (this is internal code and we make no guarantees about the API stability). Specifically, 4a465fb and f470cb8 changed some of the arguments that were being passed.

Since react-tap-event-plugin is effectively a fork of TapEventPlugin, it needs to incorporate upstream changes to be compatible with any next release. I sent a PR to react-tap-event-plugin that mirrors these changes. In my testing this makes it compatible with React 15 RC2, and I would expect it to also be compatible with the final 15 because that code hasn’t changed since. You can find that PR as zilverline#65.

I’m closing this but please let us know if you have any other issues. Cheers!

Member

gaearon commented Apr 1, 2016

This is not a regression.

If you look at the history of TapEventPlugin in the React codebase you will see that it was affected by one of the recent refactorings (this is internal code and we make no guarantees about the API stability). Specifically, 4a465fb and f470cb8 changed some of the arguments that were being passed.

Since react-tap-event-plugin is effectively a fork of TapEventPlugin, it needs to incorporate upstream changes to be compatible with any next release. I sent a PR to react-tap-event-plugin that mirrors these changes. In my testing this makes it compatible with React 15 RC2, and I would expect it to also be compatible with the final 15 because that code hasn’t changed since. You can find that PR as zilverline#65.

I’m closing this but please let us know if you have any other issues. Cheers!

@gaearon gaearon closed this Apr 1, 2016

@appsforartists

This comment has been minimized.

Show comment
Hide comment
@appsforartists

appsforartists Apr 1, 2016

I didn't realize that TapEventPlugin was still maintained internally.

What's the plan for unifying the two? React should support touch events out-of-the-box. If for some reason that's out-of-scope, can Facebook use/maintain the npm package, so we don't have two parallel copies of the same code?

appsforartists commented Apr 1, 2016

I didn't realize that TapEventPlugin was still maintained internally.

What's the plan for unifying the two? React should support touch events out-of-the-box. If for some reason that's out-of-scope, can Facebook use/maintain the npm package, so we don't have two parallel copies of the same code?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 1, 2016

Member

I don't think it's maintained per se. It's just that there was an internal refactoring that changed the argument order, and @soprano went through all matching call sites, changing them there, and it happened to include TapEventPlugin. We make no guarantees about it not being broken. I think we should remove it from the core source code. But yeah, this means that its maintainers will need to track changes to internal APIs. We don't have a better solution right now. Eventually we might expose event system officially as well as DOM configs but if addons taught us anything, it's that anything we expose creates a huge maintenance burden down the road which creates significant delays in development of React itself. So please excuse us that we don't have a good story around this right now. We are happy to discuss handling taps in a separate issue though so feel free to file it! Thanks.

Member

gaearon commented Apr 1, 2016

I don't think it's maintained per se. It's just that there was an internal refactoring that changed the argument order, and @soprano went through all matching call sites, changing them there, and it happened to include TapEventPlugin. We make no guarantees about it not being broken. I think we should remove it from the core source code. But yeah, this means that its maintainers will need to track changes to internal APIs. We don't have a better solution right now. Eventually we might expose event system officially as well as DOM configs but if addons taught us anything, it's that anything we expose creates a huge maintenance burden down the road which creates significant delays in development of React itself. So please excuse us that we don't have a good story around this right now. We are happy to discuss handling taps in a separate issue though so feel free to file it! Thanks.

@jimfb

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb Apr 1, 2016

Contributor

@gaearon is correct. To add to his comment, the plan is to rip it out of the core repository and have this handled in user land, as per: #436 (comment)

Contributor

jimfb commented Apr 1, 2016

@gaearon is correct. To add to his comment, the plan is to rip it out of the core repository and have this handled in user land, as per: #436 (comment)

@appsforartists

This comment has been minimized.

Show comment
Hide comment
@appsforartists

appsforartists Apr 1, 2016

Thanks guys.

For some reason, I remembered touch events not being detected without TapEventPlugin being installed. I just tested it, and that is no longer the case. Since Chrome and Safari have both removed their tap delays for mobile pages, it's probably time to stop using TapEventPlugin and just us the onClick event.

As for gestures, I'm considering building higher-order-components to detect them, and I certainly won't be quite about it if it happens. 😉

appsforartists commented Apr 1, 2016

Thanks guys.

For some reason, I remembered touch events not being detected without TapEventPlugin being installed. I just tested it, and that is no longer the case. Since Chrome and Safari have both removed their tap delays for mobile pages, it's probably time to stop using TapEventPlugin and just us the onClick event.

As for gestures, I'm considering building higher-order-components to detect them, and I certainly won't be quite about it if it happens. 😉

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 1, 2016

Member

Since Chrome and Safari have both removed their tap delays for mobile pages, it's probably time to stop using TapEventPlugin and just use the onClick event.

👍

Member

gaearon commented Apr 1, 2016

Since Chrome and Safari have both removed their tap delays for mobile pages, it's probably time to stop using TapEventPlugin and just use the onClick event.

👍

@ffxsam

This comment has been minimized.

Show comment
Hide comment
@ffxsam

ffxsam Apr 8, 2016

@appsforartists

Since Chrome and Safari have both removed their tap delays for mobile pages

But how long will it take for these changes to hit iOS Safari? The latest iOS 9.3.1 doesn't have fast tap response.

ffxsam commented Apr 8, 2016

@appsforartists

Since Chrome and Safari have both removed their tap delays for mobile pages

But how long will it take for these changes to hit iOS Safari? The latest iOS 9.3.1 doesn't have fast tap response.

@pudgereyem

This comment has been minimized.

Show comment
Hide comment
@pudgereyem

pudgereyem Sep 23, 2016

Since Chrome and Safari have both removed their tap delays for mobile pages, it's probably time to stop using TapEventPlugin and just use the onClick event.

Totally agree @appsforartists.

However, while onClick is working fine (meaning; without 350ms delay) in modern iOS Safari, it does not work with Cordova. Anyone that has encountered the same issue, and know why that is the case?

pudgereyem commented Sep 23, 2016

Since Chrome and Safari have both removed their tap delays for mobile pages, it's probably time to stop using TapEventPlugin and just use the onClick event.

Totally agree @appsforartists.

However, while onClick is working fine (meaning; without 350ms delay) in modern iOS Safari, it does not work with Cordova. Anyone that has encountered the same issue, and know why that is the case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment