Skip to content
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

ResponderEventPlugin regression in React 15.4.0 #8370

Closed
necolas opened this issue Nov 21, 2016 · 8 comments
Closed

ResponderEventPlugin regression in React 15.4.0 #8370

necolas opened this issue Nov 21, 2016 · 8 comments

Comments

@necolas
Copy link
Contributor

necolas commented Nov 21, 2016

Do you want to request a feature or report a bug?

Report a bug.

What is the current behavior?

The ResponderEventPlugin no longer recognizes start DOM events.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar (template: https://jsfiddle.net/reactjs/69z2wepo/).

The EventPluginUtils.isStartish helper only checks for mouse/touch start events. In this section of code, the topLevelType used to be only topMouseDown or topTouchStart in React 15.3, but is now topClick in React 15.4.0. This means the event plugin can no longer recognize start events.

This PR exposes the issue in react-native-web, which uses the ResponderEventPlugin: necolas/react-native-web#255

What is the expected behavior?

The same behaviour as exhibited with React 15.3.2.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

Works in React 15.3.2. Doesn't work in React 15.4.0.

@sophiebits
Copy link
Collaborator

If you add a React DOM node to your tree with onMouseDown={function() {}}, does that affect the behavior?

@necolas
Copy link
Contributor Author

necolas commented Nov 23, 2016

Yes it does! It appears that all the Touchable and PanResponder examples work (mouse and touch) if the page also includes:

<div
  onMouseDown={()=>{}}
  onMouseMove={()=>{}}
  onMouseUp={()=>{}}
  onTouchStart={()=>{}}
  onTouchMove={()=>{}}
  onTouchEnd={()=>{}}
/>

@remon-nashid
Copy link

I'm wondering how can we move this forward. Any directions to help a capable developer try to submit a pull request?

@gaearon
Copy link
Collaborator

gaearon commented Dec 13, 2016

If you're looking for an intro to the event system, this might be helpful. https://www.youtube.com/watch?v=dRo_egw7tBc

@sophiebits
Copy link
Collaborator

We need to add dependencies to these event types:

It is tricky though because the plugin is shared between web and native and they have different event names. It may work to just always include the mouse events but I'm not sure if that causes problems in RN.

@necolas
Copy link
Contributor Author

necolas commented Dec 13, 2016

Which dependencies exactly? I've set mouse dependencies in RNW but that stopped working with React@15.4 - https://github.com/necolas/react-native-web/blob/f8f28980957ecb46393981d9b4bdfc820da06c26/src/modules/injectResponderEventPlugin.js

@sophiebits
Copy link
Collaborator

Oh, I would have thought that works. This code should run for your responder event handler and cause those deps to be listened to:

https://github.com/facebook/react/blob/v15.4.0/src/renderers/dom/shared/ReactDOMComponent.js#L221

@necolas
Copy link
Contributor Author

necolas commented Dec 28, 2016

The problem was that the EventConstants module no longer exports a key mirror – all the event type values are now null after this patch. Resolved the issue in RNW here: necolas/react-native-web@44ecbc0#diff-fecd5b3cfaeb4f62d70f4219e850f039

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants