[NativeAnimated][Android] Add support for animated events #9253

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
@janicduplessis
Collaborator

janicduplessis commented Aug 5, 2016

This adds support for Animated.event driven natively. This is WIP and would like feedback on how this is implemented.

At the moment, it works by providing a mapping between a view tag, an event name, an event path and an animated value when a view has a prop with a AnimatedEvent object. Then we can hook into EventDispatcher, check for events that target our view + event name and update the animated value using the event path.

For now it works with the onScroll event but it should be generic enough to work with anything.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 5, 2016

By analyzing the blame information on this pull request, we identified @kmagiera and @ericvicenti to be potential reviewers.

ghost commented Aug 5, 2016

By analyzing the blame information on this pull request, we identified @kmagiera and @ericvicenti to be potential reviewers.

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
Collaborator

janicduplessis commented Aug 5, 2016

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Aug 5, 2016

Collaborator

Other thought:

  • Should Animated Events be represented somehow as a node in the graph? I went with a separate data structure to just get it working.
  • We need to map event names properly. On Android events have different names than in JS, ie topScroll for the onScroll prop. There is a mapping of that in UIManager so it should be possible to use that to convert it properly.
  • Is there another way to attach the event to the view? Right now if the view uses a native driven event it need to be an animated view, ie: Animated.ScrollView.
Collaborator

janicduplessis commented Aug 5, 2016

Other thought:

  • Should Animated Events be represented somehow as a node in the graph? I went with a separate data structure to just get it working.
  • We need to map event names properly. On Android events have different names than in JS, ie topScroll for the onScroll prop. There is a mapping of that in UIManager so it should be possible to use that to convert it properly.
  • Is there another way to attach the event to the view? Right now if the view uses a native driven event it need to be an animated view, ie: Animated.ScrollView.

@ghost ghost added the CLA Signed label Aug 5, 2016

+ const prop = nextProps[key];
+ if (prop instanceof AnimatedEvent && prop.__isNative) {
+ // TODO: Map event names using the map in UIManager.
+ if (key === 'onScroll') {

This comment has been minimized.

@brentvatne

brentvatne Aug 6, 2016

Collaborator

Which props aside from onScroll do you think we might want to be able to bind natively to with Animated.event?

@brentvatne

brentvatne Aug 6, 2016

Collaborator

Which props aside from onScroll do you think we might want to be able to bind natively to with Animated.event?

This comment has been minimized.

@brentvatne

brentvatne Aug 6, 2016

Collaborator

Actually pan responders would be a good example of these kinds of events

@brentvatne

brentvatne Aug 6, 2016

Collaborator

Actually pan responders would be a good example of these kinds of events

This comment has been minimized.

@kmagiera

kmagiera Aug 11, 2016

Contributor

@brentvatne this approach will only work for "direct" events. That is because touch handling has a bubbling logic written in JS and we cannot determine which view is going to be responsible for handling the touch without running some JS code

@kmagiera

kmagiera Aug 11, 2016

Contributor

@brentvatne this approach will only work for "direct" events. That is because touch handling has a bubbling logic written in JS and we cannot determine which view is going to be responsible for handling the touch without running some JS code

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Aug 6, 2016

Collaborator

This is so exciting!

Should Animated Events be represented somehow as a node in the graph? I went with a separate data structure to just get it working.

Not sure about this one, will leave it to someone else to comment on.

Is there another way to attach the event to the view? Right now if the view uses a native driven event it need to be an animated view, ie: Animated.ScrollView.

Another option would be to just make ScrollView take care of attaching the event, but this doesn't work for other views and leaks the responsibility for this outside of the Animated codebase.

We need to know:

  • what event to watch for (name of the prop -> event mapping)
  • react tag for the view
  • when to detach the event

I'm sure that there are ways we could work around this but I can't think of any solid ones, so I'm for Animated.ScrollView here.

cc @vjeux @sahrens

Collaborator

brentvatne commented Aug 6, 2016

This is so exciting!

Should Animated Events be represented somehow as a node in the graph? I went with a separate data structure to just get it working.

Not sure about this one, will leave it to someone else to comment on.

Is there another way to attach the event to the view? Right now if the view uses a native driven event it need to be an animated view, ie: Animated.ScrollView.

Another option would be to just make ScrollView take care of attaching the event, but this doesn't work for other views and leaks the responsibility for this outside of the Animated codebase.

We need to know:

  • what event to watch for (name of the prop -> event mapping)
  • react tag for the view
  • when to detach the event

I'm sure that there are ways we could work around this but I can't think of any solid ones, so I'm for Animated.ScrollView here.

cc @vjeux @sahrens

@ghost ghost added the CLA Signed label Aug 6, 2016

@foghina

This comment has been minimized.

Show comment
Hide comment
@foghina

foghina Aug 11, 2016

Contributor
Contributor

foghina commented Aug 11, 2016

@ghost ghost added the CLA Signed label Aug 11, 2016

@@ -9,6 +9,8 @@
package com.facebook.react.animated;
+import android.util.Log;

This comment has been minimized.

@kmagiera

kmagiera Aug 11, 2016

Contributor

revert this

@kmagiera

kmagiera Aug 11, 2016

Contributor

revert this

mUIImplementation = uiImplementation;
+ eventDispatcher.addListener(this);

This comment has been minimized.

@kmagiera

kmagiera Aug 11, 2016

Contributor

I think we should not dispatch event to JS in the case when it is handled by the native animated driver, this would generate an unnecessary traffic over the bridge

@kmagiera

kmagiera Aug 11, 2016

Contributor

I think we should not dispatch event to JS in the case when it is handled by the native animated driver, this would generate an unnecessary traffic over the bridge

This comment has been minimized.

@janicduplessis

janicduplessis Aug 12, 2016

Collaborator

I think we should still send events to JS since some component could do some JS logic in their callback and there is also the event listener that we would still need to call into js for https://github.com/facebook/react-native/blob/master/Libraries/Animated/src/AnimatedImplementation.js#L1887

@janicduplessis

janicduplessis Aug 12, 2016

Collaborator

I think we should still send events to JS since some component could do some JS logic in their callback and there is also the event listener that we would still need to call into js for https://github.com/facebook/react-native/blob/master/Libraries/Animated/src/AnimatedImplementation.js#L1887

+
+ @Override
+ public void onEventDispatched(Event event) {
+ if (!mEventDrivers.isEmpty() && mEventDrivers.containsKey(event.getViewTag() + event.getEventName())) {

This comment has been minimized.

@kmagiera

kmagiera Aug 11, 2016

Contributor

checking emptiness is unnecessary. You can also do a get before that if which will save one hashmap lookup as well as one string allocation for concatenating tag with the name

@kmagiera

kmagiera Aug 11, 2016

Contributor

checking emptiness is unnecessary. You can also do a get before that if which will save one hashmap lookup as well as one string allocation for concatenating tag with the name

This comment has been minimized.

@janicduplessis

janicduplessis Aug 12, 2016

Collaborator

I changed this code a bit but I added the empty check to fast path the case where there is no events to avoid the string concatenation.

@janicduplessis

janicduplessis Aug 12, 2016

Collaborator

I changed this code a bit but I added the empty check to fast path the case where there is no events to avoid the string concatenation.

@ghost ghost added the CLA Signed label Aug 11, 2016

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment

@janicduplessis updated the pull request.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 11, 2016

@janicduplessis updated the pull request.

ghost commented Aug 11, 2016

@janicduplessis updated the pull request.

@ghost ghost added the CLA Signed label Aug 11, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 11, 2016

@janicduplessis updated the pull request.

ghost commented Aug 11, 2016

@janicduplessis updated the pull request.

@ghost ghost added the CLA Signed label Aug 11, 2016

+ }
+
+ _attachNativeEvents(newProps) {
+ if (newProps !== this.props) {

This comment has been minimized.

@kmagiera

kmagiera Aug 12, 2016

Contributor

This can perhaps be optimised so that we don't reattach native props every time when non relevant property gets changed. Also, I'm not sure if this statement can even evaluate to true (if props hasn't been updated componentWillReceiveProps will not be triggered)

@kmagiera

kmagiera Aug 12, 2016

Contributor

This can perhaps be optimised so that we don't reattach native props every time when non relevant property gets changed. Also, I'm not sure if this statement can even evaluate to true (if props hasn't been updated componentWillReceiveProps will not be triggered)

This comment has been minimized.

@janicduplessis

janicduplessis Aug 12, 2016

Collaborator

I think the common case is to create a new event in every render so it should be fine to reattach everytime, I'll take another look see if I can improve it though.

<ScrollView
  onScroll={Animated.event({...})
/>
@janicduplessis

janicduplessis Aug 12, 2016

Collaborator

I think the common case is to create a new event in every render so it should be fine to reattach everytime, I'll take another look see if I can improve it though.

<ScrollView
  onScroll={Animated.event({...})
/>

This comment has been minimized.

@kmagiera

kmagiera Aug 23, 2016

Contributor

We should perhaps add to the documentation that creating event in render method is not the most effective way

@kmagiera

kmagiera Aug 23, 2016

Contributor

We should perhaps add to the documentation that creating event in render method is not the most effective way

+ value.__makeNative();
+
+ invariant(
+ path.indexOf('nativeEvent') >= 0,

This comment has been minimized.

@kmagiera

kmagiera Aug 12, 2016

Contributor

I think we should expect nativeEvent to always be the first element on the path, no?

@kmagiera

kmagiera Aug 12, 2016

Contributor

I think we should expect nativeEvent to always be the first element on the path, no?

+ nativeEventPath,
+ animatedValueTag: value.__getNativeTag(),
+ });
+ return;

This comment has been minimized.

@kmagiera

kmagiera Aug 12, 2016

Contributor

can you please refactor this to if { } else { } it's difficult to notice the return statement here and this type of construct (return in relatively long if block) may lead to bugs in the future

@kmagiera

kmagiera Aug 12, 2016

Contributor

can you please refactor this to if { } else { } it's difficult to notice the return statement here and this type of construct (return in relatively long if block) may lead to bugs in the future

@janicduplessis janicduplessis changed the title from [WIP][NativeAnimated][Android] Add support for animated events to [NativeAnimated][Android] Add support for animated events Aug 12, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 12, 2016

@janicduplessis updated the pull request.

ghost commented Aug 12, 2016

@janicduplessis updated the pull request.

@ghost ghost added the CLA Signed label Aug 12, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 15, 2016

@janicduplessis updated the pull request.

ghost commented Aug 15, 2016

@janicduplessis updated the pull request.

@ghost ghost added the CLA Signed label Aug 15, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 15, 2016

@janicduplessis updated the pull request.

ghost commented Aug 15, 2016

@janicduplessis updated the pull request.

@ghost ghost added the CLA Signed label Aug 15, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 15, 2016

@janicduplessis updated the pull request.

ghost commented Aug 15, 2016

@janicduplessis updated the pull request.

@ghost ghost added the CLA Signed label Aug 15, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 15, 2016

@janicduplessis updated the pull request.

ghost commented Aug 15, 2016

@janicduplessis updated the pull request.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 15, 2016

@janicduplessis updated the pull request.

ghost commented Aug 15, 2016

@janicduplessis updated the pull request.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 15, 2016

@janicduplessis updated the pull request.

ghost commented Aug 15, 2016

@janicduplessis updated the pull request.

@ghost ghost added the CLA Signed label Aug 15, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 15, 2016

@janicduplessis updated the pull request.

ghost commented Aug 15, 2016

@janicduplessis updated the pull request.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 15, 2016

@janicduplessis updated the pull request.

ghost commented Aug 15, 2016

@janicduplessis updated the pull request.

@ghost ghost added the CLA Signed label Aug 15, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 15, 2016

@janicduplessis updated the pull request.

ghost commented Aug 15, 2016

@janicduplessis updated the pull request.

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Sep 9, 2016

Collaborator

Rebased to fix conflict, @bestander let me know if there is anything I can do to help.

Collaborator

janicduplessis commented Sep 9, 2016

Rebased to fix conflict, @bestander let me know if there is anything I can do to help.

@ghost ghost added the CLA Signed label Sep 9, 2016

@bestander

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander Sep 9, 2016

Contributor

Thanks, @janicduplessis, I posted this in the group https://www.facebook.com/groups/reactnativeoss/permalink/1588313934798568/?comment_id=1588884458074849&comment_tracking=%7B%22tn%22%3A%22R%22%7D

TL;DR: there is some weird test infra issue that blocks the merge.
It is not related to the code, I just don't want to disable tests and would rather fix it.
Planning to poke the people with more context and land it next week

Contributor

bestander commented Sep 9, 2016

Thanks, @janicduplessis, I posted this in the group https://www.facebook.com/groups/reactnativeoss/permalink/1588313934798568/?comment_id=1588884458074849&comment_tracking=%7B%22tn%22%3A%22R%22%7D

TL;DR: there is some weird test infra issue that blocks the merge.
It is not related to the code, I just don't want to disable tests and would rather fix it.
Planning to poke the people with more context and land it next week

@ghost ghost added the CLA Signed label Sep 9, 2016

+ value.__makeNative();
+
+ eventMappings.push({
+ nativeEventPath: path,

This comment has been minimized.

@eslint-bot

eslint-bot Sep 9, 2016

access of computed property/element Computed property/element cannot be accessed on property nativeEvent of unknown type

@eslint-bot

eslint-bot Sep 9, 2016

access of computed property/element Computed property/element cannot be accessed on property nativeEvent of unknown type

+ if (value instanceof AnimatedValue) {
+ value.__makeNative();
+
+ eventMappings.push({

This comment has been minimized.

@eslint-bot

eslint-bot Sep 9, 2016

property nativeEvent of unknown type This type is incompatible with iteration expected on object

@eslint-bot

eslint-bot Sep 9, 2016

property nativeEvent of unknown type This type is incompatible with iteration expected on object

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment

@ghost ghost added the CLA Signed label Sep 9, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

This comment has been minimized.

Show comment
Hide comment

@ghost ghost added the CLA Signed label Sep 10, 2016

@foghina

This comment has been minimized.

Show comment
Hide comment
Contributor

foghina commented Sep 16, 2016

@foghina

This comment has been minimized.

Show comment
Hide comment
@foghina

foghina Sep 16, 2016

Contributor

Just importing it again, see if I can fix the test somehow.

Contributor

foghina commented Sep 16, 2016

Just importing it again, see if I can fix the test somehow.

@ghost ghost added the Import Started label Sep 16, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 16, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

ghost commented Sep 16, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

@ghost ghost added the CLA Signed label Sep 16, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 16, 2016

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

ghost commented Sep 16, 2016

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

@ghost ghost added CLA Signed and removed Import Started labels Sep 16, 2016

@nihgwu

This comment has been minimized.

Show comment
Hide comment
@nihgwu

nihgwu Sep 18, 2016

Contributor

@janicduplessis how can I use this in a ListView, I've just tried but failed with warning Invalid prop 'onScroll' of type 'object' supplied to 'ListView', expected 'function'

Contributor

nihgwu commented Sep 18, 2016

@janicduplessis how can I use this in a ListView, I've just tried but failed with warning Invalid prop 'onScroll' of type 'object' supplied to 'ListView', expected 'function'

@ghost ghost added the CLA Signed label Sep 18, 2016

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Sep 18, 2016

Collaborator

To use it with ListView you'll need to create an AnimatedListView using Animated.createAnimatedComponent.

Collaborator

janicduplessis commented Sep 18, 2016

To use it with ListView you'll need to create an AnimatedListView using Animated.createAnimatedComponent.

@nihgwu

This comment has been minimized.

Show comment
Hide comment
@nihgwu

nihgwu Sep 18, 2016

Contributor

I did that but still failed, have you tried it?

???? iPhone

? 2016?9?19??01:25?Janic Duplessis <notifications@github.commailto:notifications@github.com> ???

To use it with ListView you'll need to create an AnimatedListView using Animated.createAnimatedComponent.

You are receiving this because you commented.
Reply to this email directly, view it on GitHubhttps://github.com/facebook/react-native/pull/9253#issuecomment-247860836, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ACeY8kSDrw2975LYMGSiQ0_dhMyuQrGzks5qrXP2gaJpZM4JeI2-.

Contributor

nihgwu commented Sep 18, 2016

I did that but still failed, have you tried it?

???? iPhone

? 2016?9?19??01:25?Janic Duplessis <notifications@github.commailto:notifications@github.com> ???

To use it with ListView you'll need to create an AnimatedListView using Animated.createAnimatedComponent.

You are receiving this because you commented.
Reply to this email directly, view it on GitHubhttps://github.com/facebook/react-native/pull/9253#issuecomment-247860836, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ACeY8kSDrw2975LYMGSiQ0_dhMyuQrGzks5qrXP2gaJpZM4JeI2-.

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Sep 18, 2016

Collaborator

Ok, I think I know why. Does it work if you add

  getScrollableNode: function() {
    if (this._scrollComponent && this._scrollComponent.getScrollableNode) {
      return this._scrollComponent.getScrollableNode();
    }
  },

in ListView.js

Collaborator

janicduplessis commented Sep 18, 2016

Ok, I think I know why. Does it work if you add

  getScrollableNode: function() {
    if (this._scrollComponent && this._scrollComponent.getScrollableNode) {
      return this._scrollComponent.getScrollableNode();
    }
  },

in ListView.js

@nihgwu

This comment has been minimized.

Show comment
Hide comment
@nihgwu

nihgwu Sep 19, 2016

Contributor

@janicduplessis It's definitely awesome, working so great, say goodbye to bad scrolling effects, thank you again, wish this can be merged soon

Contributor

nihgwu commented Sep 19, 2016

@janicduplessis It's definitely awesome, working so great, say goodbye to bad scrolling effects, thank you again, wish this can be merged soon

@ghost ghost added the CLA Signed label Sep 19, 2016

@ghost ghost closed this in 6565929 Sep 19, 2016

@foghina

This comment has been minimized.

Show comment
Hide comment
@foghina

foghina Sep 19, 2016

Contributor

BOOM.

Contributor

foghina commented Sep 19, 2016

BOOM.

@nihgwu

This comment has been minimized.

Show comment
Hide comment
@nihgwu

nihgwu Sep 19, 2016

Contributor

@janicduplessis what about to add support for ListView

Contributor

nihgwu commented Sep 19, 2016

@janicduplessis what about to add support for ListView

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Sep 19, 2016

Collaborator

I'll make another PR for the ListView fix.

Collaborator

janicduplessis commented Sep 19, 2016

I'll make another PR for the ListView fix.

rozele pushed a commit to Microsoft/react-native-windows that referenced this pull request Sep 27, 2016

Add support for animated events
Summary:
This adds support for `Animated.event` driven natively. This is WIP and would like feedback on how this is implemented.

At the moment, it works by providing a mapping between a view tag, an event name, an event path and an animated value when a view has a prop with a `AnimatedEvent` object. Then we can hook into `EventDispatcher`, check for events that target our view + event name and update the animated value using the event path.

For now it works with the onScroll event but it should be generic enough to work with anything.
Closes facebook/react-native#9253

Differential Revision: D3759844

Pulled By: foghina

fbshipit-source-id: 86989c705847955bd65e6cf5a7d572ec7ccd3eb4

nihgwu added a commit to nihgwu/react-native that referenced this pull request Dec 7, 2016

facebook-github-bot pushed a commit that referenced this pull request Dec 14, 2016

fix ListView to work with Native Animated.event
Summary:
suggested by janicduplessis in #9253 (comment)
I'm using this in my own apps
Closes #11339

Differential Revision: D4325343

fbshipit-source-id: f1da26a2107093865f04e1d81245b33482776001

DanielMSchmidt added a commit to DanielMSchmidt/react-native that referenced this pull request Jan 4, 2017

fix ListView to work with Native Animated.event
Summary:
suggested by janicduplessis in facebook#9253 (comment)
I'm using this in my own apps
Closes facebook#11339

Differential Revision: D4325343

fbshipit-source-id: f1da26a2107093865f04e1d81245b33482776001

This issue was closed.

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