fix: modal integration #466
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kirillzyusko
added
🐛 bug
📊 Package size report
|
kirillzyusko
commented
Jun 10, 2024
android/src/main/java/com/reactnativekeyboardcontroller/views/EdgeToEdgeReactViewGroup.kt
Outdated
Show resolved
Hide resolved
kirillzyusko
force-pushed
the
fix/modal
branch
2 times, most recently
from
ff42f29 to
24f714b
Compare
…e a modal rootView, not app rootView - otherwise "focused input" functionality will be broken)
2 tasks
kirillzyusko
added a commit
that referenced
this pull request
Dec 3, 2024
## 📜 Description
Fixed a crash when `progress` value becomes `Infinity` and can not be
serialized to JS.
## 💡 Motivation and Context
It happens when we try to divide any double/float number (except `0`) by
`0.0`. In our case we were trying to divide `height /
persistentKeyboardHeight`. To shed more light on the issue I modified
condition `it.isNan()` to `it.isNaN() || it.isInfinite()`, and we get:
```bash
LOG {"duration": 160, "eventName": "onKeyboardMoveStart", "height": 0, "progress": 0, "target": -1}
LOG {"duration": 160, "eventName": "onKeyboardMove", "height": 47.71428680419922, "progress": 0, "target": -1}
LOG {"duration": 160, "eventName": "onKeyboardMove", "height": 47.71428680419922, "progress": 0, "target": -1}
LOG {"duration": 160, "eventName": "onKeyboardMove", "height": 47.71428680419922, "progress": 0, "target": -1}
LOG {"duration": 160, "eventName": "onKeyboardMove", "height": 46.57143020629883, "progress": 0, "target": -1}
LOG {"duration": 160, "eventName": "onKeyboardMove", "height": 42.85714340209961, "progress": 0, "target": -1}
LOG {"duration": 160, "eventName": "onKeyboardMove", "height": 36.57143020629883, "progress": 0, "target": -1}
LOG {"duration": 160, "eventName": "onKeyboardMove", "height": 28, "progress": 0, "target": -1}
LOG {"duration": 160, "eventName": "onKeyboardMove", "height": 17.714284896850586, "progress": 0, "target": -1}
LOG {"duration": 160, "eventName": "onKeyboardMove", "height": 9.142857551574707, "progress": 0, "target": -1}
LOG {"duration": 160, "eventName": "onKeyboardMove", "height": 2.2857143878936768, "progress": 0, "target": -1}
LOG {"duration": 160, "eventName": "onKeyboardMove", "height": 0, "progress": 0, "target": -1}
LOG {"duration": 160, "eventName": "onKeyboardMove", "height": 0, "progress": 0, "target": -1}
LOG {"duration": 160, "eventName": "onKeyboardMoveEnd", "height": 0, "progress": 0, "target": -1}
```
In `onStart` the `keyboardHeight=0`, `isKeyboardVisible-false` and we
don't update `this.persistentKeyboardHeight` (it's 0.0 by default).
From logs above it looks like keyboard is animating from `47` pixels to
`0`, but in fact there is no keyboard animation on the screen at all! 🤯
So in my understand it's a bug somewhere in google code (or in my code -
but such animation is not correct). Below are the options that I
considered for a fix:
### 1️⃣ add a condition, that will check if a prev `height !== 0` and if
`nextHeight === 0` then ignore an animation
Such solution will add more complexity, more if-statements without an
explicit benefits and warranty that it will fix/work for other similar
scenarios.
### 2️⃣ check that target == -1 and ignore animation then
Not a reliable solution, because we start to depend on 3rd party
variables state. And it's a known fact, that if you get a transitive
dependencies, then the solution drastically becomes more complex.
### 3️⃣ add `isInfinite` check and set progress to `0` then
While it fixes a crash we still get an incorrect `height` values so any
animation that depends on these values will be running and it will look
strange 😬
### 4️⃣ don't attach keyboard callbacks to Modal window on Android 9
The most perspective solution - on Android < 12 all events are passing
through the main window (and actually attaching a callback is not
necessary there, but when I implemented
#466
and
#624
I decided to keep the code for consistency). So if we remove these
callbacks (if we attach them to window only for Android 12) then all
code works well on both platforms.
Closes
#714
## 📢 Changelog
### Android
- don't attach keyboard callbacks on Android < 12 to `Modal` window;
## 🤔 How Has This Been Tested?
Tested in reproduction example and on CI.
## 📸 Screenshots (if appropriate):
https://github.com/user-attachments/assets/76331e0d-6779-4a4b-a837-ba4802ac4139
## 📝 Checklist
- [x] CI successfully passed
- [x] I added new mocks and corresponding unit-tests if library API was
change
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📜 Description
Fixed a problem of keyboard movement not being detected in Modal window on Android.
💡 Motivation and Context
The most challenging in this PR is getting an access to
dialog. Initially I thought to create an additional view (ModalKeyboardProvider) and then through this view get an access to thedialog, but after some experiments I realized, that we'll get an access toDialogRootViewGroupand this class is not holding a reference todialog, so this idea failed.Another approach was to listen to all events in
eventDispatcherand when we detectonShow(topShow) event, then usinguiManagerwe can resolve the view and getReactModalHostView. This approach work, but it has one downside - we listen to all events. It doesn't hit the performance, but this approach looks like a workaround (it has too many transitive dependencies).A PR facebook/react-native#45534 introduces a new API for detection show/hide modals. I hope it'll be merged at some point of time and we can start to use new API. In a meantime I'll use the current approach and will incapsulate all work in
ModalAttachedWatcher- in this case we have our own interface for dealing with modals, and we can change internals without affecting other files 😎Closes #369
Potentially helps to fix #387
📢 Changelog
E2E
modaltest;Android
ModalAttachedWatcherclass;KeyboardAnimationCallbacknow accepts config to reduce amount of passed params (KeyboardAnimationCallbackConfig);eventPropagationViewtoKeyboardAnimationCallbackandFocusedInputObserver;WindowInsetsCompat.CONSUMEDfromonApplyWindowInsetsinstead ofinsets;syncKeyboardPositiontoKeyboardAnimationCallback;🤔 How Has This Been Tested?
Tested manually on:
📸 Screenshots (if appropriate):
📝 Checklist