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: intercept input events along with keyboard events #760

Merged
merged 11 commits into from
Jan 21, 2025

Conversation

kirillzyusko
Copy link
Owner

@kirillzyusko kirillzyusko commented Jan 15, 2025

📜 Description

Fixed a problem when focusDidSet event wasn't emitted/FocusedInputHolder hold a wrong reference/useReanimatedFocusedInput return layout as null when TextInput has a focus, but keyboard is not shown.

💡 Motivation and Context

Before functionality with layout retrieval was toughly coupled with keyboard events. And it worked - if keyboard is shown, then there is 100% probability that input has a focus. However we may have a different situation, when input has a focus but keyboard is not shown (showSoftInputFocus={false}/physical keyboard connected). Of course "focused input" functionality should work independently of keyboard events.

1️⃣ Naive approach - let's rely on didBeginEditing/didEndEditing events

The first native approach was to rely on focus/blur events. It works well for iOS 16, 17, 18. But thanks to our e2e tests they caught a bug on iOS 15. The problem can be shown schematically as:

- iOS 15: keyboardWillShow -> didReceiveFocus, keyboardWillHide (not fired when we switch) -> didReceiveBlur
- iOS 17: didReceiveFocus -> keyboardWillShow, keyboardWillHide (not fired when we switch) -> didReceiveBlur

So basically on iOS 15 keyboardWillShow is emitted before didBeginEditing. As a result onStart event doesn't have a layout and KeyboardAwareScrollView can not perform a scroll into correct position (by specification useReanimatedFocusedInput should provide an information before or along with keyboard events).

Last, but not least - after further investigation I discovered, that when blur/hide happens we may have several chain of events:

  • keyboardWillHide -> blur (when keyboard simply gets closed);
  • blur (when keyboard stays on screen and you switch between inputs);
  • blur -> keyboardWillHide (when you focus to the next input, but next input has showSoftInputFocus={false})

So to sum up: we may have a random order of events, but should handle all of them properly.

So what should we do?

2️⃣ Gateway approach

The second approach I've been thinking of is a creation of a gateway for events. I. e. we will listen for all events simultaneously, but will handle only first "focus"/"blur" events.

How it works:

Focus

iOS 15
  • we receive keyboardWillShow event - let's store a reference, setup KVO, emit event;
  • we receive didBeginEditing - ignore this event, since it was already handled by keyboardWillShow
iOS 17
  • we receive didBeginEditing event - let's store a reference, setup KVO, emit event;
  • we receive keyboardWillShow - ignore this event, since it was already handled by didBeginEditing

Note

This order is handled by

   if UIResponder.current == currentResponder {
     // focus was already handled by keyboard event
     return
   }

Blur

keyboardWillHide -> blur (when keyboard simply gets closed)

Similar to focus gateway we added:

    if currentResponder == nil {
      // blur was already handled by keyboard event
      return
    }
blur (when keyboard stays on screen and you switch between inputs)

However we have another small problem - when we switch between inputs blur event is emitted, but keyboardWillHide is not (and we don't need to send noFocusedInput event during a switch). To handle it I added this code:

    DispatchQueue.main.async {
      // check that it wasn't a switch between inputs
      if self.currentResponder == nil {
        self.onBlur()
      }
    }

We schedule event sending asynchronously, and wait. If we receive subsequent "focus" event, then we ignore that event.

blur -> keyboardWillHide (when you focus to the next input, but next input has showSoftInputFocus={false})

Last thing to consider. To fix this basically when we dispatch onBlur we need to check, that we indeed don't have a first responder in chain. To do this I added a check:

    if UIResponder.current is TextInput {
      return
    }

Closes #758

📢 Changelog

iOS

  • added listeners for textDidEndEditingNotification and textDidBeginEditingNotification for UITextView/UITextField;
  • implemented a gateway approach for managing potentially duplicated events.

🤔 How Has This Been Tested?

Tested on iPhone 16 Pro iOS 18.1 + e2e tests for iOS 17, 16, 15.

📸 Screenshots (if appropriate):

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2025-01-21.at.11.45.20.mp4

📝 Checklist

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

Copy link
Contributor

github-actions bot commented Jan 15, 2025

📊 Package size report

Current size Target Size Difference
168038 bytes 167703 bytes 335 bytes 📈

@kirillzyusko kirillzyusko changed the title fix: intercept responder events instead of keyboard events fix: intercept input events instead of keyboard events Jan 17, 2025
@kirillzyusko kirillzyusko changed the title fix: intercept input events instead of keyboard events fix: intercept input events along with keyboard events Jan 19, 2025
@kirillzyusko kirillzyusko self-assigned this Jan 19, 2025
@kirillzyusko kirillzyusko added 🍎 iOS iOS specific focused input 📝 Anything about focused input functionality labels Jan 19, 2025
@kirillzyusko kirillzyusko mentioned this pull request Jan 20, 2025
2 tasks
kirillzyusko added a commit that referenced this pull request Jan 20, 2025
## 📜 Description

Make `useFocusedInputHandler` compatible with iOS < 13.

## 💡 Motivation and Context

The library was always compatible with really old iOS versions. However
there was a one handler (`onSelectionChange`) which was a stub on iOS <
13.

In this PR I'm adding support for old iOS versions for this handler via
`KVO` approach + dispatching events on text changes.

The new algorithm can be breaking into next steps:
- on saving a new reference to delegate we remove old KVO;
- on saving a new reference to delegate we add KVO for UITextField (iOS
< 13 only);
- we dispatch selection event from KVO/change text events (to be fully
backward comaptible with iOS 13+, because single KVO don't emit events
for text changes - only selection);
- when text editing has been completed - we remove KVO.

One thing I potentially don't really like a lot is that we remove KVO in
`endEditing` event. Starting from
#760
we will return delegate to its original variant on `endEditing` event,
but it seems like we'll have kind of transitive dependencies (the fact
that we receive this event in two places). However at the moment I can
not imagine a situation when we will not substitute delegate back when
we finish text editing, so theoretically this solution should be okay.

Closes
#763

## 📢 Changelog

<!-- High level overview of important changes -->
<!-- For example: fixed status bar manipulation; added new types
declarations; -->
<!-- If your changes don't affect one of platform/language below - then
remove this platform/language -->

### iOS

- `setTextFieldDelegate` accepts second `textField` param; 
- store `observedTextFieldForSelection` in composite delegate class;
- added KVO helpers function (to assure we can remove it only one time);
- remove KVO when new delegate is set;
- add new KVO when new delegate is set;
- send selection events from text change/kvo;

## 🤔 How Has This Been Tested?

Tested on macOS 12 with XCode 14 and iPhone X iOS 12.4 simulator.

## 📸 Screenshots (if appropriate):


https://github.com/user-attachments/assets/98d01bae-0d58-42e7-9658-ff382b692087

## 📝 Checklist

- [x] CI successfully passed
- [x] I added new mocks and corresponding unit-tests if library API was
changed
@kirillzyusko kirillzyusko force-pushed the fix/intercept-responder-events-instead-of-keyboard branch from 5b8d6c3 to 12a4830 Compare January 20, 2025 10:31
@kirillzyusko kirillzyusko force-pushed the fix/intercept-responder-events-instead-of-keyboard branch from 12a4830 to 4145759 Compare January 21, 2025 10:02
@kirillzyusko kirillzyusko marked this pull request as ready for review January 21, 2025 10:26
@kirillzyusko kirillzyusko merged commit db0d0cc into main Jan 21, 2025
15 checks passed
@kirillzyusko kirillzyusko deleted the fix/intercept-responder-events-instead-of-keyboard branch January 21, 2025 10:55
kirillzyusko added a commit that referenced this pull request Feb 7, 2025
## 📜 Description

Fixed a problem when input looses a focus, but `FocusedInputObserver`
doesn't send `noFocusedInputEvent` to JS.

## 💡 Motivation and Context

The problem was introduced in this PR
#760
before we were relying on keyboard evens, but in #760 I re-worked the
approach and started to use blur/focus events. However I couldn't use
only input events, because on iOS 15 keyboard event (`keyboardWillShow`)
was arriving earlier than focus event, and for us it's crucial to know
the layout of focused input inside `onStart` (i. e. before keyboard
movement starts).

I decided to keep backward compatibility and keep keyboard events in
place. However it comes with own set of challenges. For example:

<img width="829" alt="image"
src="https://github.com/user-attachments/assets/0b718794-39a7-4f45-a089-04d92d9a7688"
/>

This check works well for Paper, but doesn't work for Fabric. And for
focus events we can keep two event sources, because logic for focus is
simple -> "we got a focus -> save the state and push event to JS". For
blur it's different:
- keyboard can be closed but focus still can be present;
- when we switch between inputs we also receive `blur` event but we
should ignore it;
- we may have a random order of events including some missing events.

Initially I don't know how to fix it. But then I realized, that on
Android we already have a similar situation. We can looses a focus and
keyboard will be focus, we can have a focus and close a keyboard, so
overall it's a very similar to what we can have on iOS right now. But
why I don't experience any problems on Android? Because I just rely on
"focus"/"blur" events there (and actually relying on `keyboardWillShow`
on iOS is needed to have a stable order of events in JS). So at this
point of time I thought "what if I remove `keyboardWillHide` handler?".
Yes, it will not work as before (some events may be delayed), but for
dismissing keyboard it's not so important at the moment (because on
Android we can also have any order of those events).

So after thinking a lot I decided that it can be safe to remove that
handler completely and reduce logic complexity presented in this class.

Closes
#798

## 📢 Changelog

<!-- High level overview of important changes -->
<!-- For example: fixed status bar manipulation; added new types
declarations; -->
<!-- If your changes don't affect one of platform/language below - then
remove this platform/language -->

### iOS

- rely on `blur` event only for sending `noFocusedInputEvent` (remove
`keyboardWillHide` event);
- check in `blur` event `UIResponder.current` presence instead of
`self.currentResponder`.

## 🤔 How Has This Been Tested?

Tested manually on iPhone 15 Pro iOS 17.5

## 📸 Screenshots (if appropriate):


https://github.com/user-attachments/assets/471afd00-9fdb-4fca-b159-985203baab1d

## 📝 Checklist

- [x] CI successfully passed
- [x] I added new mocks and corresponding unit-tests if library API was
changed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focused input 📝 Anything about focused input functionality 🍎 iOS iOS specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

focusDidSet is not called when TextInput has showSoftInputOnFocus={false}
1 participant