fix: do not overwrite external inputAccessoryView on Fabric#48339
Closed
kirillzyusko wants to merge 1 commit intofacebook:mainfrom
Closed
fix: do not overwrite external inputAccessoryView on Fabric#48339kirillzyusko wants to merge 1 commit intofacebook:mainfrom
inputAccessoryView on Fabric#48339kirillzyusko wants to merge 1 commit intofacebook:mainfrom
Conversation
facebook-github-bot
added
CLA Signed
Contributor
Author
|
@cipolleschi sorry to tag you, but I know you do a lot of iOS stuff in react-native, so wanted to kindly ask you to review this PR 🙏 👀 |
cipolleschi
approved these changes
Dec 19, 2024
Contributor
cipolleschi
left a comment
cipolleschi
left a comment
There was a problem hiding this comment.
This fix looks good. Thank you for taking care of it!
Contributor
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Contributor
|
@cipolleschi merged this pull request in 5fc5827. |
Collaborator
|
This pull request was successfully merged by @kirillzyusko in 5fc5827 When will my fix make it into a release? | How to file a pick request? |
robhogan
pushed a commit
that referenced
this pull request
Dec 30, 2024
Summary: If 3rd party libs are using `inputAccessoryView` - the current code can easily break it. Whenever props gets changed we call `setDefaultInputAccessoryView` which will simply overwrite the current `inputAccessoryView` (which is highly undesirable). The same fix on paper was made ~7 years ago: bf36983 ## Changelog: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [IOS] [FIXED] - Fixed problem with accessory view & 3rd party libs For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: #48339 Test Plan: Make sure `inputAccessoryView` functionality works as before Reviewed By: javache Differential Revision: D67451188 Pulled By: cipolleschi fbshipit-source-id: bc3fa82ae15f8acedfd0b4e17bdea69cbd8c8a8d
Collaborator
|
This pull request was successfully merged by @kirillzyusko in d34032b When will my fix make it into a release? | How to file a pick request? |
2 tasks
kirillzyusko
added a commit
to kirillzyusko/react-native-keyboard-controller
that referenced
this pull request
Jan 15, 2025
## 📜 Description Added an ability to specify `offset` for interactive keyboard dismissal on iOS. ## 💡 Motivation and Context In this PR I'm exposing `KeyboardGestureArea` on iOS and adding two props for that: `offset` and `textInputNativeID`. This PR is a re-thinking concept of how we work with `inputAccessoryView` on iOS. To make long text short - default `InputAccessoryView` comes with many restrictions, such as not growing `TextInput`, unability to specify position on the screen, weird animations on unmount, complexity with managing `SafeArea` insets, etc. We already have `KeyboardStickyView` that don't have all that problems, but if you interactively dismiss a keyboard then interactive dismissal starts from a top border of the keyboard (not the input). Taking a step back and utilising `inputAccessoryView` (moving a view from RN hierarchy directly into `inputAccessoryView`) is possible, but comes with a previous set of challenges. In this PR I decided to think about different concepts between iOS/Android and how to make a solution that will work everywhere identically. And the idea is to create an invisible/non-interactable instance of `inputAccessoryView`, that will simply extend the keyboard area (but keyboard-controller will know about that offset and will automatically exclude it from final keyboard dimensions, so you can use everything as you used before). Schematically all process can be shown on a diagram below:  However new approach comes with its own set of challenges. Mainly they come from the fact how keyboard dismissal works on iOS, and in simple words: - when you perform `Keyboard.dismiss()`/press enter then whole combination (keyboard + inputAccessoryView) is treated as a single keyboard and entire element gets hidden in a single animation. - when you perform interactive dismissal, then we have two fold animation - first we dismiss a keyboard, and in second stage we dismiss `inputAccessoryView`. From all the description above it's clear, that we want to ignore `inputAccessoryView` animations or exclude its height from the animation (when its animated as a single element). To solve the first problem (when keyboard dismissed as a single element) we need to remove `inputAccessoryView` and only then perform an animation. Otherwise if we use default hooks `useKeyboardAnimation`/`useReanimatedKeyboardAnimation` that rely on layout animation, then we will see unsynchronized animation (because for example actual keyboard height is 250 + 50, but in JS we give only value of 250, so we will animate from 250 to 0, though actual animation will be from 300 to 0). To fix that I had to swizzle into `resignFirstResponder`. In this method we see, if we have `InvisibleAccessoryView`, then we postpone a keyboard dismissal and remove current `inputAccessoryView`. In this case we will dismiss a keyboard without `inputAccessoryView`, so it will work as it works before. The second main challenge was a time when to remove `inputAccessoryView` during interactive keyboard dismissal. The initial idea was to remove it as soon as dismiss gesture begins. However I rejected this idea in d11afd6 mainly because it was causing a lot of issues (such as ghost animation when keyboard is fully dismissed). When we remove that code it removes additional complexity and we remove `inputAccessoryView` when we call `resignFirstResponder` (happens when keyboard gets dismissed, i. e. first phase passed). In this case it works more predictable. Last but not least - it's wort to note, that the idea with invisible `inputAccessoryView` is not new in iOS community, and some even native projects are utilizing it: https://github.com/iAmrMohamed/AMKeyboardFrameTracker Closes #250 ## 📢 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 --> ### Docs - mention that `KeyboardGestureArea` is not Android specific anymore; - add new `textInputNativeID` description + show how to use it. ### JS - don't exclude `iOS` for `KeyboardGestureArea` in codegen; - expose new `textInputNativeID` property for `KeyboardGestureArea`; - applied patch in fabric example app facebook/react-native#48339 - make `interpolator` optional (will be `ios` on `iOS` and `linear` on `Android`) - make growing/multiline `TextInput` in interactive iOS keyboard example; ### iOS - expose `KeyboardGestureArea` on iOS as well - added `InvisibleInputAccessoryView` class; - added `KeyboardEventsIgnorer` class; - added `KeyboardAreaExtender` class; - added `KeyboardOffsetProvider` class; - added `KeyboardEventsIgnorer` class; - added `UIResponderSwizzle` class; - added `shouldIgnoreKeyboardEvents` event to `Notification`; - added `nativeID` extension to `UIResponder` (and it's mock for a native project); ### Android - added no-op setters for `textInputNativeId`; ## 🤔 How Has This Been Tested? Tested locally on: - iPhone 6s (iOS 15.8, real device); - iPhone 11 (iOS 18.0, iOS 18.1, real device) - iPhone 16 Pro (iOS 18.0, simulator) - iPhone 15 Pro (iOS 17.5, simulator) - iPhone 14 Pro (iOS 16.5, simulator) ## 📸 Screenshots (if appropriate): https://github.com/user-attachments/assets/097d76e1-4f79-4a27-89b7-43a479b6b32b ## 📝 Checklist - [x] CI successfully passed - [x] I added new mocks and corresponding unit-tests if library API was changed
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Summary:
If 3rd party libs are using
inputAccessoryView- the current code can easily break it. Whenever props gets changed we callsetDefaultInputAccessoryViewwhich will simply overwrite the currentinputAccessoryView(which is highly undesirable).The same fix on paper was made ~7 years ago: bf36983
Changelog:
[IOS] [FIXED] - Fixed problem with accessory view & 3rd party libs
Test Plan:
Make sure
inputAccessoryViewfunctionality works as before