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

Scrolling a 'moving' FlatList causes it to stutter or block scrolling completely on Android #38470

Open
tjzel opened this issue Jul 17, 2023 · 21 comments
Labels
Component: FlatList Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. Needs: Attention Issues where the author has responded to feedback. Needs: Triage 🔍 Platform: Android Android applications.

Comments

@tjzel
Copy link
Contributor

tjzel commented Jul 17, 2023

Description

When you create a FlatList that moves as it's being scrolled (change in offset for the FlatList is a change in top property of FlatList's style) it stutters heavily. This behavior is much less disruptive when movement is scaled down compared to scrolling and becomes app-breaking when upscaled. See provided videos:

  1. Multiplier: 1.0
    mult1.webm
  2. Mutliplier: 2.0
    mult2.webm
  3. Multiplier: 0.5
    mult0.5.webm

It was originally an issue in react-native-reanimated but after digging into it I narrowed it down to react-native and rewrote the code to not use react-native-reanimated. It seems to had been in the code for a long time, since this posts that dates 3 years back is about the same thing.

I checked it on versions 0.72, 0.71 and 0.70. On both Fabric and Paper and the result is always the same (although it feels a bit like Fabric behaviour is worse).

iOS works properly on each version too.

It seems like there is a problem when calculating layout and touch events - that's what I conducted due to multiplier making it better or worse. After moving a FlatList twice the distance it scrolled and having the component moved React Native seems to 'think' that the finger is now further than it should be and lowers the offset? That's just my guess.

React Native Version

0.72.3

Output of npx react-native info

System:
OS: macOS 13.4.1
CPU: (10) arm64 Apple M2 Pro
Memory: 49.31 MB / 16.00 GB
Shell:
version: "5.9"
path: /bin/zsh
Binaries:
Node:
version: 18.16.0
path: /var/folders/jg/m839qn593nn7w_h3n0r9k25c0000gn/T/yarn--1689593442476-0.9098900178237601/node
Yarn:
version: 1.22.19
path: /var/folders/jg/m839qn593nn7w_h3n0r9k25c0000gn/T/yarn--1689593442476-0.9098900178237601/yarn
npm:
version: 9.5.1
path: ~/.nvm/versions/node/v18.16.0/bin/npm
Watchman:
version: 2023.06.12.00
path: /opt/homebrew/bin/watchman
Managers:
CocoaPods:
version: 1.12.1
path: /Users/user/.rbenv/shims/pod
SDKs:
iOS SDK:
Platforms:
- DriverKit 22.4
- iOS 16.4
- macOS 13.3
- tvOS 16.4
- watchOS 9.4
Android SDK: Not Found
IDEs:
Android Studio: 2022.1 AI-221.6008.13.2211.9619390
Xcode:
version: 14.3.1/14E300c
path: /usr/bin/xcodebuild
Languages:
Java:
version: 11.0.19
path: /usr/bin/javac
Ruby:
version: 3.2.2
path: /Users/user/.rbenv/shims/ruby
npmPackages:
"@react-native-community/cli": Not Found
react:
installed: 18.2.0
wanted: 18.2.0
react-native:
installed: 0.72.3
wanted: 0.72.3
react-native-macos: Not Found
npmGlobalPackages:
"react-native": Not Found
Android:
hermesEnabled: true
newArchEnabled: true
iOS:
hermesEnabled: true
newArchEnabled: false

Steps to reproduce

Just need to run the provided code snippet.

Snack, code example, screenshot, or link to a repository

https://github.com/tjzel/ReactNativeMovingFlatList

import React from 'react';
import {Dimensions, FlatList, StyleSheet, Text, View} from 'react-native';

const {height: SCREEN_HEIGHT} = Dimensions.get('screen');

const data = Array.from({length: 30}, (_, index) => ({
  id: `${index + 1}`,
  text: `Item ${index + 1}`,
}));

const multiplier = 1.0;

export default function App() {
  const [animatedVerticalScroll, setAnimatedVerticalScroll] = React.useState(0);
  // used to detect stutters
  const previousValue = React.useRef(0);

  const listAnimatedStyle = {
    top:
      animatedVerticalScroll * multiplier > SCREEN_HEIGHT * 0.7
        ? 0
        : SCREEN_HEIGHT * 0.7 - animatedVerticalScroll * multiplier,
  };

  const renderItem = ({item}) => (
    <View style={styles.item}>
      <Text>{item.text}</Text>
    </View>
  );

  const scrollHandler = event => {
    // logger to detect if there was a stutter
    if (previousValue.current > event.nativeEvent.contentOffset.y) {
      console.log(
        `\nScrolling stuttered!\nPrevious offset was ${previousValue.current}\nCurrent offset is ${event.nativeEvent.contentOffset.y}\n`,
      );
    } else {
      console.log(event.nativeEvent.contentOffset.y);
    }
    setAnimatedVerticalScroll(event.nativeEvent.contentOffset.y);
    previousValue.current = event.nativeEvent.contentOffset.y;
  };

  return (
    <View style={styles.container}>
      <View style={styles.containerBehind}>
        <Text>Multiplier: {multiplier}</Text>
      </View>
      <FlatList
        data={data}
        renderItem={renderItem}
        style={[styles.listContainer, listAnimatedStyle]}
        onScroll={scrollHandler}
      />
    </View>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
  },
  containerBehind: {
    width: '100%',
    height: '100%',
    flex: 1,
    backgroundColor: 'red',
    alignItems: 'center',
    justifyContent: 'center',
  },
  listContainer: {
    top: 0,
    left: 0,
    position: 'absolute',
    zIndex: 10,
    width: '100%',
    height: '100%',
    backgroundColor: 'green',
  },
  item: {
    height: 50,
  },
});
@cortinico
Copy link
Contributor

Could you put your snippet in this repro template @tjzel and attach it to this issue?
https://github.com/react-native-community/reproducer-react-native

@tjzel
Copy link
Contributor Author

tjzel commented Jul 17, 2023

Sure, it's added!

@github-actions github-actions bot added Needs: Attention Issues where the author has responded to feedback. and removed Needs: Author Feedback labels Jul 17, 2023
@cortinico cortinico added the Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. label Jul 17, 2023
@efstathiosntonas
Copy link

Just a sidenote, I’ve seen this happening with FlashList too.

@efstathiosntonas
Copy link

Hey folks, is this PR responsible for this?

#38475

@efstathiosntonas
Copy link

efstathiosntonas commented Jul 18, 2023

After building from source with the above PR included it's even worse @ryancat:

demo video
Screen.Recording.2023-07-18.at.10.02.03.mov

@ryancat
Copy link
Contributor

ryancat commented Jul 18, 2023

@efstathiosntonas #38475 is meant to support props that allow ScrollView to get throttled. If not used it should have no effect on ScrollView. I'll take a look to see why it's making any differences here.

@efstathiosntonas
Copy link

Thanks for the clarification @ryancat. Are there any updates on this one @cortinico?

@cortinico
Copy link
Contributor

Thanks for the clarification @ryancat. Are there any updates on this one @cortinico?

No updates. We're happy to receive a PR that fixes this (so it's up for grab)

@aureosouza
Copy link

This is blocker for us as well. Since iOS works well, it could be connected with the way Android handles fractional pixel values for animation, described here and here.

@ryancat
Copy link
Contributor

ryancat commented Jul 26, 2023

@efstathiosntonas #38475 is meant to support props that allow ScrollView to get throttled. If not used it should have no effect on ScrollView. I'll take a look to see why it's making any differences here.

VirtualizedList is defaulting scrollEventThrottle to 50 (See source), which is what FlatList using and got impacted.

I thought about dropping the default for Android, but on iOS we've been having the same default already. It's the right thing to do to align both platforms and to have a default so that we can improve native frame rate. If you feel the throttle is working against your use case, you may set a lower value (any value less than 17 should have no effect on throttling).

@NickGerleman
Copy link
Contributor

NickGerleman commented Jul 27, 2023

I think we should remove this default, even though it was previously set on iOS. It is going to be incorrect for the vast majority of use-cases. Users shouldn't need to know on an RN update that they now need to set an obscure prop to a lower value. Time-based throttling has other issues as well (and VirtualizedList already has a mechanism to avoid starvation).

@NickGerleman
Copy link
Contributor

any value less than 17 should have no effect on throttling

Separate from the throttling, we should not be adding new code to React Native which is hardcoded to assume 60fps. This is not correct on many devices, including even older or lower end Android devices.

NickGerleman added a commit to NickGerleman/react-native that referenced this issue Jul 27, 2023
Summary:
This is specific to the scenario, and device, and time-based sampling as implemented on Android may inherently create stutters because we may not proess events at a consistent cadence (and we may never process the last events).

facebook#38475 made this code no longer no-op on Android, which caused scroll regressions documented in facebook#38470

We are already coalescing scroll events on both Android and iOS, which will ensure we are not flooded with events. Though after this change, it is possible some heavy scenarios will see more time dedicated to FlatList work after this.

Changelog:
[General][Changed] - Remove default 50ms Scroll Event Throttling in VirtualizedList

Differential Revision: D47823772

fbshipit-source-id: 8e78f2349c1a8134908a2e190368b8f84ff84e06
NickGerleman added a commit to NickGerleman/react-native that referenced this issue Jul 27, 2023
…ook#38648)

Summary:
Pull Request resolved: facebook#38648

This is specific to the scenario, and device, and time-based sampling as implemented on Android may inherently create stutters because we may not proess events at a consistent cadence (and we may never process the last events).

facebook#38475 made this code no longer no-op on Android, which caused regressions documented in facebook#38470 due to VirtualizedList having more out-of-date information.

We are already coalescing scroll events on both Android and iOS, which will ensure we are not flooded with events. VirtualizedList also already inserts an artificial 50ms delay to new renders by default when high priority work is not happening (see `updateCellsBatchingPeriod`). This limits the heavy work done by VirtualizedList (no new renders or expensive math on scroll events), while letting the list still have the most recent events.

We can eventually remove this once VirtualizedList is able to use OffScreen universally.

Changelog:
[General][Changed] - Remove default 50ms Scroll Event Throttling in VirtualizedList

Differential Revision: D47823772

fbshipit-source-id: c3cbd81bdd74ade79ec050b6ef02c4d0c43f4db2
NickGerleman added a commit to NickGerleman/react-native that referenced this issue Jul 27, 2023
…ook#38648)

Summary:
Pull Request resolved: facebook#38648

This is specific to the scenario, and device, and time-based sampling as implemented on Android may inherently create stutters because we may not proess events at a consistent cadence (and we may never process the last events).

facebook#38475 made this code no longer no-op on Android, which caused regressions documented in facebook#38470 (comment) due to VirtualizedList having more out-of-date information.

We are already coalescing scroll events on both Android and iOS, which will ensure we are not flooded with events. VirtualizedList also already inserts an artificial 50ms delay to new renders by default when high priority work is not happening (see `updateCellsBatchingPeriod`). This limits the heavy work done by VirtualizedList (no new renders or expensive math on scroll events), while letting the list still have the most recent events.

We can eventually remove this once VirtualizedList is able to use OffScreen universally.

Changelog:
[General][Changed] - Remove default 50ms Scroll Event Throttling in VirtualizedList

Differential Revision: D47823772

fbshipit-source-id: 0abf312b9cf5c5f25457b58f4b8cbb393789b14d
@NickGerleman
Copy link
Contributor

NickGerleman commented Jul 28, 2023

Anecdotally, I am aware of some code which implements a similar scenario and it seems to work okay on Android. Their implementation looks like:

  1. Translating via transform, instead of top (top might cause relayout?)
  2. Uses Animated.FlatList and the Animated.event APIs. Can natively accelerate the transforms without hitting JS I think
         onScroll={Animated.event(
            [{nativeEvent: {contentOffset: {x: this._scrollViewPos}}}],
            {useNativeDriver: true},
          )}

Animated.FlatList does override scrollEventThrottle to effectively disable it, as well.

@efstathiosntonas
Copy link

@NickGerleman I removed scrollEventThrottle on VirtualizedLists and it throws this:

"@react-native/virtualized-lists": "^0.72.6"

You specified `onScroll` on a <ScrollView> but not `scrollEventThrottle`. You will only receive one event. Using `16` you get all the events but be aware that it may cause frame drops, use a bigger number if you don't need as much precision.

@tjzel
Copy link
Contributor Author

tjzel commented Jul 28, 2023

@NickGerleman Yeah there are several workarounds for this and by no means my code is made to be efficient - while using translate sounds like a smart move I think people actually want to use layout redefining prop in this scenario.

@efstathiosntonas
Copy link

efstathiosntonas commented Jul 28, 2023

translateY can be really tricky though since it “shifts” the entire list up leaving empty space at the bottom

@NickGerleman
Copy link
Contributor

@NickGerleman I removed scrollEventThrottle on VirtualizedLists and it throws this:

"@react-native/virtualized-lists": "^0.72.6"

You specified `onScroll` on a <ScrollView> but not `scrollEventThrottle`. You will only receive one event. Using `16` you get all the events but be aware that it may cause frame drops, use a bigger number if you don't need as much precision.

I ended up running into this with the above PR. Some components like Animated.FlatList currently pass scrollEventThrottle={0.0001} as a value to cause iOS to emit scroll events, while not actually throttle, so I am changing the effective default to that to keep behavior of no throttling on Android, and to change before to not throttle on iOS by default.

This specific behavior and expectation is a bit quirky, and is probably some artifact of RN back in the day being written for iOS before Android, and the two diverging. I am kind of tempted to try to run an experiment to change this behavior in the new architecture (maybe not worth the churn to change behavior in Paper).

@aureosouza
Copy link

aureosouza commented Jul 28, 2023

@NickGerleman thanks for support on this, we've tried using translateY as suggested, but we still see the stuttering in Android, as well we see issues with the footer, as not being able to scroll the whole list. Do you see any workaround for this with current RN?

Using demo, modified to:

import React from 'react';
import {Animated, Dimensions, StyleSheet, Text, View} from 'react-native';

const {height: SCREEN_HEIGHT} = Dimensions.get('screen');

const data = Array.from({length: 60}, (_, index) => ({
  id: `${index + 1}`,
  text: `Item ${index + 1}`,
}));

const App = () => {
  const scrollY = new Animated.Value(0);

  const translateY = scrollY.interpolate({
    inputRange: [0, SCREEN_HEIGHT],
    outputRange: [SCREEN_HEIGHT * 0.7, 0],
    extrapolate: 'extend',
  });

  const renderItem = ({item}) => (
    <View style={styles.item}>
      <Text>{item.text}</Text>
    </View>
  );

  return (
    <View style={styles.container}>
      <View style={styles.containerBehind}>
        <Text>Multiplier: 1.0</Text>
      </View>
      <Animated.FlatList
        data={data}
        renderItem={renderItem}
        style={[
          styles.listContainer,
          {
            transform: [{translateY}],
            paddingTop: scrollY,
          },
        ]}
        onScroll={Animated.event(
          [{nativeEvent: {contentOffset: {y: scrollY}}}],
          {useNativeDriver: false},
        )}
        ListFooterComponent={<Animated.View style={{height: scrollY}} />}
        scrollEventThrottle={16}
      />
    </View>
  );
};

const styles = StyleSheet.create({
  container: {
    flex: 1,
  },
  containerBehind: {
    width: '100%',
    height: '100%',
    flex: 1,
    backgroundColor: 'red',
    alignItems: 'center',
    justifyContent: 'center',
  },
  listContainer: {
    left: 0,
    position: 'absolute',
    zIndex: 10,
    width: '100%',
    height: '100%',
    backgroundColor: 'green',
  },
  item: {
    height: 50,
  },
});

export default App;

NickGerleman added a commit to NickGerleman/react-native that referenced this issue Jul 31, 2023
…ook#38648)

Summary:
Pull Request resolved: facebook#38648

facebook#38475 made this code no longer no-op on Android, which caused regressions documented in facebook#38470 (comment) due to VirtualizedList having more out-of-date information.

We are already coalescing scroll events on both Android and iOS, which will ensure we are not flooded with events. VirtualizedList also already inserts an artificial 50ms delay to new renders by default when high priority work is not happening (see `updateCellsBatchingPeriod`). This limits the heavy work done by VirtualizedList (no new renders or expensive math on scroll events), while letting the list still have the most recent events.

We can eventually remove this once VirtualizedList is able to use OffScreen universally.

Changelog:
[General][Changed] - Remove default 50ms Scroll Event Throttling in VirtualizedList

Reviewed By: ryancat

Differential Revision: D47823772

fbshipit-source-id: bf0befb1b9146e2173069c484735dc287b956e37
NickGerleman added a commit to NickGerleman/react-native that referenced this issue Jul 31, 2023
…ook#38648)

Summary:
Pull Request resolved: facebook#38648

facebook#38475 made this code no longer no-op on Android, which caused regressions documented in facebook#38470 (comment) due to VirtualizedList having more out-of-date information.

We are already coalescing scroll events on both Android and iOS, which will ensure we are not flooded with events. VirtualizedList also already inserts an artificial 50ms delay to new renders by default when high priority work is not happening (see `updateCellsBatchingPeriod`). This limits the heavy work done by VirtualizedList (no new renders or expensive math on scroll events), while letting the list still have the most recent events.

We can eventually remove this once VirtualizedList is able to use OffScreen universally.

Changelog:
[General][Changed] - Remove default 50ms Scroll Event Throttling in VirtualizedList

Reviewed By: ryancat

Differential Revision: D47823772

fbshipit-source-id: 5f538b7531a1907d4831fdb81ea0cda56af38bd2
NickGerleman added a commit to NickGerleman/react-native that referenced this issue Jul 31, 2023
…ook#38648)

Summary:
Pull Request resolved: facebook#38648

facebook#38475 made this code no longer no-op on Android, which caused regressions documented in facebook#38470 (comment) due to VirtualizedList having more out-of-date information.

We are already coalescing scroll events on both Android and iOS, which will ensure we are not flooded with events. VirtualizedList also already inserts an artificial 50ms delay to new renders by default when high priority work is not happening (see `updateCellsBatchingPeriod`). This limits the heavy work done by VirtualizedList (no new renders or expensive math on scroll events), while letting the list still have the most recent events.

We can eventually remove this once VirtualizedList is able to use OffScreen universally.

Changelog:
[General][Changed] - Remove default 50ms Scroll Event Throttling in VirtualizedList

Reviewed By: ryancat

Differential Revision: D47823772

fbshipit-source-id: dc6290ad02f9cbd40429270fc878d51cbb9b56b9
facebook-github-bot pushed a commit that referenced this issue Jul 31, 2023
Summary:
Pull Request resolved: #38648

#38475 made this code no longer no-op on Android, which caused regressions documented in #38470 (comment) due to VirtualizedList having more out-of-date information.

We are already coalescing scroll events on both Android and iOS, which will ensure we are not flooded with events. VirtualizedList also already inserts an artificial 50ms delay to new renders by default when high priority work is not happening (see `updateCellsBatchingPeriod`). This limits the heavy work done by VirtualizedList (no new renders or expensive math on scroll events), while letting the list still have the most recent events.

We can eventually remove this once VirtualizedList is able to use OffScreen universally.

Changelog:
[General][Changed] - Remove default 50ms Scroll Event Throttling in VirtualizedList

Reviewed By: ryancat

Differential Revision: D47823772

fbshipit-source-id: 55d22a1074235ccc1b2cf167f6b1758640c79edb
@tjzel
Copy link
Contributor Author

tjzel commented Nov 2, 2023

Hey @NickGerleman, have you had the time to look into this? I built an app with nightly RN version and it seems the issue is still there.

@guvenkaranfil
Copy link

It still persist. Is there any update @NickGerleman

@hattat-34
Copy link

I guess onScroll event has missing layout calculation, the list height is increase if another view height decrease which outside of the list(padding leads bug in this case). IOS side has no recalculation of the list height during scroll so there is no stuttering
@tjzel @NickGerleman

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: FlatList Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. Needs: Attention Issues where the author has responded to feedback. Needs: Triage 🔍 Platform: Android Android applications.
Projects
None yet
Development

No branches or pull requests

8 participants