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 maintainVisibleContentPosition on Android during momentum scroll #43425

Closed
wants to merge 3 commits into from

Conversation

janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented Mar 12, 2024

Summary:

When using maintainVisibleContentPosition (mvcp) on Android it doesn't work properly when items are added during a momentum scroll. This happens because the android scrollview momentum scroll animation overrides the scroll position that the mvcp implementation sets here.

To fix this we need to cancel the momentum scroll animation, update the scroll position then restart the scroll animation with the previous animation remaining momentum.

Changelog:

[ANDROID] [FIXED] - Fix maintainVisibleContentPosition during momentum scroll

Test Plan:

Tested in RNTester on Android with both vertical and horizontal scrollviews using the following code:

// packages/rn-tester/js/RNTesterAppShared.js

import {
  Button,
  SafeAreaView,
  ScrollView,
  Text,
  View,
} from 'react-native';
import React, {useLayoutEffect, useRef, useState} from 'react';

const generateUniqueKey = () => `_${Math.random().toString(36).substr(2, 9)}`

const initialData = Array.from(Array(100).keys()).map(n => ({
  id: generateUniqueKey(),
  value: n,
}))

function ListItem({item}) {
  const color = `hsl(${item.value * 10}, 75%, 85%)`;

  return (
    <View
      style={{
        backgroundColor: color,
        height: 100,
      }}>
      <Text>List item: {item.value}</Text>
    </View>
  );
}

export default function FlatListRepro() {
  const numToAdd = 10;
  const [numbers, setNumbers] = useState(initialData);
  const ref = useRef();

  const addAbove = () => {
    setNumbers(prev => {
      const additionalNumbers = Array.from(Array(numToAdd).keys())
        .map(n => ({
          id: generateUniqueKey(),
          value: prev[0].value - n - 1,
        }))
        .reverse();

      return additionalNumbers.concat(prev);
    });
  };

  useLayoutEffect(() => {
    ref.current.scrollTo({y: numbers.length * 100, animated: false});
  // eslint-disable-next-line react-hooks/exhaustive-deps
  }, []);

  return (
    <SafeAreaView style={{flex: 1}}>
      <View style={{flexDirection: 'row', alignItems: 'center'}}>
        <Button title="Add Above" onPress={addAbove} />
      </View>
      <View>
        <ScrollView
          ref={ref}
          maintainVisibleContentPosition={{
            minIndexForVisible: 0,
          }}
        >
          {numbers.map((item) => (
            <ListItem key={item.id} item={item} />
          ))}
        </ScrollView>
      </View>
    </SafeAreaView>
  )
}

Before:

Screen.Recording.2024-03-11.at.23.03.27.mov

After:

Screen.Recording.2024-03-11.at.22.42.16.mov

@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. Contributor A React Native contributor. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Mar 12, 2024
@analysis-bot
Copy link

analysis-bot commented Mar 12, 2024

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 18,016,606 -57,269
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 21,371,041 -69,580
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 6c50418
Branch: main

@janicduplessis
Copy link
Contributor Author

@NickGerleman I updated it to use the same logic from HorizontalScrollView (and added it to ScrollView).

Comment on lines 1422 to 1427
// If we have any pending custon flings (e.g. from aninmated `scrollTo`, or flinging to a snap
// point), finish them, commiting the final `scrollX`.
// TODO: Can we be more graceful (like OverScroller flings)?
if (getFlingAnimator().isRunning()) {
getFlingAnimator().end();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this piece still relevant? I.e. if adjustment happens during programmatically triggered fling?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be pretty rare cases, but I added the extra logic just in case.

There I used cancel instead of end since I do not want to commit the last scroll position of the animation. I think it is better to cancel the scroll still than to have content jumps because of the animation being wrong.

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Mar 15, 2024
@facebook-github-bot
Copy link
Contributor

@NickGerleman merged this pull request in a9e6759.

@janicduplessis janicduplessis deleted the @janic/mvcp-test branch March 15, 2024 07:06
@darkbasic
Copy link

Wow that was so quick compared to how long it took for the original issue to be fixed!

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. Contributor A React Native contributor. Merged This PR has been merged. 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.

5 participants