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

BEGAN state isn't handled if taps are very quick #596

Closed
d4vidi opened this issue Feb 4, 2020 · 11 comments
Closed

BEGAN state isn't handled if taps are very quick #596

d4vidi opened this issue Feb 4, 2020 · 11 comments

Comments

@d4vidi
Copy link

d4vidi commented Feb 4, 2020

Description

Assuming even a simple JS-side reanimated-Code block that depends on both BEGAN and END states (associated with down and up press events from a tap), the BEGAN state will not be handled if the tap gesture (touch and release) is done very quickly.

In particular, we find this to be an issue with automated tests written with Detox.

For example, consider this simple usage of reanimated:

const buttonOpacity = new Value(1.0);
const gestureState = new Value(-1);

// ...
<Reanimated.Code>
  {() =>
    block([
      cond(eq(gestureState, State.BEGAN), set(buttonOpacity, 0.25)),
      cond(eq(gestureState, State.END), set(buttonOpacity, 1.0))
    ])
  }
</Reanimated.Code>

If the tap is performed really fast, the BEGAN will not be fired, and with that the opacity flicker will not take place.

Root cause

I've done some research as to what causes this and this is what I dug up.

The root of the problem - on Android, at least, is that the history of states doesn't get recorded in ValueNodes. Rather, ValueNodes only keep the most recent value set to it. Combined with the fact that these updates' handling is only triggered via the choreographer's callback - which is only dispatched async'ly in ~16ms intervals, if the gap between events is small enough (i.e. both take place fast enough to precede the next callback exec) then you end up with the BEGAN getting lost, and only the ACTIVE and END being handled.

Additional Artefacts, Reproduction

In essence, this can be reproduced by running Wix' public RN UI-Lib project's demo app, as explained by @ethanshar's comment below.

Technical Info

  • React native version: 0.61.5
  • Reanimated version: 1.7.0
@ethanshar
Copy link

ethanshar commented Feb 5, 2020

Please take a look at the attached GIF demonstrating how the button doesn't react to "soft" taps while "hard" taps work.

To reproduce this
You can checkout this code
https://github.com/wix/react-native-ui-lib/blob/demo/TouchableTapIssue/demo/src/screens/PlaygroundScreen.js

And the source code for this component can be found here
https://github.com/wix/react-native-ui-lib/blob/demo/TouchableTapIssue/src/incubator/TouchableOpacity.js#L58
TouchableOpacityIssue

@d4vidi
Copy link
Author

d4vidi commented Feb 6, 2020

Any response by the maintainers here would be appreciated :)

@d4vidi
Copy link
Author

d4vidi commented Feb 24, 2020

@kmagiera any thoughts on this?

@kmagiera
Copy link
Member

Thanks for this report and investing your time investigating this issue. I understand the root cause and wonder if you know if this has any impact outside of the domain of automated testing? That is whether it is possible for touch screens to report events with such frequency if they are touched vs events being generated by external sources (mouse clicks, automation)

@d4vidi
Copy link
Author

d4vidi commented Feb 27, 2020

@kmagiera we've tried it on high-end devices and when running our apps smoothly (high refresh-rates) the taps seem to always be registered properly. However, it is fairly easy to reproduce not just in the domain of automation, but with usage of emulators / simulators on dev computers. For example, the apple trackpad seems sensitive enough so as to allow for an easy reproduction. I imagine that also in some low-end devices, where refresh rates are low, this bug might be an issue as well.

Nevertheless automation is paramount, esp. with Detox as it is already integrated into React Native's CI itself. I don't know what tests look like there, but assuming reanimated components are bundled in at some point, they might endure flakiness on Android.

Last but not least, I can't help but feel I ought to emphasize that the root of the problem here is in the architectural core. Namely, this specific bug is most likely merely a single symptom, and it is not unlikely that similar issues exist and that are waiting to be found (with the same root cause).

@d4vidi
Copy link
Author

d4vidi commented Mar 11, 2020

Update: this seems to also happen, at times, on iOS devices as well, in automation using EarlGrey.

@rotemmiz
Copy link

@kmagiera this is also reproducible on slow devices, where frames are more easily dropped.

@d4vidi
Copy link
Author

d4vidi commented Mar 19, 2020

@kmagiera So given the situation, we're now considering submitting a fix through a PR. I tried a few naive solutions such as maintaining a values queue inside ValueNode but these attempts have been futile so far (animations and state changes go crazy 😄).

Do you have any suggestions as to how we should approach this, given the broader outlook on things?

@kmagiera
Copy link
Member

Hey, sorry for delay responding here. And apologies for not being able to focus on this right now to verify my suggestion. One thing I was interested in checking re this issue is whether the event is triggered correct number of times. As far as I understand the issue, we run some updates using <Code> block which only runs once despite the even firing multiple times. If that's the case maybe we can put some of the logic currently in <Code> directly to event handler (you can use arbitrary nodes to handle event, check second example here https://github.com/software-mansion/react-native-reanimated/blob/master/docs/pages/10.event.md)

I would consider the above as a workaround. We are currently working on rewriting core parts of reanimated and think that this problem is not going to be relevant after the change we are making. We expected the above to be ready for beta tests end of April.

@d4vidi
Copy link
Author

d4vidi commented Mar 29, 2020

@kmagiera good to hear, and thanks for responding nonetheless!
We will give the events API a try asap, and report back. However, event triggering count isn't the case here: Events received from the gesture handler via RNGestureHandlerStateChangeEvent are simply handled by the equiv. EventNode immediately, namely, outside the implicit value-change ➡️ frame-draw synchronization loop. If events with different values are reported fast enough, the intermediate value gets overlooked.

@jakub-gonet
Copy link
Member

v2 has been released recently, I think it's worth checking it out. This issue is mostly connected with the automated test system and shouldn't affect real users so I believe we can close it.

gmaclennan added a commit to digidem/mapeo-mobile that referenced this issue Sep 29, 2020
It seems like in e2e tests the detox tap() is too fast for RNGH to
register, causing button taps to fail:
software-mansion/react-native-reanimated#596
gmaclennan added a commit to digidem/mapeo-mobile that referenced this issue Sep 29, 2020
…dit (#472)

* chore: use react-native touchables in tests (RNGH = flaky tests)

It seems like in e2e tests the detox tap() is too fast for RNGH to
register, causing button taps to fail:
software-mansion/react-native-reanimated#596

* fix: Show "discard edits" not "discard observation" when cancelling edit

fixes #381

* add tests

* fix crash in test
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

5 participants