Skip to content

Commit

Permalink
(Android/ScrollView) Fix onMomentumScrollEnd being called multiple ti…
Browse files Browse the repository at this point in the history 
…mes (#32433)

Summary:
I noticed that `onMomentumScrollEnd` is called multiple times on Android.

1. It is always called three times with the last value
2. It is sometimes called many times befire the scrolling stops when the pagingEnabled prop is true

See:
<img src="https://user-images.githubusercontent.com/17070498/137640334-301b32a7-3f59-403f-ba7e-a898666aaf3e.png" width="400"/>

I used the following code to get the logs:
```
import React from 'react';
import {SafeAreaView, ScrollView, Text, View} from 'react-native';

const App = () => {
  const onMomentumScrollEnd = ({nativeEvent}) => {
    console.log(
      'onMomentumScrollEnd',
      nativeEvent.contentOffset.x,
      nativeEvent.contentOffset.y,
    );
  };

  const onMomentumScrollBegin = ({nativeEvent}) => {
    console.log(
      'onMomentumScrollBegin',
      nativeEvent.contentOffset.x,
      nativeEvent.contentOffset.y,
    );
  };

  return (
    <SafeAreaView>
      <ScrollView
        horizontal
        pagingEnabled
        onMomentumScrollEnd={onMomentumScrollEnd}
        onMomentumScrollBegin={onMomentumScrollBegin}>
        {new Array(10).fill(0).map((_, index) => {
          return (
            <View
              style={{width: 400, height: 400, backgroundColor: 'red'}}
              key={index}>
              <Text>{index}</Text>
            </View>
          );
        })}
      </ScrollView>
    </SafeAreaView>
  );
};

export default App;

```

From what I understood:

1. We do not check that `mStableFrames` is >= 3 when emitting the event (line 798) and we keep executing the runnable, so it is emitted until `mStableFrames` equals  3. When `mStableFrames` equals 3 we stop executing the runnable (line 809). That's why it gets called 3 times.
2. When `pagingEnabled` is true, the `postOnAnimationDelayed` method is called twice (line 794 and line 806). I believe it causes the runnable to be executing too often, and the `onMomentumScrollEnd` event to be emitted too many times.

I updated the code so:

1.  The event is emitted only once, and at the same time we stop executing the runnable
2. The `postOnAnimationDelayed` method is called at most once per execution of the runnable

## Changelog

[Android] [Fixed] - Fix ScrollView's onMomentumScrollEnd being called multiple times on Android

Pull Request resolved: #32433

Test Plan: I tested using the code above with every combination of `horizontal` and `pagingEnabled` values.

Reviewed By: NickGerleman

Differential Revision: D47297163

Pulled By: ryancat

fbshipit-source-id: 7c31175d941ff13bed20dac03fb92d2b56e94dec
  • Loading branch information
@facebook-github-bot
AntoineDoubovetzky authored and facebook-github-bot committed Jul 14, 2023
1 parent 6d206a3 commit 06668fc
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 42 deletions.
36 changes: 15 additions & 21 deletions ...ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -825,7 +825,6 @@ private void handlePostTouchScrolling(int velocityX, int velocityY) {
new Runnable() {

private boolean mSnappingToPage = false;
private boolean mRunning = true;
private int mStableFrames = 0;

@Override
Expand All @@ -834,7 +833,8 @@ public void run() {
// We are still scrolling.
mActivelyScrolling = false;
mStableFrames = 0;
mRunning = true;
ViewCompat.postOnAnimationDelayed(
ReactHorizontalScrollView.this, this, ReactScrollViewHelper.MOMENTUM_DELAY);
} else {
// There has not been a scroll update since the last time this Runnable executed.
ReactScrollViewHelper.updateFabricScrollState(ReactHorizontalScrollView.this);
Expand All @@ -847,31 +847,25 @@ public void run() {
// fire before the first scroll event of an animated scroll/fling, and stop
// immediately.
mStableFrames++;
mRunning = (mStableFrames < 3);

if (mPagingEnabled && !mSnappingToPage) {
// Only if we have pagingEnabled and we have not snapped to the page do we
// need to continue checking for the scroll. And we cause that scroll by asking for
// it
mSnappingToPage = true;
flingAndSnap(0);
ViewCompat.postOnAnimationDelayed(
ReactHorizontalScrollView.this, this, ReactScrollViewHelper.MOMENTUM_DELAY);
} else {

if (mStableFrames >= 3) {
mPostTouchRunnable = null;
if (mSendMomentumEvents) {
ReactScrollViewHelper.emitScrollMomentumEndEvent(ReactHorizontalScrollView.this);
}
disableFpsListener();
} else {
if (mPagingEnabled && !mSnappingToPage) {
// If we have pagingEnabled and we have not snapped to the page
// we need to cause that scroll by asking for it
mSnappingToPage = true;
flingAndSnap(0);
}
// The scrollview has not "stabilized" so we just post to check again a frame later
ViewCompat.postOnAnimationDelayed(
ReactHorizontalScrollView.this, this, ReactScrollViewHelper.MOMENTUM_DELAY);
}
}

// We are still scrolling so we just post to check again a frame later
if (mRunning) {
ViewCompat.postOnAnimationDelayed(
ReactHorizontalScrollView.this, this, ReactScrollViewHelper.MOMENTUM_DELAY);
} else {
mPostTouchRunnable = null;
}
}
};
ViewCompat.postOnAnimationDelayed(
Expand Down
36 changes: 15 additions & 21 deletions ...ct-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,6 @@ private void handlePostTouchScrolling(int velocityX, int velocityY) {
new Runnable() {

private boolean mSnappingToPage = false;
private boolean mRunning = true;
private int mStableFrames = 0;

@Override
Expand All @@ -654,7 +653,8 @@ public void run() {
// We are still scrolling.
mActivelyScrolling = false;
mStableFrames = 0;
mRunning = true;
ViewCompat.postOnAnimationDelayed(
ReactScrollView.this, this, ReactScrollViewHelper.MOMENTUM_DELAY);
} else {
// There has not been a scroll update since the last time this Runnable executed.
ReactScrollViewHelper.updateFabricScrollState(ReactScrollView.this);
Expand All @@ -667,31 +667,25 @@ public void run() {
// fire before the first scroll event of an animated scroll/fling, and stop
// immediately.
mStableFrames++;
mRunning = (mStableFrames < 3);

if (mPagingEnabled && !mSnappingToPage) {
// Only if we have pagingEnabled and we have not snapped to the page do we
// need to continue checking for the scroll. And we cause that scroll by asking for
// it
mSnappingToPage = true;
flingAndSnap(0);
ViewCompat.postOnAnimationDelayed(
ReactScrollView.this, this, ReactScrollViewHelper.MOMENTUM_DELAY);
} else {

if (mStableFrames >= 3) {
mPostTouchRunnable = null;
if (mSendMomentumEvents) {
ReactScrollViewHelper.emitScrollMomentumEndEvent(ReactScrollView.this);
}
disableFpsListener();
} else {
if (mPagingEnabled && !mSnappingToPage) {
// If we have pagingEnabled and we have not snapped to the page
// we need to cause that scroll by asking for it
mSnappingToPage = true;
flingAndSnap(0);
}
// The scrollview has not "stabilized" so we just post to check again a frame later
ViewCompat.postOnAnimationDelayed(
ReactScrollView.this, this, ReactScrollViewHelper.MOMENTUM_DELAY);
}
}

// We are still scrolling so we just post to check again a frame later
if (mRunning) {
ViewCompat.postOnAnimationDelayed(
ReactScrollView.this, this, ReactScrollViewHelper.MOMENTUM_DELAY);
} else {
mPostTouchRunnable = null;
}
}
};
ViewCompat.postOnAnimationDelayed(
Expand Down

0 comments on commit 06668fc

Please sign in to comment.