Fix ScrollView interactions with Android's CoordinatorLayout

[kkafar]

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:

[janicduplessis]

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

[grahammendick]

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]

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]

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]

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.

[ferrannp]

@alanleedev @javache currently, the nestedScrollEnabled is not working properly (if no enough content, the prop is broken). Now, you already have this in your code:

https://github.com/facebook/react-native/blob/main/packages/react-native/Libraries/Components/ScrollView/ScrollView.js#L1693C1-L1697C41

  render(): React.Node | React.Element<string> {
    const [NativeDirectionalScrollView, NativeDirectionalScrollContentView] =
      this.props.horizontal === true
        ? NativeHorizontalScrollViewTuple
        : NativeVerticalScrollViewTuple;

So you have ScrollView for vertical scroll and HorizontalScrollView for horizontal.

In Android docs, it says this for vertical scroll:

And also:

NestedScrollView is just like ScrollView, but it supports acting as both a nested scrolling parent and child on both new and old versions of Android. Nested scrolling is enabled by default.

So I think this PR is completely justified.

Another alternative could be:

      this.props.nestedScrollEnabled === true ? NativeNestedVerticalScrollView
      this.props.horizontal === true
        ? NativeHorizontalScrollViewTuple
        : NativeVerticalScrollViewTuple;

But is it worth to write another implementation (NativeNestedVerticalScrollView) if we follow Android recommendation? Anyways, both ways would work the same. But I believe we can go one way or another instead of leaving nestedScrollEnabled as it is (broken).

[grahammendick]

I refreshed my memory on this one. The Android ScrollView returns false from OnInterceptTouchEvent when the content is less than the height. So OnTouchEvent isn't called and the ScrollView doesn’t dispatch nested scroll events.

@kkafar could you check whether NestedScrollView returns true from OnInterceptTouchEvent when the content is less than the height, please?

If the NestedScrollView does return true, I think we can say that this is an Android bug with enabling nested scrolling on a regular ScrollView. Then we can go with the approach proposed by @ferrannp

[alanleedev]

Hey folks, thanks for the suggestions. The proposals do seem to make sense but as a framework we need to be bit more careful about the changes and the impact. I'm trying to have more internal discussion on this and will try to give an update next week.

[facebook-github-bot]

@alanleedev has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[alanleedev]

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.

@grahammendick @kkafar If we can fix the case for scroll view content is less than the scroll view height then would this be a sufficient fix for now before we migrate to NestedScrollView ?

[grahammendick]

@alanleedev Yep, that would work

[kkafar]

I'll be able to devote my time to do that (try to patch current scrollview) somewhere early next week, if that's ok I volunteer.

[alanleedev]

I'll be able to devote my time to do that (try to patch current scrollview) somewhere early next week, if that's ok I volunteer.

@kkafar That would be great. Thanks.

[alanleedev]

I'll be able to devote my time to do that (try to patch current scrollview) somewhere early next week, if that's ok I volunteer.

@kkafar Any updates on this?

[alanleedev]

comment from @kkafar on linked PR:

no other workaround found. There is one approach suggested by grahammendick, however yet untested.

[kkafar]

Hey, sorry (!) for a such long delay. PTO + always something higher in backlog. I do have still this on my radar, as I need to fix this behaviour to land with stable react-native-screens 4.0. I'm not declaring any specific date but it is pretty close to the top of by backlog right now.

[alanleedev]

@kkafar Thanks for the update.