-
-
Notifications
You must be signed in to change notification settings - Fork 87
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
kirillzyusko
merged 11 commits into
main
from
fix/intercept-responder-events-instead-of-keyboard
Jan 21, 2025
Merged
fix: intercept input events along with keyboard events #760
kirillzyusko
merged 11 commits into
main
from
fix/intercept-responder-events-instead-of-keyboard
Jan 21, 2025
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📊 Package size report
|
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
5b8d6c3
to
12a4830
Compare
… keyboard, or when you switch to another input and this input has `showSoftInput={false}`
12a4830
to
4145759
Compare
2 tasks
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📜 Description
Fixed a problem when
focusDidSet
event wasn't emitted/FocusedInputHolder
hold a wrong reference/useReanimatedFocusedInput
returnlayout
asnull
whenTextInput
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
eventsThe 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:
So basically on iOS 15
keyboardWillShow
is emitted beforedidBeginEditing
. As a resultonStart
event doesn't have a layout andKeyboardAwareScrollView
can not perform a scroll into correct position (by specificationuseReanimatedFocusedInput
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:
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
keyboardWillShow
event - let's store a reference, setup KVO, emit event;didBeginEditing
- ignore this event, since it was already handled bykeyboardWillShow
iOS 17
didBeginEditing
event - let's store a reference, setup KVO, emit event;keyboardWillShow
- ignore this event, since it was already handled bydidBeginEditing
Note
This order is handled by
Blur
keyboardWillHide -> blur (when keyboard simply gets closed)
Similar to focus gateway we added:
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, butkeyboardWillHide
is not (and we don't need to sendnoFocusedInput
event during a switch). To handle it I added this code: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:Closes #758
📢 Changelog
iOS
textDidEndEditingNotification
andtextDidBeginEditingNotification
forUITextView
/UITextField
;🤔 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