From 3425cd06dfcceb794c0a99d5cacab5431ace3a7b Mon Sep 17 00:00:00 2001 From: Kirill Zyusko Date: Tue, 3 Dec 2024 22:59:47 +0100 Subject: [PATCH] fix: crash when Modal gets shown on Android < 9 (#718) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 📜 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 https://github.com/kirillzyusko/react-native-keyboard-controller/pull/466 and https://github.com/kirillzyusko/react-native-keyboard-controller/pull/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 https://github.com/kirillzyusko/react-native-keyboard-controller/issues/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 --- .../modal/ModalAttachedWatcher.kt | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/android/src/main/java/com/reactnativekeyboardcontroller/modal/ModalAttachedWatcher.kt b/android/src/main/java/com/reactnativekeyboardcontroller/modal/ModalAttachedWatcher.kt index 902b882f7..2bcbf13fa 100644 --- a/android/src/main/java/com/reactnativekeyboardcontroller/modal/ModalAttachedWatcher.kt +++ b/android/src/main/java/com/reactnativekeyboardcontroller/modal/ModalAttachedWatcher.kt @@ -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