Fix touch event issue #594
Conversation
Due to the update that attaches touch events to the node diretly, Some components rely on refs. As a result, mock ref has to be added to the test. https://reactjs.org/docs/test-renderer.html#ideas
This reverts commit d1afdd5. There is already a PR resolving the same issue. So it is reverted.
| // As a workaround, touch events have to be added to directly to node | ||
| // https://github.com/facebook/react/issues/9809#issuecomment-414072263 | ||
| this.container.addEventListener('touchstart', this.handleChange) | ||
| this.container.addEventListener('touchmove', this.handleChange) |
There was a problem hiding this comment.
It seems they should be removeEventListener-ed inside componentWillUnmount or unbindEventListeners.
There was a problem hiding this comment.
@fsubal It's done. Thanks for reviewing 😄
Aren't they GCed when unmounted as they are attached to the node in the component? But I guess it is safe to remove them explicitly in componentWillUnmount.
| componentDidMount() { | ||
| // e.preventDefault() does not work properly on React synthetic touch event. | ||
| // As a workaround, touch events have to be added to directly to node | ||
| // https://github.com/facebook/react/issues/9809#issuecomment-414072263 |
There was a problem hiding this comment.
follow up here, the comment you're referencing doesn't actually show the solution.
You need to update your use to one of these:
- add a
{ passive: false }option to theaddEventListener- based on this solution touchstart preventDefault() does not prevent click event. facebook/react#9809 (comment)
- use
el.ontouchstart- based on this solution touchstart preventDefault() does not prevent click event. facebook/react#9809 (comment)
Note if you use the { passive: false } option route, you should probably do the check to see if it is supported too.
There was a problem hiding this comment.
@hartzis I have used the second option. I think the diffrence is that el.addEventListener('touchstart') is used instead of el.ontouchstart. Aren't those same? Please correct me if I'm wrong.
Thanks for following up!
There was a problem hiding this comment.
They are different. Pretty helpful explanation: https://stackoverflow.com/a/35093997
In order to gain the ability to call preventDefault you will need to add also add the { passive: false } option for chrome.
this.container.addEventListener('touchstart', this.handleChange, {passive: false})
or do what dan recomended
this.container.ontouchstart = this.handleChange
Let me know if this helps, i just stumbled across this PR since it referenced a facebook issue i was looking at and i'm dealing with a lot of the same issues as the maintainer for https://github.com/dogfessional/react-swipeable
There was a problem hiding this comment.
@hartzis Thanks for the information. After looking into this issue more, I found that ontouchstart is currently an experimental feature. https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/ontouchstart. Also in Chrome, the flag touch-event needs to be enabled to use ontouchstart API.
I'm not sure why dan used ontouchstart...
Regarding passive option, I think it does not need to be set to false unless the event handler is attached to document level e.g. window, document or body. Document level touch event handlers are treated as passive
There was a problem hiding this comment.
@cokencode That is an incredibly helpful link, thank you for sharing. I either totally missed or forgot the window, document or body part of that change. You rock!
This PR will resolve #581
e.preventDefault()is not working properly on React's synthetic touch events.As a workaround, touch event handlers are attached directly to DOM node.
facebook/react#9809 (comment)