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

Fix ScrollView interactions with Android's CoordinatorLayout #44099

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kkafar
Copy link
Contributor

@kkafar kkafar commented Apr 15, 2024

Summary:

Hello, I'm recently researching & implementing native material 3 bottom sheets, toolbars etc. and hitting the wall with various interactions with react-native's ScrollView. Here's one example that's documented on SO: link, but there at least few others related to gestures & dragging (undraggable bottom sheet when scrollViewContentLength < scrollViewHeight, happy to elaborate here). This comes down to the fact, that standard android.widget.ScrollView does not work well with CoordinatorLayout. Therefore I'm coming forward with question whether you would consider inheriting after androidx.core.widget.NestedScrollView which solves aforementioned issues (and mostlikely many others related to interaction with CoordinatorLayout). I want to point out that even Android docs & source code suggest to use NestedScrollView for vertical scrolling in lieu of ScrollView.

Happy to iterate here, would love some guidance especially in testing. I'm also open for implementing NestedScrollingChild / NestedScrollingParent / ... interfaces if you don't think change in inheritance chain is possible.

Changelog:

[ANDROID] [FIXED] - ReactScrollView interactions with CoordinatorLayout

Test Plan:

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Software Mansion Partner: Software Mansion Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Apr 15, 2024
@kkafar kkafar changed the title WIP Fix ScrollView interactions with Android CoordinatorLayout Fix ScrollView interactions with Android CoordinatorLayout Apr 15, 2024
@kkafar kkafar changed the title Fix ScrollView interactions with Android CoordinatorLayout Fix ScrollView interactions with Android's CoordinatorLayout Apr 15, 2024
@janicduplessis
Copy link
Contributor

I also experimented with integrating CoordinatorLayout in RN screens previously and this was a needed change to make it work.

@grahammendick
Copy link
Contributor

I got the ScrollView working with the CoordinatorLayout in the Navigation router.

If you set nestedScrollEnabled on the ScrollView then this will get you most of the way there. The only remaining problem is when the scroll view content is less than the scroll view height. I fixed this by manually dispatching nested scroll events. I put this fix in years ago so I can't be sure what the problem is, but I've a vague memory that React Native blocks the touch events from bubbling.

@javache
Copy link
Member

javache commented Apr 16, 2024

Thanks for the PR! I'd be inclined to not switch to NestedScrollView without a deeper understanding of the changes in behaviours involved.

The other suggestions made here make sense, and would be happy look at a PR that fixes the issue of missing event when scroll view content is less than the scroll view height.

@kkafar
Copy link
Contributor Author

kkafar commented Apr 16, 2024

I would love some testing guidance (maybe issues with behaviour I should put particular emphasis to test?) & more precise description of what do I need to provide you with to change this inclination (if thats feasible). Right now I haven't noticed any unusual behaviour after applying suggested changes (beside fixed interactions with CoordinatorLayout)

Here's potentially useful diff of ScrollView vs NestedScrollView implementation, but haven't got time to analyse differences yet (surely will report after I do).

As for attempts to patch the implementation w/o inheriting from NestedScrollView I'm down for trying out patching this some time next week. Will report back once I do.

@alanleedev
Copy link
Contributor

alanleedev commented Apr 25, 2024

I think argument for NestedScrollView is valid but I don't think we can judge if it is safe to swap the implementation for Android's ReactScrollView just by looking at the diff of Android's ScrollView vs NestedScrollView.
We need a way to make sure that this won't introduce any behavior changes for existing Android apps, but not sure if we have or could come up with such complete set of tests that could make this guarantee.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Software Mansion Partner: Software Mansion Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants