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

Refactor keyboard observer on iOS #4360

Merged
merged 2 commits into from
Apr 18, 2023
Merged

Refactor keyboard observer on iOS #4360

merged 2 commits into from
Apr 18, 2023

Conversation

kmagiera
Copy link
Member

Summary

This PR refactors the logic in REAKeyboardEventObserver to address some number of different issues:

  1. no change notification when keyboard size changes due to change in language or showing suggestions bar
  2. issue with keyboard height notification when opening modal (see Add KeyboardAvoidingView #4339)
  3. issue with non-zero height after keyboard hides completely.

Here is the list of the main thing that this refactor changes:

  1. We no longer use willShow/willHide/didShow/didHide notifications for keyboard and only use willChangeFrame which is the most reliable one and triggers also when the keyboard updates
  2. We no longer use a technique where the keyboard size is determined based on the frame of the keyboard view. This was problematic as that frame would sometimes reflect a one-frame delayed size which resulted in the animation not coming down to 0 or not updating at all when immediately hidden or appear (by a modal). Instead, we use UIView animation to animate a complementary view that is placed outside of the screen visible area. We use UIView animation to animate height of that invisible view and then use display link to access presentation layer in order to read up-to-date height.

There are few gatchas related to the new technique used:

  1. Because we only look at frame change notification we need to properly determine the state of the keyboard. We do it based on the target height. If it is 0 then we consider the state to be either closing or closed – we select the final state based on whether there is animation running (closed state when no animation, or closing when we are animating). This prevents us from changing state to closing when keyboard is only becoming shorter (e.g. when text suggestion bar is closed). We handle opening/open state in similar way.
  2. Due to the UIView animation technique we need to check presentationLayer.animationKeys to determine if the layer runs the animation. If it doesn't then we extract height from the view frame and not from the presentationLayer frame, as the presentationLayer may not have an up-to-date information (it will be delayed by one frame).

Test plan

Use example from #4339 and animated keyboard screen from Example app. Open/close text suggestion bar to see how the app reacts when keyboard is changing height and not just opening/closing.

object:nil];
}

[self->_listeners setObject:listener forKey:listenerId];
if (self->_state == UNKNOWN) {
[self recognizeInitialKeyboardState];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the initial keyboard state correct after these changes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We always override the state in keyboardWillChangeFrame before sending any event

@terrysahaidak
Copy link
Contributor

This is awesome @kmagiera!
I'll play around next week with this PR on my branch that uses useAnimatedKeyboard heavily and where all these problems were discovered Expensify/App#16356

Copy link
Member

@tomekzaw tomekzaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I can confirm this PR fixes the issue with missing height update on keyboard change. Here are the logs from toggling between English and Emoji keyboard. Note that for each change, there are exactly two updates in an interval of one frame (16 ms):

  1. As for the issue with opening a modal, it looks like after these changes useAnimatedKeyboard-based KeyboardAvoidingView behaves identically to the one from React Native so I guess this is also solved. Also, here are the logs from opening a modal while keyboard is open:

  1. I can also confirm this PR fixes the issue with non-zero height after keyboard hides completely. Previously, I was able to reproduce it consistently using the following conditional breakpoint:

  1. Because of the shape of the animation curve, sometimes (in particular, at the beginning and at the end of animation) as well as the precision of the measurement, keyboard height doesn't change in up to 5 subsequent frames. In such case (when both height and state don't change), we could skip calling listeners on the native side.
Nagranie.z.ekranu.2023-04-17.o.12.57.38.mov
  1. I know this is outside of scope of this PR but why we need to have two identical listeners in useAnimatedKeyboard.ts?

listenerId.current = subscribeForKeyboardEvents((state, height) => {
'worklet';
keyboardEventData.state.value = state;
keyboardEventData.height.value = height;
}, options);

listenerId.current = subscribeForKeyboardEvents((state, height) => {
'worklet';
keyboardEventData.state.value = state;
keyboardEventData.height.value = height;
}, options);

  1. Unfortunately, KeyboardAvoidingViewExample crashes on reload (Example app, iOS). It's not a regression as it also does crash on the main branch but it's certainly worth looking into, especially since reloading works fine on other examples. Here's the stack trace:

Zrzut ekranu 2023-04-17 o 13 24 15

@kmagiera kmagiera added this pull request to the merge queue Apr 18, 2023
Merged via the queue into main with commit 874d829 Apr 18, 2023
@kmagiera kmagiera deleted the repro-4339 branch April 18, 2023 09:14
fluiddot pushed a commit to wordpress-mobile/react-native-reanimated that referenced this pull request Jun 5, 2023
## Summary

This PR refactors the logic in REAKeyboardEventObserver to address some
number of different issues:
1) no change notification when keyboard size changes due to change in
language or showing suggestions bar
2) issue with keyboard height notification when opening modal (see
software-mansion#4339)
3) issue with non-zero height after keyboard hides completely.

Here is the list of the main thing that this refactor changes:
1) We no longer use willShow/willHide/didShow/didHide notifications for
keyboard and only use willChangeFrame which is the most reliable one and
triggers also when the keyboard updates
2) We no longer use a technique where the keyboard size is determined
based on the frame of the keyboard view. This was problematic as that
frame would sometimes reflect a one-frame delayed size which resulted in
the animation not coming down to 0 or not updating at all when
immediately hidden or appear (by a modal). Instead, we use UIView
animation to animate a complementary view that is placed outside of the
screen visible area. We use UIView animation to animate height of that
invisible view and then use display link to access presentation layer in
order to read up-to-date height.

There are few gatchas related to the new technique used:
1) Because we only look at frame change notification we need to properly
determine the state of the keyboard. We do it based on the target
height. If it is 0 then we consider the state to be either closing or
closed – we select the final state based on whether there is animation
running (closed state when no animation, or closing when we are
animating). This prevents us from changing state to closing when
keyboard is only becoming shorter (e.g. when text suggestion bar is
closed). We handle opening/open state in similar way.
2) Due to the UIView animation technique we need to check
presentationLayer.animationKeys to determine if the layer runs the
animation. If it doesn't then we extract height from the view frame and
not from the presentationLayer frame, as the presentationLayer may not
have an up-to-date information (it will be delayed by one frame).

## Test plan

Use example from software-mansion#4339 and animated keyboard screen from Example app.
Open/close text suggestion bar to see how the app reacts when keyboard
is changing height and not just opening/closing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants