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

Make ResponderEventPlugin work with DOM renderer (not in working state) #4303

Closed
wants to merge 4 commits into from

Conversation

dieppe
Copy link

@dieppe dieppe commented Jul 6, 2015

DISCLAIMER: this is my first PR to react, and I may have missed a lot, since I'm not yet fully acquainted with the internals of react, nor am I aware of the possible changes underway in the event system (or any other system, as a matter of fact 😉).
Also, this PR is not intended to be merged (at least for the time being), but is here as a base for discussion.


This is an attempt at making the ResponderEventPlugin closer to working on DOM 😃.

The problems are as follow:

  1. there are some inconsistencies between react-native and react-dom Touch events:
    • W3C events have a timeStamp property on the Event object, not on the Touch object,
    • W3C events changedTouches property is not an Array, but a TouchList.
  2. the plugins only handle touch events right now but should be able to handle mouse{Down,Move,Up} events as well,
  3. the timeStamp property is not always reliable, see Cordova app (iOS) returning from background has incorrect timestamp for touch events zilverline/react-tap-event-plugin#19.

Here are some ideas:

  • Regarding 1., we can change the way Touch* events are created on react native so that they match more closely the W3C events,
  • Regarding 2., I am not sure how to proceed here. First of, it should not be hard to make the ResponderTouchHistoryStore work with Mouse event, since one can view the Mouse event as the unique Touch object in both changedTouches and targetTouches properties.
    But we should not in any case use the same ResponderHistory for Touch and Mouse event. First because I don't see how that can make sense 😉, and second because Mouse events can be fired after a Touch event, describing the same interaction, if the Touch event was not prevented.
    Finally, I fail to see how we can make the ResponderEventPlugin truly shared between all platforms (native, browser, ...) if it needs to know about all this...
    It seems there was a plan as to how to handle the Mouse events during the react-native implementation, as per this comment (in ReactNativeEventEmitter.js):
  /**
   * [...]
   * Web desktop polyfills only need to construct a fake touch event with
   * identifier 0, also abandoning traditional click handlers.
   */
  • Regarding 3., again, I'm not sure how to proceed. The easiest way to deal with that is to use Date.now instead of the timeStamp property, but I'm unclear on the implications. Is there a way to rewrite the timeStamp property of the events if we detect we are on iOS 8 before any EventPlugin is called?

I included the PanResponder from react-native as I don't see anything preventing us from using it on desktop too. I did not modify anything, and so it crashes on Mouse events for now, but since it depends on the ResponderEventPlugin, we should first decide on what to do there.

To conclude, nothing prevents the ResponderEventPlugin to work properly for Touch and Mouse events, it's only a matter of finding the right way to do that. Since it's such a nice abstraction for event handling, I'm all 👂s 😉.

Clément Vollet added 4 commits July 6, 2015 15:04
ReactBrowserEventEmitter.listenTo loops on the dependencies array in
order to setup the right listeners for the plugin.
There is no method forEach on the changedTouches property of the touch
native events. The changeTouches property is a TouchList which is not
an Array.
This is a bad fix imo, because the problem lies more in the
discrepancies between the React Native events and the W3C events.

But at least it allows us to use the responder plugin…

Also note that using event.timeStamp can be problematic on iOS 8
webview, as seen here:
zilverline#19
This is working for touch events, but fails miserably on mouse events
(which is the expected result right now).
@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@dieppe dieppe changed the title Make ResponderEventPlugin work DOM renderer (not in working state) Make ResponderEventPlugin work with DOM renderer (not in working state) Jul 6, 2015
@jimfb
Copy link
Contributor

jimfb commented Jul 6, 2015

I'm treating this mostly as an RFC; is this something we'd want? The ResponderEventPlugin predates me.

Events: @syranide
Big PR: @spicyj @sebmarkbage

@dieppe There is a lot to consider when making changes to the core (both history and future plans) and it's often helpful to start small in order to learn. Before tackling a big PR like this, my intuition is that it's worth trying to tackle a few of these: https://github.com/facebook/react/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+bug%22

@dieppe
Copy link
Author

dieppe commented Jul 6, 2015

@jimfb Duly noted 😉, I'll have a look at those.

The thing is, I wanted to play with the ResponderEventPlugin because I intend on using it in a mobile app, and already did the work to make it usable (without mouse events, which is fine for me). After briefly talking to @sebmarkbage at the ReactConf Europe he advised me to fill in an issue, which this PR kinda is.

After that, it's really up to you guys to see if it fits with your plans at all, but I just wanted to open the discussion with a few points that seemed interesting (but may well not be 😉), since I'd really like to see the Responder abstraction working on web too.

@sebmarkbage
Copy link
Collaborator

This is a great start. I would like to move the source of truth of these files from react-native into this repo. After that we can rebase and reevaluate.

cc @jordwalke for thoughts on unification.

@sophiebits
Copy link
Contributor

This is repo the source of truth for all but these files which I didn't copy properly:

https://github.com/facebook/react-native/blob/62e8ddc20561a39c3c839ab9f83c95493df117c0/packager/blacklist.js#L17-L19

@gaearon
Copy link
Collaborator

gaearon commented Mar 26, 2016

I’m closing in favor of #6352 which is the new issue I created. Incidentally, #6338 should bring in the relevant missing dependencies, so we can look at this again.

We are learning towards using issues (perhaps in a separate RFCs repo) rather than stale PRs for discussions, so I’m closing this PR. However your work is very much appreciated, and you are welcome to contributing to discussing this in #6352, and if there is a consensus, to continue this exciting work.

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

Successfully merging this pull request may close these issues.

6 participants