Skip to content

Commit

Permalink
fix: crash when Modal gets shown on Android < 9 (#718)
Browse files Browse the repository at this point in the history
## 📜 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
  • Loading branch information
kirillzyusko authored Dec 3, 2024
1 parent 08f9861 commit 3425cd0
Showing 1 changed file with 9 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,17 @@ class ModalAttachedWatcher(
)

rootView.addView(eventView)
// on Android < 12 all events for `WindowInsetsAnimationCallback`
// go through main `rootView`, so we don't need to stop main
// callback - otherwise keyboard transitions will not be animated
this.callback()?.suspend(areEventsComingFromOwnWindow)
ViewCompat.setWindowInsetsAnimationCallback(rootView, callback)
ViewCompat.setOnApplyWindowInsetsListener(eventView, callback)

if (areEventsComingFromOwnWindow) {
// on Android < 12 all events for `WindowInsetsAnimationCallback`
// go through main `rootView`, so we don't need to stop main
// callback - otherwise keyboard transitions will not be animated
this.callback()?.suspend(true)
// attaching callback to Modal on Android < 12 can cause ghost animations, see: https://github.com/kirillzyusko/react-native-keyboard-controller/pull/718
// and overall attaching additional callbacks (if animation events go through the main window) is not necessary
ViewCompat.setWindowInsetsAnimationCallback(rootView, callback)
ViewCompat.setOnApplyWindowInsetsListener(eventView, callback)

// when modal is shown then keyboard will be hidden by default
//
// - if events are coming from main window - then keyboard position
Expand Down

0 comments on commit 3425cd0

Please sign in to comment.