Skip to content

Commit

Permalink
fix: properly report height for didShow event happening during anim…
Browse files Browse the repository at this point in the history
…ation (#557)

## 📜 Description

Fixed a case when `onEnd`/`keyboardDidShow` reports incorrect (previous
frame) keyboard height.

## 💡 Motivation and Context

Fixes regressions introduced in
#539

Initially I thought it's safe to read current keyboard frame in
`didAppear` lifecycle (I thought it'll be always dispatched after
animation finish, so why not to read it). But it was a fatal mistake
because:
- when keyboard changes its size it changes it immediately and in
`didAppear` we'll read old (previous) value ⛔
- on iOS 15 when modal gets hidden, then keyboard appears instantly and
iOS 15 dispatches only one event, so we'll read old frame (i. e. `0`); 🔴
- when iOS 17 with `secureTextEntry` randomly attaches/detaches
`inputAccessoryView`, then we may also encounter a situation, when we
read old frame and we will avoid keyboard incorrectly ❌

So to sum up all previous approaches:
- relying on `duration === 0` is not reliable because keyboard can be
hidden immediately but with `duration === 250` 🤯
- reading current frame in `keyboardDidShow` is not correct approach
because in some situations we may read old values 😔

This PR is a third revision of the solution 😂 Now I learned all lessons
(at least I hope so) and invented a better mechanism. Let's go back to
original problem.

The original problem was in the fact, that a resize event can be
dispatched during animation AND it'll contain incorrect information
about keyboard size (it'll have final destination, though animation
hasn't finished yet). I decided to catch that situation in the code - I
added `didShowDeadline` variable and in `keyboardWillAppear` I set it as
`timestamp + duration`. In `keyboardDidAppear` I verify, that this event
arrived later, and if so, then we read `keyboardHeight` value from
`notification`. Otherwise, if event `didShow` was received before
deadline (i. e. resize event) we can not read data from notification (as
it may not reflect a real state of the things) and instead we are
reading it from keyboard view layer (falling back to what is actually
shown on the screen). Schematically pipeline looks like:

<img width="687" alt="image"
src="https://github.com/user-attachments/assets/a5fd7b2f-ee8c-4c86-82fc-d017bb01f363">

I think such approach is safer and it looks like it will fix all known
issues and at the same time it doesn't introduce additional complexity
to the JS codebase.
 
Fixes
#327 (comment)
Expensify/App#47679

## 📢 Changelog

### iOS

- added `didShowDeadline` variable;
- initialize `didShowDeadline` in `willAppear`;
- check if `didShowDeadline` is bigger than current timestamp, and if
yes, then only then read values from layer. Otherwise as before - from a
notification

## 🤔 How Has This Been Tested?

Tested manually on many devices.

## 📸 Screenshots (if appropriate):

### iOS 15

|Before|After|
|-------|-----|
|<video
src="https://github.com/user-attachments/assets/8253efaa-a20e-4566-b747-7676b6c6cd1e">|<video
src="https://github.com/user-attachments/assets/988340c6-da7e-40f5-bcd2-6ec95e7d1ea4">|

### Keyboard resize

|Before|After|
|-------|-----|
|<video
src="https://github.com/user-attachments/assets/6d008543-38b1-46e3-b792-b4ef813fea77">|<video
src="https://github.com/user-attachments/assets/fc7108d7-e996-4568-8f6e-1cefdf7a3e9a">|

### `KeyboardAvoidingView`

|Before|After|
|-------|-----|
|<video
src="https://github.com/user-attachments/assets/d43f8b53-d442-4a42-a573-d3a2e30a04ad">|<video
src="https://github.com/user-attachments/assets/c7cfe29e-7467-497f-9f4d-b824956f50d6">|

## 📝 Checklist

- [x] CI successfully passed
- [x] I added new mocks and corresponding unit-tests if library API was
changed
  • Loading branch information
kirillzyusko authored Aug 21, 2024
1 parent 1c58299 commit 9b5510d
Showing 1 changed file with 9 additions and 3 deletions.
12 changes: 9 additions & 3 deletions ios/observers/KeyboardMovementObserver.swift
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public class KeyboardMovementObserver: NSObject {
private var duration = 0
private var tag: NSNumber = -1
private var animation: KeyboardAnimation?
private var didShowDeadline: Int64 = 0

@objc public init(
handler: @escaping (NSString, NSNumber, NSNumber, NSNumber, NSNumber) -> Void,
Expand Down Expand Up @@ -168,6 +169,7 @@ public class KeyboardMovementObserver: NSObject {
let keyboardHeight = keyboardFrame.cgRectValue.size.height
self.keyboardHeight = keyboardHeight
self.duration = duration
didShowDeadline = Date.currentTimeStamp + Int64(duration)

onRequestAnimation()
onEvent("onKeyboardMoveStart", Float(keyboardHeight) as NSNumber, 1, duration as NSNumber, tag)
Expand All @@ -193,18 +195,22 @@ public class KeyboardMovementObserver: NSObject {
}

@objc func keyboardDidAppear(_ notification: Notification) {
let timestamp = Date.currentTimeStamp
let (duration, frame) = metaDataFromNotification(notification)
if let keyboardFrame = frame {
let (position, _) = keyboardView.framePositionInWindow
let keyboardHeight = keyboardFrame.cgRectValue.size.height
tag = UIResponder.current.reactViewTag
self.keyboardHeight = keyboardHeight
// if the event is caught in between it's highly likely that it could be a "resize" event
// so we just read actual keyboard frame value in this case
let height = timestamp >= didShowDeadline ? keyboardHeight : position
// always limit progress to the maximum possible value
let progress = min(position / self.keyboardHeight, 1.0)
let progress = min(height / self.keyboardHeight, 1.0)

onCancelAnimation()
onEvent("onKeyboardMoveEnd", position as NSNumber, progress as NSNumber, duration as NSNumber, tag)
onNotify("KeyboardController::keyboardDidShow", getEventParams(position, duration))
onEvent("onKeyboardMoveEnd", height as NSNumber, progress as NSNumber, duration as NSNumber, tag)
onNotify("KeyboardController::keyboardDidShow", getEventParams(height, duration))

removeKeyboardWatcher()
setupKVObserver()
Expand Down

0 comments on commit 9b5510d

Please sign in to comment.