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

onScroll on KeyboardAwareScrollView is not fired #337

Closed
kirillzyusko opened this issue Jan 19, 2024 · 11 comments · Fixed by #339 or #408
Closed

onScroll on KeyboardAwareScrollView is not fired #337

kirillzyusko opened this issue Jan 19, 2024 · 11 comments · Fixed by #339 or #408
Assignees
Labels
🐛 bug Something isn't working 📚 components Anything related to the exported components of this library

Comments

@kirillzyusko
Copy link
Owner

Describe the bug

onScroll on KeyboardAwareScrollView is not fired

Code snippet

Just add onScroll={console.log} in example app.

Repo for reproducing

You can use example app.

To Reproduce
Steps to reproduce the behavior:

  1. Go to KeyboardAwareScrollView
  2. Scroll view
  3. See console output

Expected behavior

You should see events if you scroll ScrollView.

Smartphone (please complete the following information):

  • Desktop OS: MacOS 14.2.1
  • Device: iPhone 15 Pro
  • OS: iOS 15.2
  • RN version: 0.72.4
  • RN architecture: old
  • JS engine: Hermes
  • Library version: 1.10.2

Additional context

It can be fixed by adding next code into onScroll handler:

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

+        if (onScrollProp) {
+          runOnJS(onScrollProp)({ nativeEvent: e });
+        }
      },
    },
    [],
  );

or assign our onScroll handler as onScrollReanimated (REA intercept all events for a given view, so there is no difference which prop to use).

@kirillzyusko kirillzyusko added the 📚 components Anything related to the exported components of this library label Jan 19, 2024
@kirillzyusko kirillzyusko self-assigned this Jan 19, 2024
@kirillzyusko kirillzyusko added the 🐛 bug Something isn't working label Jan 19, 2024
@z1haze
Copy link

z1haze commented Apr 3, 2024

This is not fixed. onScroll does NOT fire from KeyboardAwareScrollView. @kirillzyusko please reopen this. I even pulled in 1.10.3 which was the version that supposedly fixed this issue to make sure it was not just a regression. It remains broken on that version, too.

@kirillzyusko
Copy link
Owner Author

@z1haze thanks for bringing it up 👀 May I kindly ask you to test #408 and see whether it fixes the problem or not?

I believe it should fix because I just tested example app and had the same issue as you - and this PR resovles the problem. But I'm more than sure that previous approach was working - most likely with a new release of RN or REA it's not working now, but again it was working before 👀 😅 .

@DaltonPelkey
Copy link

DaltonPelkey commented Apr 4, 2024

Hey @kirillzyusko. Checking your solution, would it not be more effective to use a normal scroll callback?
This is my current patch for it:

    const onScroll = useCallback((event:NativeSyntheticEvent<NativeScrollEvent>) => {
      position.value = event.nativeEvent.contentOffset.y;
      onScroll2?.(event);
    }, [onScroll2]);

Is there an underlying purpose for using the reanimated hook for the callback?

Edit:
To add more context. I replaced the onScrollReanimated mystery prop you were using with onScroll

@kirillzyusko
Copy link
Owner Author

Is there an underlying purpose for using the reanimated hook for the callback?

Yeah, it will update the value on UI thread synchronously. In your case it'll be updated on JS thread and it will not be as efficient as updating on UI thread.

Potentially it's possible to use your solution (I am reading scroll shared value only when keyboard movement happens - that value is not driving any animation, so it's okay to miss some frames during update), but of course I need to check that there is no performance hits 😅

@DaltonPelkey
Copy link

That makes sense. I originally tried the runOnJS solution, but I think it wasn't working correctly because the scroll handler from props needs to be passed to the useAnimatedScrollHandler deps array. My callback wasn't being updated with its own dependencies for that reason, I think.

Aside from that, if the regular callback works, it seems less "hacky" to do that since you can pass the entire event rather than just the nativeEvent properties.

@kirillzyusko
Copy link
Owner Author

Aside from that, if the regular callback works, it seems less "hacky" to do that since you can pass the entire event rather than just the nativeEvent properties.

Yeah, for sure 👍 I changed a code in my PR - would you mind to test it and confirm whether it fixes the problem in your project? 👀

@DaltonPelkey
Copy link

I've been using the JS implementation since this morning. Everything seems to be fine. My only concern was that it would mess with something since it is not longer using Reanimated's hook. I also wasn't sure what was going on with that mystery onScrollReanimated prop.

@kirillzyusko
Copy link
Owner Author

@DaltonPelkey onScrollReanimated is an internal prop and was used to intercept onScroll event 🙂 So it shouldn't be a breaking change.

My only concern was that it would mess with something since it is not longer using Reanimated's hook.

I think it shouldn't. And you are right - this implementation looks less hacky so let's try this 👀

I'll merge PR tomorrow 👍

@z1haze
Copy link

z1haze commented Apr 5, 2024

@kirillzyusko we could not find anywhere that prop was being used. Could you shed some light on that?

@kirillzyusko
Copy link
Owner Author

@z1haze the ScrollView was coming from Reanimated. So output from useAnimatedScrollHandler can be attached as any prop to ScrollView and it'll work (literally it can be abracadabra and it will intercept necessary events - that's how REA works).

But yes, it's kind of hack and it will produce TS issues anyway, so better not use it 👀

kirillzyusko added a commit that referenced this issue Apr 5, 2024
## 📜 Description

Fixed firing of `onScroll` in `KeyboardAwareScrollView`

## 💡 Motivation and Context

Initially it was fixed in
#339
and I'm sure it was working 😅

However it looks like in current setup (RN 0.73, REA 3.8.0) such way is
not working and `onScroll` property still gets ignored 😔

So in this PR I followed a different path: initially I wanted to fire
this handler via `runOnJS`, but we can not follow a full signature of
the method, since REA has only `nativeEvent` property (and indeed in
this case some properties would be simply missing).

The different path was to update a shared value from JS thread and call
a callback as usually - it fixes a problem in old code and in a new, so
for now this approach looks decent 👍

Let's see how it works in a wild life 👀

Also in this PR I'm changing content of `tea.yaml` - it should be done
in separate PR, but I don't want to open another PR just to fix quotes,
so decided to merge everything in a single one 🙈

Closes
#337

## 📢 Changelog

### JS

- fire `onScroll` in callback;
- react on scroll position change in JS thread.

### FS (file system)

- added single quotes `'` for `tea.yaml`;

## 🤔 How Has This Been Tested?

Tested manually on iPhone 15 Pro.

## 📸 Screenshots (if appropriate):

<img width="474" alt="image"
src="https://github.com/kirillzyusko/react-native-keyboard-controller/assets/22820318/11896adc-3fdd-4ab3-acf9-9f32f27e787c">

## 📝 Checklist

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

Published under 1.11.6 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 📚 components Anything related to the exported components of this library
Projects
None yet
3 participants