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

An ability to have JS handler along with animated handler simultaneously #6204

Open
kirillzyusko opened this issue Jul 3, 2024 · 16 comments · May be fixed by #6268
Open

An ability to have JS handler along with animated handler simultaneously #6204

kirillzyusko opened this issue Jul 3, 2024 · 16 comments · May be fixed by #6268
Assignees
Labels
Feature request Platform: Android This issue is specific to Android Platform: iOS This issue is specific to iOS Platform: Web This issue is specific to web

Comments

@kirillzyusko
Copy link
Contributor

kirillzyusko commented Jul 3, 2024

Description

I would like to have an ability to have 2 handlers (JS + reanimated) at the same time. Before I could achieve that using next code:

const CustomScrollView = ({onScroll: onScrollFromProps}) => {
  const onScroll = useAnimatedScrollHandler({
      onScroll: (event) => {
  }});

  return <Reanimated.ScrollView onScrollReanimated={onScroll} onScroll={onScrollFromProps} />
};

But at some point of time this construction stopped to work. I think it happens because of this:

if (value.workletEventHandler.eventNames.length > 0) {
value.workletEventHandler.eventNames.forEach((eventName) => {
props[eventName] = has('listeners', value.workletEventHandler)
? (
value.workletEventHandler.listeners as Record<string, unknown>
)[eventName]
: dummyListener;
});

Another option was to add such code:

const CustomScrollView = ({onScroll: onScrollFromProps}) => {
  const onScroll = useAnimatedScrollHandler({
      onScroll: (event) => {
      
      runOnJS(onScrollFromProps)({nativeEvent: event});
  }});

But in this case some of properties (currentTarget, cancellable, etc.) will be ignored.

Is there a recommended way to achieve this? 👀

Steps to reproduce

Add the code from description.

Snack or a link to a repository

N/A

Reanimated version

3.12.1

React Native version

0.74.2

Platforms

Android, iOS

JavaScript runtime

Hermes

Workflow

React Native

Architecture

Paper (Old Architecture)

Build type

Debug app & dev bundle

Device

iOS simulator

Device model

iPhone 15

Acknowledgements

Yes

Copy link

github-actions bot commented Jul 3, 2024

Hey! 👋

The issue doesn't seem to contain a minimal reproduction.

Could you provide a snack or a link to a GitHub repository under your username that reproduces the problem?

@github-actions github-actions bot added Missing repro This issue need minimum repro scenario Platform: Android This issue is specific to Android Platform: iOS This issue is specific to iOS labels Jul 3, 2024
@kirillzyusko
Copy link
Contributor Author

cc @tomekzaw as we discussed it with you in direct messages 🙃

@tomekzaw
Copy link
Member

tomekzaw commented Jul 3, 2024

Thanks for reporting this issue, I've forwarded it internally. cc @tjzel @szydlovsky

@szydlovsky szydlovsky self-assigned this Jul 3, 2024
kirillzyusko added a commit to kirillzyusko/react-native-keyboard-controller that referenced this issue Jul 4, 2024
## 📜 Description

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

## 💡 Motivation and Context

<!-- Why is this change required? What problem does it solve? -->
<!-- If it fixes an open issue, please link to the issue here. -->

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:

```tsx
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

<!-- High level overview of important changes -->
<!-- For example: fixed status bar manipulation; added new types
declarations; -->
<!-- If your changes don't affect one of platform/language below - then
remove this platform/language -->

### 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|
|-------|-----|
|<img width="399" alt="image"
src="https://github.com/kirillzyusko/react-native-keyboard-controller/assets/22820318/5511fb3e-54c3-4495-9955-13e933839941">|<img
width="397" alt="image"
src="https://github.com/kirillzyusko/react-native-keyboard-controller/assets/22820318/00d679ba-5986-4fb6-a41a-9c6a1944c98d">|

## 📝 Checklist

- [x] CI successfully passed
- [x] I added new mocks and corresponding unit-tests if library API was
changed
@kirillzyusko
Copy link
Contributor Author

I think the ideal solution would be to re-use useComposedEventHandlers hook and allow to include plain JS-callbacks 🤔

I haven't thought about the exact API yet, but I think a nice solution would be to have something like that:

const onScrollHandler1 = useAnimatedScrollHandler({
    onScroll(e) {
      console.log('Scroll handler 1 onScroll event');
    },
  });
const onScrollHandler2 = (e) => console.log(e);
const composedHandler = useComposedEventHandler([
    onScrollHandler1,
    { onScroll: onScrollHandler2, js: true },
  ]);
// ...
<Animated.ScrollView onScroll={composedHandler}>

And my idea how to achieve that was a modification of useComposedEventHandler:

const handler = useEvent<Event, Context>(...

handler.hasJSHandler = hasJSHandler(handlers)

return handler;

// ... and then in AnimatedComponent we need to check - if `hasJSHandler=true` then we don't need to substitute handler with `dummyListener` and we should use composed js-handler (i. e. propagate one event through all JS handers provided in `useComposedEventHandler`

Of course this is a very rough idea... If you have a better idea on how to achieve his - let me know, I'll be glad to hear that 👀 But my idea was re-using current API without bringing additional complexity to developers who is going to use that 🙈

But if you see a drawbacks in the proposed solution - let me know 👍

@szydlovsky
Copy link
Contributor

@kirillzyusko I'll try building a PoC based on your idea, I will get back to you once I have some more intel on it 👍

@kirillzyusko
Copy link
Contributor Author

By the way - I temporarily solved the problem by using useScrollViewOffset hook instead of useAnimatedScrollHandler. I was needed to know only the scrolled distance and do not perform any custom logic. And in this case useScrollViewOffset doesn't override onScroll property and I can use onScroll + useScrollViewOffset simulateneously.

But I think that the ability to have a composed mixture of worklet/js handlers still would be amazing feature 😎

@szydlovsky
Copy link
Contributor

@kirillzyusko Hi again! Coming back to you after some digging. I have a very very early stage PoC and it seems like it may fly! I've found some limitations though:

  1. I had to specifically choose which events can be passed as JS handlers. For now, I've used the 5 scroll events (scroll, begin drag, end drag, begin momentum, end momentum). Do you think you would find a use-case for any other events? If so - which ones? Before I am able to tackle this problem, I can just directly try adding needed events (for now).
  2. It will be possible to use only one JS handler per JS event (which is quite understandable because of messing with props underneath). So far, I've come up with API like this:
const composedHandler = useComposedEventHandler(
    [onDragHandler, onMomentumHandler],
    {
      onBeginDrag: (_e) => {
        console.log('[JS] begin drag');
      },
      onMomentumBegin: (_e) => {
        console.log('[JS] begin momentum');
      },
    }
  );

So, the JS handlers are being passed in a similar (if not the same) object as in useAnimatedScrollHandler, as a separate second argument of useComposedEventHandler

Tell me what you think, I am open for suggestions 😄

@szydlovsky
Copy link
Contributor

szydlovsky commented Jul 11, 2024

And a sneak peak of the PoC:

347903530-f19eceb1-c3c2-43ce-a65f-ea20f5d7a672.mp4

@kirillzyusko
Copy link
Contributor Author

I had to specifically choose which events can be passed as JS handlers. For now, I've used the 5 scroll events (scroll, begin drag, end drag, begin momentum, end momentum). Do you think you would find a use-case for any other events? If so - which ones? Before I am able to tackle this problem, I can just directly try adding needed events (for now).

I think all events that intercepted by useAnimatedScrollHandler (as you said - these 5 events) are enough 👍

It will be possible to use only one JS handler per JS event (which is quite understandable because of messing with props underneath). So far, I've come up with API like this:

I think it's not a limitation, but maybe it is possible to be consistent and have the API like:

const composedHandler = useComposedEventHandler(
    [onDragHandler, onMomentumHandler],
    [{ // <- an array of JS handlers
      onBeginDrag: (_e) => {
        console.log('[JS] begin drag');
      },
      onMomentumBegin: (_e) => {
        console.log('[JS] begin momentum');
      },
    },
    {onBeginDrag: props.onBeginDrag}]
  );

Could be probably convenient to have an ability to compose multiple JS handlers (for example if you want to combine a handler from props and your own). But this is just idea - will be glad to hear a feedback as well 🙂

@szydlovsky
Copy link
Contributor

@kirillzyusko Alright, thanks for the response! I'll stick to the scrolling events for now. If it comes to the API, yeah, I think I might have an idea on how to merge multiple JS handlers as well - once again I'll let you know as soon as I figure something out 😄

@szydlovsky
Copy link
Contributor

Hey @kirillzyusko I have the earliest PoC patch for you to try:
reanimated_js_handlers.patch
The api is currently what you asked for, namely, a second argument being an array of JS handlers. You can compose multiple ones.
I am yet to correctly detect changes in JS handlers, so that might not work perfectly. Also, I am planning to use just one argument with a union type.
Anyway, let me know what you think and I'd love some critical feedback 😄

@szydlovsky szydlovsky added Platform: Web This issue is specific to web and removed Missing repro This issue need minimum repro scenario labels Jul 16, 2024
@kirillzyusko
Copy link
Contributor Author

@szydlovsky thank you for your hard work 💪

I tested changes from your PR and for me it looks like they work well 👀 (though I tested only basics scenarios)

Great job 🎉

@szydlovsky
Copy link
Contributor

Ahh, just as I pushed some cool changes - now the hook should react to any changes in its argument, including JS handlers + just a single, union-typed argument. Nonetheless, here's the newest patch: rea_js_handlers_second.patch and I'm opening the feature to reviews 😄

@kirillzyusko
Copy link
Contributor Author

@szydlovsky cool, I will try to test it tomorrow or over the weekend!

@kirillzyusko
Copy link
Contributor Author

@szydlovsky I tested and everything works well! Thank you for your hard work!

@szydlovsky
Copy link
Contributor

Super good news! I'll come back to you the last time when the PR get approved and ask if it still does well 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature request Platform: Android This issue is specific to Android Platform: iOS This issue is specific to iOS Platform: Web This issue is specific to web
Projects
None yet
4 participants