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

[ios] KeyboardAvoidingView and KeyboardStickyView do not work in native stacks where screens are presented as fullScreenModal #271

Closed
erickreutz opened this issue Nov 12, 2023 · 4 comments · Fixed by #272
Assignees
Labels
🐛 bug Something isn't working 🍎 iOS iOS specific

Comments

@erickreutz
Copy link

When using KeyboardAvoidingView or KeyboardStickyView in combination with @react-navigation/native-stack and setting presentation: "fullScreenModal" on the screen options of the screen that is using the KeyboardAvoidingView or KeyboardStickyView do not work as expected.

Here is a patch of the small changes I made to your example app to reproduce the issue:

diff --git a/example/src/navigation/RootStack/index.tsx b/example/src/navigation/RootStack/index.tsx
index 353a1491..8666b60e 100644
--- a/example/src/navigation/RootStack/index.tsx
+++ b/example/src/navigation/RootStack/index.tsx
@@ -1,6 +1,6 @@
 import React from 'react';
 
-import { createStackNavigator } from '@react-navigation/stack';
+import { createNativeStackNavigator } from '@react-navigation/native-stack';
 import { ScreenNames } from '../../constants/screenNames';
 import ExamplesStack from '../ExamplesStack';
 import ExampleMain from '../../screens/Examples/Main';
@@ -10,10 +10,13 @@ export type RootStackParamList = {
   [ScreenNames.EXAMPLES_STACK]: {};
 };
 
-const Stack = createStackNavigator<RootStackParamList>();
+const Stack = createNativeStackNavigator<RootStackParamList>();
 
 const options = {
-  [ScreenNames.EXAMPLES_STACK]: { headerShown: false },
+  [ScreenNames.EXAMPLES_STACK]: {
+    presentation: 'fullScreenModal',
+    headerShown: false,
+  },
   [ScreenNames.EXAMPLES]: { title: 'Examples' },
 };
@kirillzyusko kirillzyusko added 🐛 bug Something isn't working 🍎 iOS iOS specific 📚 components Anything related to the exported components of this library labels Nov 13, 2023
@kirillzyusko
Copy link
Owner

@erickreutz Interesting. It seems it happens, because this code:

override func willMove(toWindow newWindow: UIWindow?) {
    if newWindow == nil {
      // Will be removed from a window
      inputObserver?.unmount()
      keyboardObserver?.unmount()
    }
  }

get executed. As a temporary solution you can simply comment out this code, and in a meantime I'll try to figure out why this code is getting executed and how to properly clean up listeners 👀

@kirillzyusko kirillzyusko removed the 📚 components Anything related to the exported components of this library label Nov 13, 2023
@kirillzyusko
Copy link
Owner

@erickreutz This PR #272 should fully fix a problem - would you mind to try it out and let me know whether it works for you or not?

@erickreutz
Copy link
Author

@kirillzyusko it works! Thank you!

@kirillzyusko
Copy link
Owner

Awesome @erickreutz! For temporary solution you can apply a local patch. I'm going to test Fabric implementation (because there is a different view implementation) and when it'll be ready I'll merge the PR and publish new 1.9.2 version 😎

kirillzyusko added a commit that referenced this issue Nov 14, 2023
…ck (#272)

## 📜 Description

Fixed a conflict when using `fullScreenModal` presentation in
`native-stack`.

## 💡 Motivation and Context

`willMove(toWindow)` (with `newWindow == nil`) will be called in two
cases:
- view is unmounted;
- a navigation to modal window has occurred.

And we don't need to cleanup a listener if navigation has occurred. So
in this PR I'm switching to `willMove(toSuperview)` method that called
only when view is destroyed (when navigation occurs nothing happens).

Also I decided to unify initialization mechanism between architectures.
The view creation steps are following:
- init
- prop setters
- willMove(toSuperview)

On paper I did an initialization and mounting in `willMove` method
(setter was called on non initialized object). On fabric I initialized
in init method and mounted in prop setters (I didn't use lifecycle
methods in Fabric). However I think that ignoring lifecycle method and
keeping subscriptions is not a good idea (though events are not
propagated and there is no crashes) - potentially we may have bad memory
and other unpleasant exceptions.

So in this PR I decided to unify approach and now we're using next
schema:
- initialize observers in `init`;
- mount/unmount in prop setter;
- mount/unmount in lifecycle.

With such approach potentially on initial mount we can initialize
observer 2 times. To avoid that I've added `isMounted` property to
observers. So now they can be initialized only once (even if you call
`.mount` several times).

Last but not least - I've added private methods mount/unmount on view
level to reduce code duplication (now we have two observers, so one
method initialize both of them).

Closes
#271

## 📢 Changelog

### iOS
- switched from `willMove(toWindow)` to `willMove(toSuperview)`;

## 🤔 How Has This Been Tested?

Tested on iPhone 15 Pro (iOS 17.0.1).

## 📝 Checklist

- [x] CI successfully passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 🍎 iOS iOS specific
Projects
None yet
2 participants