-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 useAnimatedKeyboard not giving proper value when using modal #5916
base: main
Are you sure you want to change the base?
Fix useAnimatedKeyboard not giving proper value when using modal #5916
Conversation
…ving-proper-value-when-uisng-modal
Hey @maciekstosio Have you tested Fabric architecture? I've applied changes from this branch and it crashes app immediately when modal appears with: FATAL EXCEPTION: main
com.facebook.react.uimanager.IllegalViewOperationException: Trying to resolve view with tag 480 which doesn't exist
FATAL EXCEPTION: main
Process: com.keyboardcontrollerfabricexample, PID: 3726
com.facebook.react.uimanager.IllegalViewOperationException: Trying to resolve view with tag 480 which doesn't exist
at com.facebook.react.uimanager.NativeViewHierarchyManager.resolveView(NativeViewHierarchyManager.java:102)
at com.facebook.react.uimanager.UIManagerModule.resolveView(UIManagerModule.java:870)
at com.swmansion.reanimated.NodesManager.handleEvent(NodesManager.java:337)
at com.swmansion.reanimated.NodesManager.onEventDispatch(NodesManager.java:320)
at com.facebook.react.uimanager.events.FabricEventDispatcher.dispatchEvent(FabricEventDispatcher.java:43) |
…ving-proper-value-when-uisng-modal
Hi @kirillzyusko! I did test it on Fabric and it worked on example form the PR. I added |
@maciekstosio hey, yes, I'll provide a repo 👍 |
@maciekstosio it seems like I tested your PR against RN 0.73.4 (though now if I try to consume it, I'm getting an error that REA from branch is incompatible with RN 0.73.4). Did you test your PR on RN 0.73 or only RN 0.74? 🤔 |
@kirillzyusko I didn't test it on Fabric on RN 0.73, as we support Fabric since 0.74 |
@maciekstosio I have tried to test RN 0.74 + Fabric but for me it looked like modals were kind of broken there (I could open a modal, but window callbacks were not fired). But if you tested it and it worked good in your case, then I think it's okay - I will try to test everything again more carefully next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to solve some problem, but it's great to hear that it's working 🎉
|
||
private ModalActivityManager mModalActivityManager; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private ModalActivityManager mModalActivityManager; | |
private ModalActivityManager mModalActivityManager; |
@@ -340,6 +346,17 @@ public void onEventDispatch(Event event) { | |||
} | |||
|
|||
private void handleEvent(Event event) { | |||
String eventName = event.getEventName(); | |||
|
|||
if (eventName.equals(ModalActivityManager.OPEN_MODAL_EVENT_NAME)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remind me why we weren't able to add a listener to a screen to receive a notification about opening a modal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check if I could use something else.
android/src/main/java/com/swmansion/reanimated/keyboard/ModalActivityManager.java
Outdated
Show resolved
Hide resolved
android/src/main/java/com/swmansion/reanimated/keyboard/KeyboardAnimationManager.java
Outdated
Show resolved
Hide resolved
android/src/main/java/com/swmansion/reanimated/keyboard/ModalActivityManager.java
Outdated
Show resolved
Hide resolved
android/src/main/java/com/swmansion/reanimated/keyboard/ModalActivityManager.java
Outdated
Show resolved
Hide resolved
android/src/main/java/com/swmansion/reanimated/keyboard/ModalActivityManager.java
Outdated
Show resolved
Hide resolved
|
||
private final HashMap<Integer, WindowsInsetsManager> insetsManagersMap = new HashMap<>(); | ||
|
||
public ModalActivityManager( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to have more than one instance of ModalActivityManager
?
fdba215
to
4ae9b77
Compare
…ving-proper-value-when-uisng-modal pick bf61d254 feat: extract shared example and use in in both Example and FabricExample
I tested few more approaches to tackle detecting Dialog open, but none of them seems like good fit:
Other hacky solutions on hold:
Preferable solution in research:
For now we’re leaving it on hold and considering RN contribution that would make the solution less hacky. |
Summary
Fixes #5754
Because modals are open next to the root view in view hierarchy, they are not observed by InsetManager. In this PR I added separate class that manages opened dialogs, so we can listen to changes on modals too. The assumption is that the useAnimatedKeyboard gives the value of any opened keyboard, not only the one in current view.
before.useAnimatedKeyboardModal.mov
after.useAnimatedKeyboard.modal.mov
Test plan
Test component