-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Feature: Improvements to automaticallyAdjustKeyboardInsets
#37766
Feature: Improvements to automaticallyAdjustKeyboardInsets
#37766
Conversation
* main: (1824 commits) Convert FallbackJSBundleLoaderTest to Kotlin (facebook#37750) Migrate JSPackagerClientTest to Kotlin (facebook#37747) (refactor): kotlin-ify `ShareModuleTest.java` (facebook#37746) Upgrade `@react-native/codegen-typescript-test`'s Jest dependency to Jest 29 (facebook#37745) Move flow-typed definitions to repo root (facebook#37636) Convert InterpolatorTypeTest to Kotlin (facebook#37724) Update documentation of ReactHost.reload method (facebook#37691) Reduce visibility of ReactHost.destroy() method (facebook#37693) Reduce visibility in React Context (facebook#37695) Remove InstanceHandleHelper as unused (facebook#37740) Convert CompositeReactPackageTest to Kotlin (facebook#37734) Add license header to SetSpanOperation.java Convert FakeEventDispatcher to kotlin (facebook#37739) Convert FakeRCTEventEmitter to Kotlin (facebook#37733) Convert InteropModuleRegistryTest to Kotlin (facebook#37735) Bump `autorebase.yml` to `v1.8` (facebook#37584) Convert StackTraceHelperTest to Kotlin (facebook#37741) Convert BlobModuleTest class to Kotlin (facebook#37719) Update prettier to v2.8.8 (facebook#37738) Introduce BoltsFutureTask class to avoid leaking bolts.Task into ReactHost API (facebook#37744) ... # Conflicts: # BUCK # packages/react-native/React/Views/UIResponder+FirstResponder.h # packages/react-native/React/Views/UIResponder+FirstResponder.m
Hi @adamaveray! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
Why can't |
@javache Thanks for reviewing and for your feedback. This PR was initially put together by another contributor so I wasn't aware of the private API usage. I've overhauled how it's all implemented now to avoid that and to remove the niche |
* main: (135 commits) translation auto-update for i18n/twilight.config.json on master Interop: Introduce Bridge proxy Remove okhttp internal util usage (facebook#37843) Update debian to fix CI while updating Node (facebook#37841) fix: foreground ripple crash on api < 23 (facebook#37901) Re-add the top level LICENSE file (facebook#37916) Deploy 0.209.0 to xplat (facebook#37921) Re-enable direct debugging with JSC on iOS 16.4+ (facebook#37914) add emitObjectProp in parser primitives (facebook#37904) Make React-utils its own pod (facebook#37659) feat: allow custom assignment of rootView to rootViewController (facebook#37873) Switch xplat prettier config to hermes plugin (facebook#37915) Set iOS AppState to inactive when app is launching (facebook#37690) Use `fileExists` in replace_hermes script (facebook#37911) (docs): fix license url (facebook#37909) Revert D46719890: Re-enable direct debugging with JSC on iOS 16.4+ Re-enable direct debugging with JSC on iOS 16.4+ (facebook#37874) Fix component type references in xplat (facebook#37903) Remove usage of passthroughAnimatedPropExplicitValues in ScrollViewStickyHeader (facebook#37867) test runtime lifecycle callback (facebook#37897) ...
Base commit: 9faf256 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@philIip Any concerns with this? You mentioned some changes coming to keyboard layout events with iOS17.
packages/react-native/Libraries/Text/TextInput/RCTBaseTextInputView.m
Outdated
Show resolved
Hide resolved
packages/react-native/Libraries/Text/TextInput/RCTBaseTextInputView.m
Outdated
Show resolved
Hide resolved
packages/react-native/Libraries/Text/TextInput/RCTBaseTextInputView.m
Outdated
Show resolved
Hide resolved
@javache Thanks for the additional review; I've made the requested changes and also done some additional testing & updates to more closely replicate the native bottom keyboard offset amount (which I initially thought was due to an empty selection but have since found it's actually single line aka UITextField vs multiline aka UITextView). Please let me know if you have any more feedback. |
@javache Just checking in to see if there are any other issues you can see with this PR that are standing in the way of it potentially being merged? |
packages/react-native/Libraries/Text/TextInput/RCTBaseTextInputView.m
Outdated
Show resolved
Hide resolved
Thanks for adding the example (and sorry for the delay)! This mostly looks good to me, but would like to clarify what the coordinates used are relative to. |
Thanks for following up! I've implemented those requests & suggestions, let me know if you have any other feedback 🙌 |
This looks good to merge now, but there are conflicts with main in |
* main: (1012 commits) Add simple constructor for JSError (facebook#39415) Breaking: per-node pointScaleFactor (facebook#39403) Implement "tickleJS" for Hermes (facebook#39289) Add thread idle indicator (facebook#39206) Unblock `yarn android` on main (facebook#39413) Remove Codegen buck-oss scripts as they're unused (facebook#39422) Immediately dispatch events to the shared C++ infrastructure to support interruptability (facebook#39380) Fix race condition in Binding::reportMount (facebook#39420) Clone free state progression (facebook#39357) fix: return the correct default trait collection (facebook#38214) Read the React Native version and set the new arch flag properly (facebook#39388) Deprecate default_flags in Podfile (facebook#39389) Create Helper to read the package.json from Cocoapods (facebook#39390) Create helper to enforce the New Arch enabled for double released (facebook#39387) Remove layoutContext Drilling (facebook#39401) Remove JNI Binding usage of layoutContext (facebook#39402) Extract isBaselineLayout() (facebook#39400) Refactor and separate layout affected nodes react marker (facebook#39249) Bump AGP to 8.1.1 (facebook#39392) Fix broken Gradle Sync when opening the project with Android Studio (facebook#39412) ...
@javache The merge conflicts should be resolved now. Let me know if anything else needs updating! |
@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Thanks for contributing! It'd be great if you could make this feature compatible with the new architecture too. |
…6732) Summary: Fixes #46595 . It seems #37766 broke the `automaticallyAdjustKeyboardInsets` when input accessory view become first responder. ## Changelog: [IOS] [FIXED] - automaticallyAdjustKeyboardInsets not shifting scrollview content Pull Request resolved: #46732 Test Plan: Repro please see in ##46595 . Reviewed By: cipolleschi Differential Revision: D65072478 Pulled By: javache fbshipit-source-id: 7d5d7566438d4bb0e1d50074a953b18866e324d3
…6732) Summary: Fixes #46595 . It seems #37766 broke the `automaticallyAdjustKeyboardInsets` when input accessory view become first responder. [IOS] [FIXED] - automaticallyAdjustKeyboardInsets not shifting scrollview content Pull Request resolved: #46732 Test Plan: Repro please see in ##46595 . Reviewed By: cipolleschi Differential Revision: D65072478 Pulled By: javache fbshipit-source-id: 7d5d7566438d4bb0e1d50074a953b18866e324d3
Summary:
This is a reopened version of #35224 by @isidoro98 which was closed without explanation, updated to resolve new merge conflicts and now includes an example in the RN-Tester app. Aside from that it is unchanged. Here is @isidoro98's description from their original PR:
This PR builds on top of #31402, which introduced the
automaticallyAdjustsScrollIndicatorInsets
functionality. It aims to fix one of RN's longstanding pain point regarding the keyboard.The changes provide a better way of handling the
ScrollView
offset when a keyboard opens. Currently, when a keyboard opens we apply an offset to theScrollview
that matches the size of the keyboard. This approach is great if we are using anInputAccessoryView
but if we have multipleTextInputs
in aScrollView
; offsetting the content by the size of the keyboard doesn't yield the best user experience.Changelog:
[iOS] [Changed] - Scroll
ScrollView
text fields into view withautomaticallyAdjustsScrollIndicatorInsets
Test Plan:
The videos below compare the current and proposed behaviors for the following code:
As can be seen in the video, the current behavior applies an offset to the
ScrollView
content regardless of where theTextInput
sits on the screen.The proposal checks if the
TextInput
will be covered by the keyboard, and only then applies an offset. The offset applied is not the full size of the keyboard but instead only the required amount so that theTextInput
is a specific distance above the top of the keyboard (customizable using the newbottomKeyboardOffset
prop). This achieves a less "jumpy" experience for the user.The proposal doesn't change the behavior of the
ScrollView
offset when anInputAccessory
view is used, since it checks if theTextField
that triggered the keyboard is a descendant of theScrollView
or not.Why not use other existing solutions?
RN ecosystem offers other alternatives for dealing with a keyboard inside a ScrollView, such as a
KeyboardAvoidingView
or using third party libraries likereact-native-keyboard-aware-scroll-view
. But as shown in the recordings below, these solutions don't provide the smoothness or behavior that can be achieved withautomaticallyAdjustsScrollIndicatorInsets
.As shown in the videos, the
TextInput
is hidden by the keyboard for a split second before becoming visible.Code for the videos above: