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

Cancel failed buttons in reset #3350

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

oblador
Copy link
Contributor

@oblador oblador commented Jan 21, 2025

Description

This PR is an attempt at fixing bug reported in #2160.

Essentially when having a button with shouldCancelWhenOutside={true} in a ScrollView and scrolling quickly, especially when at the bottom where the scrollview exhibit rubber band effect, all buttons becomes unresponsive. This is because touchResponder is never freed after it goes into FAILED state, which typically otherwise happens when ACTION_UP is emitted – something that never happens.

I'm not sure of the ideal solution, but I'd figured I share my half-baked workaround to emit CANCEL in onReset if the button is stuck in FAILED. Curious to hear suggestions on how to improve this.

Test plan

@latekvo
Copy link
Contributor

latekvo commented Jan 22, 2025

Hey, thank you for the PR! Could you please provide a repro where #2160 still occurs?
It seems like it should've been fixed by #2187, and I'm not aware of any regressions of that bug.

I tried reproducing this issue with RectButton with shouldCancelWhenOutside={true} in a ScrollView using the steps you provided in this PR, but the issue did not occur despite my best attempts.

@oblador
Copy link
Contributor Author

oblador commented Jan 22, 2025

Hi I made a minimal reproduction here: https://snack.expo.dev/@oblador/gesture-handler-list-reproduction

Note that this is Android only, see a video of the reproduction where you can notice that before I scroll the first list item responds with a highlight, but afterwards none do:

screen-20250122-131111.mp4

@oblador
Copy link
Contributor Author

oblador commented Jan 22, 2025

For what it's worth, I've managed to get into this state even without shouldCancelWhenOutside={true}, but it's just much easier/frequent in this mode. Basically it seems to apply to any case where the handler enter failed state. It's also possible to "resurrect" the app if you know which button caused it to die by scrolling slowly on that specific button, which is in line with above explanation because then ACTION_UP is emitted.

@RohovDmytro
Copy link

RohovDmytro commented Jan 25, 2025

I also rendomly experience when in a ScrollView there are two buttons and the second one ( (it's FAB) overlaps the first one.

Sometimes pressing a FAB buttons also triggers the underlying button (the ripple animation starts). FAB triggers navigation. Then the app becomes unresponsive to onPress events. Navigating back I can see that ripple effect got stuck (never finished) on a first button.

This PR might help with that too.

Copy link
Contributor

@latekvo latekvo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I tested your solution on your repro, and I still managed to get the RectButtons stuck in the active state.

It looks like this issue is caused by NativeViewGestureHandler either never leaving the ACTIVE state, or being removed from gestureHandlers array before being reset or finished.

When i managed to get RectButton stuck, onCancel, onFail and onReset were never called, suggesting NativeViewGestureHandler was never finished in the first place (onFail doesn't exist on main, added it for debugging).

I've also tried different approaches, but haven't solved this issue yet.
I'll dig into this deeper, and keep you updated when I find any solution.

@oblador
Copy link
Contributor Author

oblador commented Jan 31, 2025

@RohovDmytro I think your issue is a different one from this, but I came across it today as well, and created a separate issue for it here: #3370

This PR doesn't solve that bug however.

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

Successfully merging this pull request may close these issues.

3 participants