fix: race condition between js and worklet

[kirillzyusko]

📜 Description

Fixed an issue where scrollPosition.value is updated with out-of-date value.

💡 Motivation and Context

The problem happens, because without animation onMove/onEnd dispatched almost simultaneously. But js handler can not update position.value and in onEnd handler in scrollPosition.value = position.value; code we set scrollPosition.value = 0.

When text input grows:

layout.value = input.value;
scrollPosition.value += maybeScroll(keyboardHeight.value, true);

We call maybeScroll, but scrollPosition.value === 0 and because of that we scroll by 16px (though we had to scroll for actualScroll (170) + 16 but we do 0 + 16, and because of that position is incorrect.

To fix the problem onEnd should actually update scrollPosition to the expected value, and to achieve that we need to update a value from worklet.

Now it's slightly problematic, because current REA doesn't support both: js + worklet handlers (see software-mansion/react-native-reanimated#6204), so I'm always re-dispatching nativeEvent manually via runOnJS.

The issue was caught originally in #488

📢 Changelog

JS

• fix race condition between scroll position update and keyboard movement.

🤔 How Has This Been Tested?

• Set focus to TextInput#3
• press enter 2 times

Tested on Android 12 (Pixel 2) with disabled animations (detox).

📸 Screenshots (if appropriate):

Before	After

📝 Checklist

• CI successfully passed
• I added new mocks and corresponding unit-tests if library API was changed

[github-actions]

📊 Package size report

Current size	Target Size	Difference
144688 bytes	144183 bytes	505 bytes 📈

[mobily]

@kirillzyusko hello! 👋 I was wondering if the new Reanimated hook useComposedEventHandler could be helpful in this case for merging scroll handlers together, just like this:

const internalOnScroll = useAnimatedScrollHandler(
  {
    onScroll: (event) => {
      position.value = event.contentOffset.y;
    },
  },
  [],
);

const onScroll = useComposedEventHandler(
  [internalOnScroll, onScrollProps].filter(Boolean),
);

[kirillzyusko]

@mobily I tried your code, but in this case onScroll from KeyboardAwareScrollView is not fired 🙈

And I think useComposedEventHandler gives an access only to nativeEvent, am I right?

[mobily]

in this case onScroll from KeyboardAwareScrollView is not fired 🙈

If you pass a function, that's the correct behavior. However, passing another scroll handler works fine and the event is fired. The current implementation causes crashes when passing a scroll handler because it's not supported internally by KeyboardAwareScrollView. Considering that KeyboardAwareScrollView is built on top of Animated.ScrollView (https://github.com/kirillzyusko/react-native-keyboard-controller/blob/main/src/components/KeyboardAwareScrollView/index.tsx#L343), it maybe should accept both formats.

1. onScroll → function callback

<KeyboardAwareScrollView
  onScroll={(e) => console.log('FUNCTION', e)}
  scrollEventThrottle={16}
>
  {children}
</KeyboardAwareScrollView>

CleanShot.2024-07-05.at.14.05.59.mp4

2.onScroll → scroll handler from Reanimated

const scrollHandler = useAnimatedScrollHandler({
  onScroll: (event) => {
    console.log('SCROLL HANDLER', event);
  },
});

<KeyboardAwareScrollView
  onScroll={scrollHandler}
  scrollEventThrottle={16}
>
  {children}
</KeyboardAwareScrollView>

CleanShot.2024-07-05.at.14.09.41.mp4

3. Implementation

The initial suggestion fails to consider the first point (a function callback). The final implementation might appear as:

const internalOnScroll = useAnimatedScrollHandler(
  {
    onScroll: (event) => {
      position.value = event.contentOffset.y;

      if (typeof onScrollProps === "function") {
        runOnJS(onScrollProps)({ nativeEvent: event });
      }
    },
  },
  [onScrollProps],
);

const onScroll = useComposedEventHandler([
  internalOnScroll,
  typeof onScrollProps === "object" ? onScrollProps : null,
]);

And here's the crash/error I encounter when passing a scroll handler (from Reanimated) to the onScroll prop:

CleanShot.2024-07-05.at.14.13.52.mp4

[kirillzyusko]

@mobily do I correctly understand that you want to attach your own reanimated scroll handler?

In this case probably you'll need to pass another property scrollHandler and implementation will look like:

const internalOnScroll = useAnimatedScrollHandler(
  {
    onScroll: (event) => {
      position.value = event.contentOffset.y;

      if (typeof onScrollProps === "function") {
        runOnJS(onScrollProps)({ nativeEvent: event });
      }
    },
  },
  [onScrollProps],
);

const onScroll = useComposedEventHandler([
  internalOnScroll,
 scrollHandler,
].filter(Boolean));

Another option would be to try to wrap KeyboardAwareScrollView into Reanimated.createAnimatedComponent and pass your instance of useAnimatedScrollHandler to your component returned by Reanimated.createAnimatedComponent (but I doubt it will work).

I hope I understand your intention properly, but if not - feel free to correct me and explain again what you are trying to achieve 😊

[mobily]

do I correctly understand that you want to attach your own reanimated scroll handler?

correct!

In this case probably you'll need to pass another property scrollHandler and implementation will look like

I would keep it as a single prop (onScroll) instead of having two (onScroll and scrollHandler). At least this is how it works when using Animated.ScrollView: you can pass either a function or a scroll handler to onScroll

Another option would be to try to wrap KeyboardAwareScrollView into Reanimated.createAnimatedComponent

I have tried this before, and it does work, but the performance seems slower, especially on Android devices

[mobily]

hello, @kirillzyusko 👋 are you considering adding support for scroll handlers? do you want me to create a new PR with the implementation?

[kirillzyusko]

Hey @mobily

Sorry for being non-responsive - I was thinking about your words. I think you are totally right - you as a developer should pass one handler to onScroll (reanimated or plain js handler) and it should automatically work.

I posted a proposal in reanimated - software-mansion/react-native-reanimated#6204 (comment) let's see what REA team thinks about that 👀

If they agree to support such functionality - I think we should simply wait for the new release and start to use new functionality. If they don't agree to support both types of handlers at the same time, then yes, we'll need to go for 2 props.

I think in a meantime you can create a local patch (to support two types of handlers) until we get a response from REA?