fix: incorrect `Modal` layout (if it was open while keyboard was visible) #624

kirillzyusko · 2024-10-09T14:24:38Z

📜 Description

Fixed a problem of Modal window not stretching to full height of the screen if Modal was opened when keyboard was open.

💡 Motivation and Context

The problem was caused by the fact that I attached OnApplyWindowListener to a rootView. Somehow it was breaking a layout and always kept keyboard space if Modal was opened when keyboard is visible.

The first approach was an attempt to resize that window. I've tried to specify .layoutParams but it was a failed attempt. Why? Because in the end I had two options:

either it wasn't working.
triggered a crash;

After some time I wrote a code where I set OnApplyWindowListener conditionally:

if keyboard open then I set it to DialogRootViewGroup (I was getting an instance of ReactModalHostView.hostVIew via reflection);
if keyboard is not open then attach to root view.

While it was working approach it has several downsides:

it is pretty complex (reflection, conditional code, making some internals methods isKeyboardVisible public one);
when you often toggle keyboard between emoji/text states, then at some point it may not update position properly.

So I decided to look for a more robust approach - and I decided to apply changes that I used in EdgeToEdgeReactViewGroup. I created a custom view (0 width and height) and was attaching it to the rootView of the modal. In this case I don't inject my custom handler into rootView, so the layout is managed properly.

While it works I noticed that sometimes red (animated) or gray (reanimated) circles may behave differently and eventually they have a broken state. The problem is that sometimes onProgress/onEnd may be triggered from main callback. I added Suspense mechanism where I can toggle off the functionality on demand and pause callbacks events.

While it was working good on Android 12+ it wasn't working on Android < 12. The problem comes from the fact that on these OS versions Android emulates keyboard transitions, and listens to main window. So if we pause main window callback then all transitions are not working - the obvious way to fix is to make .suspense call conditional.

I refactored that part (reduced code duplication, moved everything to reusable code, etc.) and now this PR is ready to be merged 😊

Closes #619

📢 Changelog

JS

added suspendable to cspell ignore;

E2E

added new test verifying a layout correctness when Modal has been opened with an opened keyboard;

Android

added new constants package with new Keyboard class (with IS_ANIMATION_EMULATED field);
added Suspendable interface;
KeyboardAnimationCallback now implements Suspendable;
added early return to onStart/onProgress/onEnd callbacks if callback is suspended;
change WindowInsetsCompat.CONSUMED back to insets;
syncKeyboardPosition now can accept height and visible as params;
pass KeyboardAnimationCallback instance to ModalAttachedWatcher from EdgeToEdgeReactViewGroup;
add eventView to rootView of Modal when modal is shown;
set setOnApplyWindowInsetsListener to eventView;
remove eventView when Modal dismissed;
force syncKeyboardPosition to closed keyboard state when Modal is shown on Android 12+;
suspend main KeyboardAnimationCallback when Modal is shown on Android 12+;
unsuspend main KeyboardAnimationCallback when Modal is dismissed;

🤔 How Has This Been Tested?

Tested manually on:

Pixel 3A (API 33);
Pixel 6 Pro (API 28);

Additionally covered everything with e2e tests 😎

📸 Screenshots (if appropriate):

Device	Before	After
Pixel 3A (API 33)
Pixel 6 Pro (API 28)

📝 Checklist

CI successfully passed
I added new mocks and corresponding unit-tests if library API was changed

github-actions · 2024-10-09T14:25:51Z

📊 Package size report

Current size	Target Size	Difference
156922 bytes	156313 bytes	609 bytes 📈

shollington-rbi · 2024-10-10T04:18:01Z

This is awesome @kirillzyusko! Excited to test it out when a new package is published.

## 📜 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

kirillzyusko added 🐛 bug Something isn't working 🤖 android Android specific 🌎 modal Anything that involves Modal usage labels Oct 9, 2024

kirillzyusko self-assigned this Oct 9, 2024

kirillzyusko added the e2e Anything about E2E tests label Oct 9, 2024

kirillzyusko added 7 commits October 9, 2024 18:24

fix: correct modal layout when keyboard was shown

d0b71f9

fix: v2

3cebff3

fix: ktlint

424a6fd

chore: code clean up

4604107

feat: cover new functionality via e2e test

d90299c

fix: follow up changes

3c89dda

fix: e2e tests

7cf54fe

kirillzyusko force-pushed the fix/619-modal-incorrect-layout branch from db835f6 to 7cf54fe Compare October 9, 2024 16:39

kirillzyusko added 2 commits October 9, 2024 21:06

refactor: address technical debt

e1d0094

fix: changes after review

f91cc1e

kirillzyusko marked this pull request as ready for review October 9, 2024 20:39

kirillzyusko merged commit bb5e9e1 into main Oct 10, 2024
22 checks passed

kirillzyusko deleted the fix/619-modal-incorrect-layout branch October 10, 2024 08:19

kirillzyusko mentioned this pull request Dec 3, 2024

fix: crash when Modal gets shown on Android < 9 #718

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: incorrect `Modal` layout (if it was open while keyboard was visible) #624

fix: incorrect `Modal` layout (if it was open while keyboard was visible) #624

kirillzyusko commented Oct 9, 2024 •

edited

Loading

github-actions bot commented Oct 9, 2024 •

edited

Loading

shollington-rbi commented Oct 10, 2024

fix: incorrect Modal layout (if it was open while keyboard was visible) #624

fix: incorrect Modal layout (if it was open while keyboard was visible) #624

Conversation

kirillzyusko commented Oct 9, 2024 • edited Loading

📜 Description

💡 Motivation and Context

📢 Changelog

JS

E2E

Android

🤔 How Has This Been Tested?

📸 Screenshots (if appropriate):

📝 Checklist

github-actions bot commented Oct 9, 2024 • edited Loading

📊 Package size report

shollington-rbi commented Oct 10, 2024

fix: incorrect `Modal` layout (if it was open while keyboard was visible) #624

fix: incorrect `Modal` layout (if it was open while keyboard was visible) #624

kirillzyusko commented Oct 9, 2024 •

edited

Loading

github-actions bot commented Oct 9, 2024 •

edited

Loading