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.

Sign up for GitHub

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

Jump to bottom

[Fabric] ScrollView + autogrowing TextInput + Reanimated prevents TextInput autogrow #5567

Open
kirillzyusko opened this issue Jan 9, 2024 · 2 comments
Open

[Fabric] ScrollView + autogrowing TextInput + Reanimated prevents TextInput autogrow #5567

kirillzyusko opened this issue Jan 9, 2024 · 2 comments
Labels
Platform: Android This issue is specific to Android Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snippet of code, snack or repo is provided Reproducible 🎉

Comments

@kirillzyusko
Copy link
Contributor

kirillzyusko commented Jan 9, 2024 •
edited
Loading

Description

A combination of ScrollView + autogrowing TextInput and reanimated prevents TextInput from a grow.

Important

I provided a snack link, but it's not reproducible in snack, because I guess it's running old arch. The issue is reproducible in new arch only - I guess you can paste a code into your FabricExample app. But if it's not reproducible on your end - let me know and i'll prepare a repo.

Steps to reproduce

  1. Click on any text input
  2. Press enter button many times

Actual result:

TextInput is not growing

telegram-cloud-document-2-5253502600879293000.mp4

Expected result:

TextInput grows

autogrow-expected.mp4

Snack or a link to a repository

https://snack.expo.dev/@kirylziusko/rea-textinput-autogrow-issue

Reanimated version

3.6.1

React Native version

0.72.4

Platforms

Android, iOS

JavaScript runtime

Hermes

Workflow

React Native

Architecture

Fabric (New Architecture)

Build type

Debug app & dev bundle

Device

Real device, simulator

Device model

  • Pixel 7 Pro (Android 14)
  • Iphone 15 Pro (iOS 17.2, simulator)

Acknowledgements

Yes
@github-actions github-actions bot added Platform: Android This issue is specific to Android Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snippet of code, snack or repo is provided labels Jan 9, 2024
@Latropos
Copy link
Contributor

Latropos commented Jan 9, 2024

@tomekzaw I've tracked this error down and it looks that I've found its origin:

    // Even if only non-layout props are changed, we need to store the update in
    // PropsRegistry anyway so that React doesn't overwrite it in the next
    // render. Currently, only opacity and transform are treated in a special
    // way but backgroundColor, shadowOpacity etc. would get overwritten (see
    // `_propKeysManagedByAnimated_DO_NOT_USE_THIS_IS_BROKEN`).

Indeed if you remove code below this comment then you can use multiline text input as expected.
Also if you change your reproduction code and change the animated style so that it doesn't contain animated transform or animated opacity the bug disappears.

react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.cpp

Line 521 in 4ad3a51

// Even if only non-layout props are changed, we need to store the update in

@Latropos
Copy link
Contributor

Latropos commented Jan 9, 2024

@tomekzaw It seems that this bug is somehow a result of this approach: #4075

@kirillzyusko kirillzyusko mentioned this issue Jan 11, 2024
Merged
1 task
kirillzyusko added a commit to kirillzyusko/react-native-keyboard-controller that referenced this issue Jan 12, 2024
@kirillzyusko
fix: remove wrapper view from KeyboardAwareScrollView (#321)
ec912c0 
## 📜 Description

Removed view wrapper. Attached that view as last child + kept padding
for this view.

## 💡 Motivation and Context

The approach with additional view was added in
#257

I don't remember exact reason, but for me it seems like it was added,
because TExtInputs were not able to grow. However such approach is
causing additional issues:

- breaks styling in some cases
- stickyHeaderIndices={[0]} produces a crash (because RN will try to
attach `Animated`-based style to REA view)

So in this PR I'm removing this view wrapper and apply additional
padding in mostly the same way as it was before
#257
(additional child-view in the end of ScrollView, but instead of `height`
I'm animating `paddingBottom`).

I've tested and it still works (i. e. inputs are growing). E2E tests
also passing (so there seems to be 1px difference, which can be
neglected, but i had to update assets to assure E2E tests consistency).

> [!IMPORTANT]
> This fix will introduce
software-mansion/react-native-reanimated#5567.
I've added that issue as "known issue" in docs page.
I think it's better to have properly working paper architecture and
Fabric can be fixed later (because it's still in early adoption and is
not widely used).

Closes
#325

## 📢 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 -->

### E2E

- added `detox-clean` command (useful when you updated XCode version and
haven't run detox tests yet)

### JS

- removed wrapper and moved view as last child

## 🤔 How Has This Been Tested?

Tested on:
- e2e (Android, iOS - paper)
- Pixel 7 Pro (Android 14, paper)

## 📸 Screenshots (if appropriate):

There is no visual difference 🙂 

## 📝 Checklist

- [x] CI successfully passed
@kirillzyusko kirillzyusko mentioned this issue Feb 23, 2024
Closed
@kirillzyusko kirillzyusko mentioned this issue Dec 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Platform: Android This issue is specific to Android Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snippet of code, snack or repo is provided Reproducible 🎉
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@kirillzyusko @Latropos