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

[Android] Fix taps triggering while swiping #7459

Merged
merged 4 commits into from
Jan 17, 2025

Conversation

gpp-0
Copy link
Contributor

@gpp-0 gpp-0 commented Jan 15, 2025

A more proper, native-level fix for the issue described in #7312.
The root of the issue was callstack/react-native-pager-view#954, ie. a cancel event wasn't dispatched to the pager's children when it started scrolling. The patch uses notifyNativeGestureStarted to take care of that. It also disables the requestDisallowInterceptTouchEvent calls because they mess with the Drawer

@gaearon
Copy link
Collaborator

gaearon commented Jan 15, 2025

Oh nice! Mind reverting #7448 and verifying it still works with your fix? Also, do you have an upstream PR to link to?

@gaearon
Copy link
Collaborator

gaearon commented Jan 15, 2025

Note I filed callstack/react-native-pager-view#960 with another minimal repro.

@gpp-0
Copy link
Contributor Author

gpp-0 commented Jan 15, 2025

Yep, I've tested callstack/react-native-pager-view#960 and it works great, no stray taps! This branch already doesn't include #7448, do you mean I should rebase and commit a revert of #7448 here? Sorry I'm fairly new to this 😅
I'm not quite sure if I should make a PR upstream (at least not yet) because I haven't tested cases with multiple nested pagers that this class I modified is supposed to handle. Honestly I'm having trouble making sense of that class, it looks a bit dodgy and I'm not sure if it works as it's supposed to...

@gaearon
Copy link
Collaborator

gaearon commented Jan 15, 2025

do you mean I should rebase and commit a revert of #7448 here? Sorry I'm fairly new to this

Yeah, I think it would make sense to push a revert of it into this PR.

@gaearon
Copy link
Collaborator

gaearon commented Jan 15, 2025

I'm not quite sure if I should make a PR upstream (at least not yet) because I haven't tested cases with multiple nested pagers that this class I modified is supposed to handle.

Sometimes it's good to send an incomplete PR to start a discussion since the maintainers there started looking into the issue as well.

@col3name
Copy link

col3name commented Jan 16, 2025

I'm not quite sure if I should make a PR upstream (at least not yet) because I haven't tested cases with multiple nested pagers that this class I modified is supposed to handle.

Sometimes it's good to send an incomplete PR to start a discussion since the maintainers there started looking into the issue as well.

I'm not quite sure if I should make a PR upstream (at least not yet) because I haven't tested cases with multiple nested pagers that this class I modified is supposed to handle.

Sometimes it's good to send an incomplete PR to start a discussion since the maintainers there started looking into the issue as well.

Im find same behavior on ios 17.7.2, bsky 1.96.4.818

iosTapTriggeringWhileSwiping.2.mp4

@gpp-0
Copy link
Contributor Author

gpp-0 commented Jan 16, 2025

Good news, I opened a slightly modified PR upstream which doesn't break nested pagers and doesn't break the drawer and overall should make more sense :)
callstack/react-native-pager-view#961

@gaearon
Copy link
Collaborator

gaearon commented Jan 17, 2025

Fantastic work.

@gaearon

This comment was marked as resolved.

@gaearon
Copy link
Collaborator

gaearon commented Jan 17, 2025

OK, I'm also layering on 88b1878 for consistency with RN code though not sure it does anything for our case.

@gaearon gaearon merged commit 5130d19 into bluesky-social:main Jan 17, 2025
7 checks passed
@gpp-0 gpp-0 deleted the feed-swipe-v2 branch January 17, 2025 05:10
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