Fix timestamps on android touch events to use milliseconds, to be consistent with iOS #8199

mikelambert · 2016-06-17T20:27:53Z

So PanReponder.onPanResponderRelease/onPanResponderTerminate receive a gestureState object containing a onPanResponderTerminate.vx/vy property. On Android and iOS, they appear to be orders of magnitude different, which appear to be due to the different scale of timestamps that are used when generating touch events.

This pull request fixes the timestamps to be milliseconds on both platforms (since I assume iOS is the more authoritative one, and is the one that react-native-viewpager's vx thresholds written written to compare against.)

As far as I can tell, the RN code doesn't use the vx/vy properties, so they should be okay. And looks like the RN code only cares about relative values of startTimestamp/currentTimestamp/previousTimestamp though, so should be fine too. it's quite possible there will be downstream android breakage with this change, particularly for those who are already compensating for the RN discrepancy.

facebook-github-bot · 2016-06-17T20:28:04Z

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

satya164 · 2016-06-17T20:39:55Z

cc @dmmiller

dmmiller · 2016-06-18T17:06:14Z

cc @andreicoman11

There was a good reason we did the timestamps this way on Android. ccing Andrei who knows why. There was a correctness issue with the loss of precision if I recall correctly.

I think that means we will not take the request, but waiting for Andrei to confirm.

andreicoman11 · 2016-06-20T10:22:32Z

I think this is a good idea. The change to nanoTime was so that we could get rid of uptimeMillis, which is known to be unreliable. We were also hoping that this would help with some crashes related to touch events, but it didn't.
We should be consistent with iOS, but we should be using the same timestamp (probably android.os.SystemClock.elapsedRealtime) for all events (see b5d2667).

dmmiller · 2016-06-20T11:18:19Z

We call System.nanoTime() all over the place. Would it make sense to abstract this away behind a SystemClock.timestamp() method and fix up callers to use that? Then we could change all places at once. What do you think @andreicoman11

andreicoman11 · 2016-06-20T12:17:58Z

It does make sense to abstract this away, but I'm not sure if it's good to have that in SystemClock. Something like EventUtils might make more sense.

dmmiller · 2016-06-28T09:31:07Z

@mikelambert Did you want to update to make an utils class that does the timing?

…timestamp we use Summary: Use the more accurate timestamp that we have computed for the touch event rather than the event timestamp that Android provides. Reviewed By: andreicoman11 Differential Revision: D3292705 fbshipit-source-id: dad082ab74406d391481d16cdac19629751aa1eb

satya164 · 2016-07-01T00:50:06Z

Would love to get this merged. @mikelambert let me know if you need help with this.

mikelambert · 2016-07-02T03:34:07Z

I can work on this. Just to clarify what you want:

I should create a new EventUtils. What package would you like it in? Same place as SystemClock, com.facebook.react.common?
All event-related code that depends on SystemClock.nanoTime() should now depend on EventUtils.timestamp(). This is basically every one the SystemClock.nanoTime references.
Would you like me to delete SystemClock.nanoTime after this is done, or leave it in unused and tested by TimingModuleTest?

(It feels weird to create EventUtils, since an accurate millisecond counter might be useful outside of events as well, and SystemClock seems like a very convenient place to put this.)

dmmiller · 2016-07-04T08:33:32Z

@mikelambert you could add a timeForEvent on SystemClock perhaps? Then you don't have to add an extra call, and it is still associated with the SystemClock.

mikelambert · 2016-07-05T21:40:27Z

I've added a SystemClock.elapsedTime() (in a similar manner to the existing functions, let me know if you'd prefer a timeForEvent() instead). And I've called it from all the event functions.

The CI build passes, but CircleCI build fails due to:

Libraries/Animated/src/tests/bezier-test.js (some bezier numbers were within 2 precision instead of the expected 3 digits)
Libraries/Components/Touchable/tests/TouchableHighlight-test.js (some view differed from the expected view, though from what I could tell it appeared to be a re-ordered list of otherwise-identical properties)

Are these two tests expected to fail, or should I be investigating them as possibly related to my change?

dmmiller · 2016-07-06T09:52:36Z

ReactAndroid/src/main/java/com/facebook/react/modules/core/Timing.java

@@ -246,7 +246,7 @@ public void createTimer(
      return;
    }

-    long initialTargetTime = SystemClock.nanoTime() / 1000000 + adjustedDuration;
+    long initialTargetTime = SystemClock.elapsedTime() / 1000000 + adjustedDuration;


Is this correct? Didn't we know switch from nano to milli?

dmmiller · 2016-07-06T09:54:21Z

I don't understand how this compiles and works. You added SystemClock.elapsedRealtime but then called ellapsedTime everywhere.

Can you clean that up please? I'm guessing the bezier test is unrelated, but the touch one might depending on what it is checking in events.

…sistent with iOS.

mikelambert · 2016-07-06T23:41:19Z

Made the changes you requested and repushed. The rest of this is just me complaining about the testing libraries and CI builds...

You're right, I'm not sure how this originally compiled/worked at all, sorry for the sloppy attempt at a second change... :( I'm surprised the Travis CI build had passed (and the CircleCI build had 99% passed), given that it shouldn't have compiled.

I had tested locally, and got errors due to TypeError: jest.disableAutomock is not a function in some framework that I couldn't quickly solve, so decided to trust the CI builds. (They didn't show this error.)

Anyways, re-pushed, and saw the Travis CI build fail on ObjC code (despite my changelist only touching Java files). WTF?

The android tests failed due to java.lang.NoClassDefFoundError: com/facebook/jni/NativeRunnable, which didn't leave much to go on.

The TouchableHighlight test error mentioned above appears to be due to a new version of jest which my npm-init pulled, and was fixed in 65eee61 , so that's fine.

I couldn't figure out how to "re-run" the CI, so I merged to HEAD (to pull in the above-mentioned jest fix), and am re-running all the CI builds again now, hoping I get better random luck this time.

mikelambert · 2016-07-07T01:48:53Z

Having merged to head and pushed, the Travis (iOS?) and Circle (Android?) CI builds both fail again. But at least they are both consistent in failing on this error:

Skipping /private/tmp/react-native-IiZCKcFX/EndToEndTest/flow/: No such file or directory
node_modules/react/lib/React.js:0
React. Duplicate module provider
current provider. See: node_modules/react-native/Libraries/react-native/React.js:0
node_modules/react/lib/ReactNative.js:0
ReactNative. Duplicate module provider
current provider. See: node_modules/react-native/Libraries/react-native/ReactNative.js:0

(which again appears unrelated to my change, but I recognize that means nothing w.r.t. my code actually passing tests)

dmmiller · 2016-07-08T21:03:25Z

@bestander Know what might be going on with this error? Also do you know if we have any animations in UI Explorer or that @mikelambert can test with?

dmmiller · 2016-07-08T21:04:49Z

Also, @mikelambert and @bestander I am heading out of town for a week on vacation, so maybe the two of you can work to get this through. @bestander, the first time we tried it, it broke animations in Onavo's app. I didn't get a chance to check other apps since I wanted to unblock them. Do you mind trying to help @mikelambert this week to try and get those fixed and this landed? Thanks!

bestander · 2016-07-08T21:13:26Z

Sure thing, @dmmiller, I'll help out.

bestander · 2016-07-08T21:16:27Z

@mikelambert, can you try removing ReactAndroid/build folder and trying again?
We had build cache problems during 0.29 release that showed similar compile problems.
If it does not work try 0.30 branch, it is quite stable.

mikelambert · 2016-07-09T19:33:37Z

Thanks, a clean build on 0.29 works fine. Unfortunately, I see no change in behavior on "Layout Animation" example with vs without my change. :(

I can't see/test Onavo's app, so I guess there's two possibilities:

Is there anything in that app that is compensating for nanoseconds on android by checking Platform.OS?
Are there any other animation tests in UIExplorer I should check that should demonstrate this broken animation behavior? I'm not really even sure what kind of animation/setup is broken in Onavo's app...

mikelambert · 2016-07-16T20:52:00Z

Any suggestions on how to reproduce the behavior seen in "Onavo's app"? I'd love to get this fix for android timing, but don't know what next steps to take. Thanks.

bestander · 2016-07-16T21:29:29Z

Hey, Mike, sorry for the delay.
We will definitely get back to you.
Just a few things came up and no one could look at the animations issue yet

dmmiller · 2016-07-21T14:47:11Z

@mikelambert Check out da063e3 which should make it much easier to change if you want to revisit this.

mikelambert · 2016-07-21T17:37:41Z

Looks great, making for a much easier change!

I am willing to revisit it (and have even tried already), but this is not blocked on me. I have no means via which to test that it works on Onavo's app any better than it did the first time I attempted to make this change. And I feel silly submitting a change that just does the same thing, knowing that it will just get reverted for the same reasons.

dmmiller · 2016-07-22T08:23:44Z

I may try internally and see what I can find.

mikelambert · 2016-08-09T00:23:09Z

Ping, any luck internally @dmmiller ?

Also of note, the release notes for 0.31 mention my change https://github.com/facebook/react-native/releases/tag/v0.31.0 , but this change was actually reverted and is not actually in 0.31, afaict:

https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/Event.java#L42

dmmiller · 2016-08-09T09:03:42Z

I haven't had a chance to look and likely won't for a bit. If you want to try again in a new commit and just update the one location now, it might work.

…sistent with iOS Summary: So `PanReponder.onPanResponderRelease/onPanResponderTerminate` receive a `gestureState` object containing a `onPanResponderTerminate.vx/vy` property. On Android and iOS, they appear to be orders of magnitude different, which appear to be due to the different scale of timestamps that are used when generating touch events. This pull request fixes the timestamps to be milliseconds on both platforms (since I assume iOS is the more authoritative one, and is the one that `react-native-viewpager`'s vx thresholds written written to compare against.) As far as I can tell, the RN code doesn't use the `vx/vy` properties, so they should be okay. And looks like the RN code only cares about relative values of `startTimestamp/currentTimestamp/previousTimestamp` though, so should be fine too. it's quite possible there will be downstream android breakage with this change, particularly for those who are already compensating for the RN discrepancy. Closes facebook#8199 Differential Revision: D3528215 Pulled By: dmmiller fbshipit-source-id: cbd25bb7e7bb87fa77b661a057643a6ea97bc3f1

Summary: So `PanReponder.onPanResponderRelease/onPanResponderTerminate` receive a `gestureState` object containing a `onPanResponderTerminate.vx/vy` property. On Android and iOS, they appear to be orders of magnitude different, which appear to be due to the different scale of timestamps that are used when generating touch events. This pull request fixes the timestamps to be milliseconds on both platforms (since I assume iOS is the more authoritative one, and is the one that `react-native-viewpager`'s vx thresholds written written to compare against.) As far as I can tell, the RN code doesn't use the `vx/vy` properties, so they should be okay. And looks like the RN code only cares about relative values of `startTimestamp/currentTimestamp/previousTimestamp` though, so should be fine too. it's quite possible there will be downstream android breakage with this change, particularly for those who are already compensating for the RN discrepancy. Closes facebook#8199 Differential Revision: D3528215 Pulled By: davidaurelio fbshipit-source-id: d81732e50a5ece2168e8347309d8d52a0db42951

…sistent with iOS Summary: So `PanReponder.onPanResponderRelease/onPanResponderTerminate` receive a `gestureState` object containing a `onPanResponderTerminate.vx/vy` property. On Android and iOS, they appear to be orders of magnitude different, which appear to be due to the different scale of timestamps that are used when generating touch events. This pull request fixes the timestamps to be milliseconds on both platforms (since I assume iOS is the more authoritative one, and is the one that `react-native-viewpager`'s vx thresholds written written to compare against.) As far as I can tell, the RN code doesn't use the `vx/vy` properties, so they should be okay. And looks like the RN code only cares about relative values of `startTimestamp/currentTimestamp/previousTimestamp` though, so should be fine too. it's quite possible there will be downstream android breakage with this change, particularly for those who are already compensating for the RN discrepancy. Closes facebook#8199 Differential Revision: D3528215 Pulled By: dmmiller fbshipit-source-id: cbd25bb7e7bb87fa77b661a057643a6ea97bc3f1

Summary: So `PanReponder.onPanResponderRelease/onPanResponderTerminate` receive a `gestureState` object containing a `onPanResponderTerminate.vx/vy` property. On Android and iOS, they appear to be orders of magnitude different, which appear to be due to the different scale of timestamps that are used when generating touch events. This pull request fixes the timestamps to be milliseconds on both platforms (since I assume iOS is the more authoritative one, and is the one that `react-native-viewpager`'s vx thresholds written written to compare against.) As far as I can tell, the RN code doesn't use the `vx/vy` properties, so they should be okay. And looks like the RN code only cares about relative values of `startTimestamp/currentTimestamp/previousTimestamp` though, so should be fine too. it's quite possible there will be downstream android breakage with this change, particularly for those who are already compensating for the RN discrepancy. Closes facebook#8199 Differential Revision: D3528215 Pulled By: davidaurelio fbshipit-source-id: d81732e50a5ece2168e8347309d8d52a0db42951

andreicoman11 · 2016-09-06T11:05:27Z

I'm looking into shipping a new version of this internally, on top of Dave's changes. Thanks for pushing on this @mikelambert

ide · 2016-09-06T19:01:15Z

Looks like said^ commit landed in 79f3950 and will go out with 0.34.

andreicoman11 · 2016-09-07T10:45:03Z

Yes, change looks good, we didn't break anything this time :)
Thanks again @mikelambert!

…sistent with iOS Summary: So `PanReponder.onPanResponderRelease/onPanResponderTerminate` receive a `gestureState` object containing a `onPanResponderTerminate.vx/vy` property. On Android and iOS, they appear to be orders of magnitude different, which appear to be due to the different scale of timestamps that are used when generating touch events. This pull request fixes the timestamps to be milliseconds on both platforms (since I assume iOS is the more authoritative one, and is the one that `react-native-viewpager`'s vx thresholds written written to compare against.) As far as I can tell, the RN code doesn't use the `vx/vy` properties, so they should be okay. And looks like the RN code only cares about relative values of `startTimestamp/currentTimestamp/previousTimestamp` though, so should be fine too. it's quite possible there will be downstream android breakage with this change, particularly for those who are already compensating for the RN discrepancy. Closes facebook/react-native#8199 Differential Revision: D3528215 Pulled By: dmmiller fbshipit-source-id: cbd25bb7e7bb87fa77b661a057643a6ea97bc3f1

Summary: So `PanReponder.onPanResponderRelease/onPanResponderTerminate` receive a `gestureState` object containing a `onPanResponderTerminate.vx/vy` property. On Android and iOS, they appear to be orders of magnitude different, which appear to be due to the different scale of timestamps that are used when generating touch events. This pull request fixes the timestamps to be milliseconds on both platforms (since I assume iOS is the more authoritative one, and is the one that `react-native-viewpager`'s vx thresholds written written to compare against.) As far as I can tell, the RN code doesn't use the `vx/vy` properties, so they should be okay. And looks like the RN code only cares about relative values of `startTimestamp/currentTimestamp/previousTimestamp` though, so should be fine too. it's quite possible there will be downstream android breakage with this change, particularly for those who are already compensating for the RN discrepancy. Closes facebook/react-native#8199 Differential Revision: D3528215 Pulled By: davidaurelio fbshipit-source-id: d81732e50a5ece2168e8347309d8d52a0db42951