Skip to content
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: cleanup kvo #146

Merged
merged 2 commits into from
Apr 18, 2023
Merged

fix: cleanup kvo #146

merged 2 commits into from
Apr 18, 2023

Conversation

kirillzyusko
Copy link
Owner

@kirillzyusko kirillzyusko commented Apr 17, 2023

📜 Description

Remove KVO when keyboard disappears.

💡 Motivation and Context

It seems like we can not attach a single listener to the keyboard when it appeared for the first time. iOS may throw KVO_IS_RETAINING_ALL_OBSERVERS_OF_THIS_OBJECT_IF_IT_CRASHES_AN_OBSERVER_WAS_OVERRELEASED_OR_SMASHED, which means, that the memory for the listener was released and thus a handler is corrupted (KVO was released while still observing the key path).

To overcome the problem I've decided to remove observation when keyboard becomes hidden. To detect the moment when keyboard got hidden I used UIWindow.didBecomeHiddenNotification (since for attaching a KVO listener I've used UIWindow.didBecomeVisibleNotification - the motivation behind "why I've used these lifecycles" is the fact, that these lifecycle methods will be triggered only one time when keyboard appears/disappears, unlike other methods, which can be called several times).

Also I've added a local variable called hasKVObserver and two helper functions:

  • setupKVObserver;
  • removeKVObserver.

These functions control the value of hasKVObserver and assure that we will not call .removeObserver if listener is not attached (such situation will cause a crash and using hasKVObserver value we avoid it).

Fixes #143

📢 Changelog

iOS

  • added UIWindow.didBecomeVisibleNotification listener;
  • clean KVO when UIWindow.didBecomeVisibleNotification is emitted and KVO is present;

🤔 How Has This Been Tested?

Tested on:

  • iPhone 14 Pro (iOS 16.2, simulator);
  • iPhone 6s (iOS 15.7, real device);

📝 Checklist

  • CI successfully passed

@kirillzyusko kirillzyusko added 🐛 bug Something isn't working 🍎 iOS iOS specific labels Apr 17, 2023
@kirillzyusko kirillzyusko self-assigned this Apr 17, 2023
@kirillzyusko kirillzyusko marked this pull request as ready for review April 18, 2023 11:43
@kirillzyusko kirillzyusko merged commit b1192a2 into main Apr 18, 2023
@kirillzyusko kirillzyusko deleted the refactor/cleanup-kvo branch April 18, 2023 11:48
@kirillzyusko kirillzyusko mentioned this pull request Jun 19, 2023
1 task
kirillzyusko added a commit that referenced this pull request Jun 21, 2023
## 📜 Description

Fixed a crash on iOS 16.5 when native alert is getting shown.

## 💡 Motivation and Context

The original exception occurred because of this error:

```bash
libc++abi: terminating due to uncaught exception of type folly::json::print_error: folly::toJson: JSON object value was an INF when serializing value at "progress"
```

`progress` was `inf` because in:

```swift
onEvent("onKeyboardMove", keyboardPosition as NSNumber, keyboardPosition / CGFloat(keyboardHeight) as NSNumber)
```

`keyboardHeight` variable was `0`. When we were trying to divide any
number by `0` in swfit we get `Inf` value, and this value can not be
serialised to JS equivalent.

The reason why this value becomes `0` is the fact that we have next
chain of calls:

```
keyboardWillHide
keyboardDidHide <- here we are setting keyboardHeight=0
keyboardWillHide <- here we setup CADisplayLink and after 16ms the crash will occur
keyboardDidHide
```

Based on output from above I had 3 options on how to fix the error:

### 1. Add `.isFinite` check

Good option, but it will prevent sending `onMove` event, though
according to values from `CADisplayLink` movement actually happens. So
we will kind of ignore actual information and it's not something that
satisfy me, because it may produce other bugs in the future.

### 2. Update `keyboardHeight` in `keyboardWillHide`

This variant also prevents a crash, but `progress` value in `onMove`
sometimes can be higher than `1` (in my tests sometimes it was 1.3+).
And it also seems like it gives incorrect calculation, because it's
strange to see progress > 1, when keyboard simply hides 🤷‍♂️

### 3. Don't set `keyboardHeight` to `0`

To come back to `1.4.x` behaviour, we don't need to reset
`keyboardHeight` to `0`. But I've added this code intentionally in
`1.5.0`, because in KV observer we are checking that keyboard should be
open and not animating:

```swift
if displayLink != nil || keyboardHeight == 0.0 {
  return
}
```

I've added this check, because I've attached KV observer before any
keyboard movement. The reason why I've added it in
`UIWindow.didBecomeVisibleNotification`/`UIWindow.didBecomeHiddenNotification`
was the fact, that these methods were called only one time (keyboard
events can be fired multiple times). But I've added `hasKVObserver`
variable in
#146
so it's safe to call `setupKVObserver`/`removeKVObserver` multiple
times.

So in this PR I've decided to stick to this approach - don't reset
`keyboardHeight` to `0` + setup KV listeners in different lifecycle
methods.

Should close
#175

## 📢 Changelog

### iOS
- moved `setupKVObserver`/`removeKVObserver` to
`keyboaardDidAppear`/`keyboardWilHide` lifecycle;
- don't set `keyboardHeight` to `0` (this value acts as
`persisteKeyboardHeight` on Android);

## 🤔 How Has This Been Tested?

Tested on:
- iPhone 6s (iOS 15.6);
- iPhone 14 Pro (iOS 16.2);
- iPhone 11 (iOS 16.5);

## 📸 Screenshots (if appropriate):

|Before|After|
|------|-----|
|<video
src="https://github.com/kirillzyusko/react-native-keyboard-controller/assets/22820318/82d46951-3471-4b59-8dd6-3062922a76f0">|<video
src="https://github.com/kirillzyusko/react-native-keyboard-controller/assets/22820318/b73d2aa2-72a9-4272-8b37-f6820d1c816a">|

## 📝 Checklist

- [x] CI successfully passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 🍎 iOS iOS specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iOS crash
1 participant