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

Feature: ScrollView automaticallyAdjustKeyboardInsets (#31402) for 0.66 Stable branch #32725

Closed
wants to merge 1 commit into from

Conversation

arunim2405
Copy link

Helps bring #31402 to 0.66 version of react native

Summary:
Retrying D30015799 (facebook@6e903b0) with a fix where ScrollViewNativeComponent was missing the automaticallyAdjustKeyboardInsets prop.
----- Original Summary
Currently, ScrollViews provide the prop `keyboardDismissMode` which lets you choose `"interactive"`. However when the keyboard is shown, it will be rendered above the ScrollView, potentially blocking content.

With the `automaticallyAdjustKeyboardInsets` prop the ScrollView will automatically adjust it's `contentInset`, `scrollIndicatorInsets` and `contentOffset` (scroll Y) props to push the content up so nothing gets blocked.

* The animation curve and duration of the Keyboard is exactly matched.
* The absolute position of the ScrollView is respected, so if the Keyboard only overlaps 10 pixels of the ScrollView, it will only get inset by 10 pixels.
* By respecting the absolute position on screen, this automatically makes it fully compatible with phones with notches (custom safe areas)
* By using the keyboard frame, this also works for different sized keyboards and even `<InputAccessoryView>`s
* This also supports `maintainVisibleContentPosition` and `autoscrollToTopThreshold`.
* I also fixed an issue with the `maintainVisibleContentPosition` (`autoscrollToTopThreshold`) prop(s), so they behave more reliably when `contentInset`s are applied. (This makes automatically scrolling to new items fully compatible with `automaticallyAdjustKeyboardInsets`)

## Changelog

* [iOS] [Added] - ScrollView: `automaticallyAdjustKeyboardInsets` prop: Automatically animate `contentInset`, `scrollIndicatorInsets` and `contentOffset` (scroll Y) to avoid the Keyboard. (respecting absolute position on screen and safe-areas)
* [iOS] [Fixed] - ScrollView: Respect `contentInset` when animating new items with `autoscrollToTopThreshold`, make `automaticallyAdjustKeyboardInsets` work with `autoscrollToTopThreshold` (includes vertical, vertical-inverted, horizontal and horizontal-inverted ScrollViews)

Pull Request resolved: facebook#31402

Test Plan:
<table>
<tr>
<th>Before</th>
<th>After</th>
</tr>
<tr>
<td>

https://user-images.githubusercontent.com/15199031/115708680-9700aa80-a370-11eb-8016-e75d81a92cd7.MP4

</td>

<td>

https://user-images.githubusercontent.com/15199031/115708699-9b2cc800-a370-11eb-976f-c4010cd96d55.MP4

</td>
</table>

### "Why not just use `<KeyboardAvoidingView>`?"

<table>
<tr>
<th>Before (with <code>&lt;KeyboardAvoidingView&gt;</code>)</th>
<th>After (with <code>automaticallyAdjustKeyboardInsets</code>)</th>
</tr>
<tr>
<td>

https://user-images.githubusercontent.com/15199031/115708749-abdd3e00-a370-11eb-8e09-a27ffaef12b8.MP4

</td>

<td>

https://user-images.githubusercontent.com/15199031/115708777-b3044c00-a370-11eb-9b7a-e040ccb3ef8c.MP4

</td>
</table>

> Also notice how the `<KeyboardAvoidingView>` does not match the animation curve of the Keyboard

### Usage

```jsx
export const ChatPage = ({
  flatListProps,
  textInputProps
}: Props): React.ReactElement => (
  <>
    <FlatList
      {...flatListProps}
      keyboardDismissMode="interactive"
      automaticallyAdjustContentInsets={false}
      contentInsetAdjustmentBehavior="never"
      maintainVisibleContentPosition={{ minIndexForVisible: 0, autoscrollToTopThreshold: 100 }}
      automaticallyAdjustKeyboardInsets={true}
    />
    <InputAccessoryView backgroundColor={colors.white}>
      <ChatInput {...textInputProps} />
    </InputAccessoryView>
  </>
);
```

## Related Issues

* Fixes facebook#31394
* Fixes facebook#13073

Reviewed By: yungsters

Differential Revision: D32578661

Pulled By: sota000

fbshipit-source-id: 45985e2844275fe96304eccfd1901907dc4f9279
@facebook-github-bot
Copy link
Contributor

Hi @arunim2405!

Thank you for your pull request and welcome to our community.

Action Required

In 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.

Process

In 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 CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@pull-bot
Copy link

pull-bot commented Dec 9, 2021

Warnings
⚠️

❔ Base Branch - The base branch for this PR is something other than main. Are you sure you want to merge these changes into a stable release? If you are interested in backporting updates to an older release, the suggested approach is to land those changes on main first and then cherry-pick the commits into the branch for that release. The Releases Guide has more information.

Messages
📖

📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

📖 📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.
📖 📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.

Generated by 🚫 dangerJS against 1810f4d

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: d7768a5

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,942,964 +649,530
android hermes armeabi-v7a 8,477,864 +853,923
android hermes x86 9,361,974 +597,893
android hermes x86_64 9,329,210 +626,182
android jsc arm64-v8a 10,626,457 +944,636
android jsc armeabi-v7a 9,548,872 +879,858
android jsc x86 10,639,758 +1,001,783
android jsc x86_64 11,250,670 +1,016,154

Base commit: d7768a5

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 9, 2021
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@mikehardy
Copy link
Contributor

@arunim2405 I love the enthusiasm, but this commit is a) a new feature (warning! warning! landing new features on stable branches is against generally recognized release management policies) and b) landed on main only 6 days ago and hasn't even seen use in the 0.67-rc releases (warning! warning! has not seen wide adoption to determine if there are corner cases etc - will almost certainly require follow-on PRs if past predicts future)

proposing untested new features for a stable branch is a bit bold, in my opinion at least.
for 0.67 though perhaps, though I think rc5 was intended to be a "last call" on that one, meaning 0.68 is most likely for this.
That said, I think you get "commitlies" from these builds and can now depend on the commitly release? That is actually fantastic as now you personally should have the feature you want in a release build format and you can provide testing feedback.

@lunaleaps is the commitly feature working for these? I seem to recall it may not be, but PR-to-release-branch commitly testing would be a huge deal I think

@kelset
Copy link
Contributor

kelset commented Dec 9, 2021

I agree with @mikehardy, this being a new feature means that it should not go in a patch release ✋

@lunaleaps
Copy link
Contributor

lunaleaps commented Dec 9, 2021

Thanks @mikehardy and @kelset for your input! Let's not add this to 0.66.4 for now (esp. since we're going into holidays). @arunim2405 We hope to get 0.67 out by latest 2nd week of 2022 so cut for 0.68 will have this and be tested.

And totally agree about commitlies.. I will be looking into this after I release 0.66.4. Maybe I can trigger one for this one when I figure out why

@lunaleaps lunaleaps closed this Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants