Fix RN bug where email input is not scrollable

[arielgreen]

This issue's PR should be submitted against our RN fork (Expensify/react-native)

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Note: this issue replaces #2726 with a broadened scope and higher price.

This issue's PR should be submitted against our RN fork (https://github.com/Expensify/react-native).

Expected Result:

Email input is scrollable when there's a long email.

Actual Result:

Due to a bug in React Native, input is not scrollable.

Action Performed:

1. Open expensify chat app
2. On the log in screen, enter a long email
3. Try to scroll the email and edit.

Workaround:

No workaround found for editing the email. Just deleting it and rewriting

Platform:

Where is this issue occurring?

Web
iOS ✔️
Android
Desktop App
Mobile Web

Version Number: 1.0.39-0

Notes/Photos/Videos:

Bug5056652_06052021_Ipad.mp4

View all open jobs on Upwork

Upwork job posting: https://www.upwork.com/jobs/~01a6100ce00489ac44

[MelvinBot]

Triggered auto assignment to @iwiznia (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[pranshuchittora]

Edited_20210518_163056.mp4
Dear sir,
I create a custom input field (you can see on this video).
I also customise as you wish.

Looks like this demo is from Android

[iwiznia]

@Naiem1808015 what we ask here is to provide a write up of the solution you are going to implement, can you do that instead of showing a video of the implementation?

[parasharrajat]

Just a heads up. I think the requirement is to This issue's PR should be submitted against our RN fork (Expensify/react-native). Implementing a custom input is already discussed here #2726.

As a hint, there are a couple of issues on the original RN repo about this abrupt behviour.

So I suggest we bold it on the description so it is not missed.

[bekazandukeli]

Hello friends, I researched about this issue, wrote proposal and showed solution in #2726 discussion.

I offer my service again now:

I will create a separate component for this exact email input. I'll manipulate style so visual will be EXACTLY THE SAME but this bug which is tightly connected to styling(because of RN) will not occur.

Best regards,
Beka Zandukeli

[iwiznia]

What we are looking for here is to fix the bug in react native itself (well, really sent to Expensify/react-native which is our fork), not to create a new component.

[devansh-sudo]

<ScrollView style={{flex:1, backgroundColor='#ffffff'}}>


<TextInput style={styles.inputText}
value={this.state.mail}
placeholder="Email"
onChangeText={(text)=>this.setState.(mail:text)}/>
<TextInput style={styles.inputText}
value={this.state.fName}
placeholder="First name"
onChangeText={(text)=>this.setState.(fName:text)}/>
<TextInput style={styles.inputText}
value={this.state.sName}
placeholder="Second name"
onChangeText={(text)=>this.setState.(sName:text)}/>

[isagoico]

Issue reproducible during today's KI retests

[arielgreen]

Reposted here.

[kidroca]

@arielgreen @iwiznia @mallenexpensify
Cannot recreate this on dev. Tried both emulator and Physical iPhone.
Is some workaround applied? Looks like not
Is this happening on a specific model or iOS version?

Cannot recreate on DEV

Screen.Recording.2021-06-04.at.11.48.02.mov

[Viacheslav80]

Hi! what do you think about this:

Screen.Recording.2021-06-04.at.18.56.20.mov

[Viacheslav80]

@kidroca Hi! The problem was reproduced on the simulator iphone X (iOS 13.7)

[Viacheslav80]

This is quite funny, but if you change the height of TextInput > 40 then the problem is no longer reproduced
I used variables.componentSizeLarge instead of componentSizeNormal in
/src/styles/styles.js

[iwiznia]

This is quite funny, but if you change the height of TextInput > 40 then the problem is no longer reproduced

We know that (check the other issue for context). The idea for this issue is to actually fix the broken behavior without changing the height.

[arielgreen]

Raised price, posted in #expensify-open-source.

[Santhosh-Sellavel]

Hi,

I propose we remove the following in textInput style

paddingTop: 10, paddingBottom: 10

since we use
textAlignVertical: 'center',

there won't be any alignment Issue also.

After removing those the component works as expected.

I was able to reproduce the bug on the simulator with version iOS 13.7. On the latest version, iOS 14.5 the issue was unable to reproduce.

Check the screen recording for the same

Without paddingTop and paddingBottom it works fine.

Simulator.Screen.Recording.-.NewiPhoneX.-.2021-06-12.at.03.08.24.mp4

But it doesn’t with padding.

Simulator.Screen.Recording.-.NewiPhoneX.-.2021-06-12.at.03.21.38.mp4

[iwiznia]

@Santhosh-Sellavel that's a workaround, but we are looking for a fix in the library and not a workaround.

[parasharrajat]

I don't think it's apple's issue. Can't you scroll on any native IOS app?

[dklymenk]

I don't think it's apple's issue. Can't you scroll on any native IOS app?

This issue is highly specific in terms of iOS version and even device. The most modern environment I personally managed to reproduce the issue in is iPhone8 running iOS 12.4. You also need to break specific thresholds in terms of fontSize and lineHeight, that are different for each font family, to reproduce it.

Anyway, I think I have sufficient proof that the issue is on the side of Apple's UIKit proprietary framework and not react native.

After I had reproduced the issue on Expensify.cash repo, I started with a blank react native project and added two TextInputs to it. One with style={{ fontSize: 28, lineHeight: 32 }} and another one with style={{ fontSize: 28, lineHeight: 64 }}. First one was the one where the issue was observed and the second one was the one with no issue.

I'll skip over the part where I was debugging and staring at react native objective C code for more time than I'd like to admit. I didn't find anything useful there; all of the font related values are either passed directly to UIKit's objects or in the cases where there was some math going on, it was executed in the same manner for both my faulty component and my working component.

This was where I was very certain that I couldn't fix the issue in react native, since there is no possible place for this issue to even exist and obviously there was no code that breaks component if iOS version is less than 13.0 or something like that.

However, to prove that issue is indeed in UIKit and not* in some obscure react native code that I missed, I had to come up with a demo project in objective C that demonstrates the issue, preferably in as few clicks as possible.

Firstly, I need to mention that the react native TextInput component with prop multiline={false} after a long chain of nested views and inheritance uses UIKit's UITextField object. You can add that object to your own iOS app project in xcode.

Secondly, in order for the demo to make sense, I need to explain what I have learned about the bug, since UIKit is a black box for the most part (all you have are documentation, header files and compiled sources), all I can share are my observations. So, the bug appears when lineHeight minus fontSize are higher than a certain value. This value is different for every font family and may vary greatly.

Finally, to prove that the issue is not in react native I came up with 2 projects. Each project has one main view with 2 inputs: a good input and a bad input. All the inputs will use the same font family and the same line height. The idea is to show that not only the bug is reproducible without react native, but also that the threshold where a good input becomes a bad input is the same.

So in these videos below you will see me demonstrating two identical sets of inputs with two identical behaviors in objectiveC and react native.

RN:

2973-RN-more-compressed.mp4

objectiveC:

2973-objectiveC-more-compressed.mp4

Here are links to project repositories:
https://github.com/dklymenk/ios-textinput-scrolling-rn
https://github.com/dklymenk/ios-textinput-scrolling-objectivec

Please let me know if I need to add instructions to those projects or you have any issues starting them or reproducing the behavior I demonstrate on videos.

Hopefully, this qualifies as sufficient proof.

Thanks.

P.S. Sorry for artifacts in the beginning of the videos, I had to compress them a quite a bit more that usual for github to accept them.

EDIT: Made a few corrections: fixed words missing mid-sentence and reference to multiline prop

[parasharrajat]

Nice job, But I suggest keeping the post small and straight for readability purposes.

[iwiznia]

Oh wow, great investigation @dklymenk!!! I think that's enough proof to show the problem is not fixable in react native. Let's go ahead and implement a workaround for it from the time being. Which workaround specifically would you recommend we use?
Also, does anyone know if there's a way to report the issue to Apple?
@arielgreen go ahead and hire @dklymenk.

[parasharrajat]

So everyone must be facing this issue. I think it could be a good opportunity to patch logic in RN textinput.

[dklymenk]

@iwiznia Actually I have found this little checkbox in xcode that says "Adjust to Fit". Enabling it seems to resolve the issue. It makes it so if the text doesn't fit the width, the font size is reduced down to a certain value specified in a different property called "Min font size" in a GUI. Setting that "Min font size" value to be at least 1 less than original font size seems to do the trick.

Please see this video.

Screen.Recording.2021-06-25.at.22.42.29.mov

I personally don't like the way it looks. It is too spiky. See last few seconds where I delete and add a character that triggers a resize. However, I feel like I have to offer a workaround that involves setting that property of UITextView in one of a few ways.

Option 1

I set that property to true and set min font size in class constructor automatically, if current version of iOS is among the ones where bug is reproducible.

Pros:

• no changes required in Expensify.cash repo
• should be easier to implement than option 2

Cons:

• doesn't allow customizability
• every user with iOS of affected version will observe the spike every time they reach the end of text input, even when that is not required

Option 2

I expose those properties as iOS only props for TextInput so you can manage them in JS.

Pros:

• you are free not to use it
• full control over values

Cons:

• when enabled, all iOS users will have to endure the spike
• requires changes to Expensify.cash repo (to every single-line text input you have), unless I make true the default value
• way more work

I actually don't recommend either of these since they are not just workarounds, but also visually unappealing workarounds. If I were to choose a workaround I would pick this one, since it's just one line in one file and the visual difference is barely distinguishable by the naked eye. Also after the affected iOS versions get phased out by apple, you will be able to delete that line and not deal with any potential conflicts in react native.

If you are going to choose one of my options above anyway, I should warn you that I probably won't be able to implement them on short notice.

[iwiznia]

Thanks for all the investigations and details. I agree with you that those 2 options are not great and that we should go with changing the input height to a size where the bug is not present.
Ideally we would not have to make this change in every input. I think we don't have to, because they all are using this. So let's update this to 41 which according to that comment you linked should fix this.... but before doing that, I assume we can't do something like define it as 40.01 or something like that and have the bug fixed too? Basically, the smallest amount of change we can do, the better.

[dklymenk]

I'm sorry, but I'm not following. You said "Ideally we would not have to make this change in every input." and then suggested that instead of changing the value that affects only text inputs, we change a variable that is used 18 times in styles/styles.js and will affect many different components: avatars, menu items, buttons, pickers, etc. What exactly do you mean here?

Anyway, I have conducted a few tests around the app to check if it's really only that one text input on login page that is affected. As far as I can tell, every text input in the app that is not multiline or a secure-input (aka password) one is affected. This means that there are way more affected inputs than non-affected ones, so, from my point of view, applying the change to all text inputs is justified. If we were to apply the fix to only inputs that require it, I would still apply the global fix suggested here and then overwrite the height in that multiline message compose and those 5 or so password inputs across the app with some new style object, but I don't believe it is necessary and the one-line fix I linked should be fine.

Regarding whether or not a height of 40.01 would be enough fix the issue. In my testing the value I put there is ultimately treated as an integer. The issue was reproducible until I got to 40.5001. So, I'm still confident we should just add 1 to the height, as suggested in the proposal I have linked.

[iwiznia]

I'm sorry, but I'm not following. You said "Ideally we would not have to make this change in every input." and then suggested that instead of changing the value that affects only text inputs, we change a variable that is used 18 times in styles/styles.js and will affect many different components: avatars, menu items, buttons, pickers, etc. What exactly do you mean here?

I mean we should not have to go to the where each input is used and change the code in each file, but instead change the config that's already used in all of them. So we change one place, but affect all usages of it.
As for it affecting every component, yes, I know, all components have the same height now, so the idea is to keep that and make them all 41px high, instead of having text inputs be 41px height and buttons and the rest 40px.

[dklymenk]

Ah, ok. I see. In that case I agree then. We just replace 40 with 41 in variables.js and call it a day.

Do you want me to submit a PR?

[shawnborton]

I don't think we should do that because things like avatar images use 40px for their height. Instead, I would recommend making a separate variable for input sizes.

[dklymenk]

If you want, I can compile a list of components where the componentSizeNormal variable is used, so you have a better understanding of affected UI elements across the app. Should help make you make a decision.

Let me know if you need this list.

[iwiznia]

I don't think we should do that because things like avatar images use 40px for their height. Instead, I would recommend making a separate variable for input sizes.

Great catch! I agree.

If you want, I can compile a list of components where the componentSizeNormal variable is used, so you have a better understanding of affected UI elements across the app. Should help make you make a decision.

No need, let's create the new variable and only use it for input sizes like @shawnborton suggested.

[dklymenk]

If it is only going to be used for textInput, do we need a variable? Seems to me like this proposal here is what we need after all?

[iwiznia]

I think Shawn meant that it should be used for all inputs, not just text inputs. Right @shawnborton?

[shawnborton]

Yes, sorry, that's what I meant @iwiznia.

[dklymenk]

Ok, so it's just text input and picker elements then, right?

[shawnborton]

Correct. Instead of 41, does 42 or 44 work? Apologies but I would love to use an even number here :)

[dklymenk]

42 also works, same with 44. So will anything above given we don't change fontSize.

[shawnborton]

Sounds good, let's go with 42 for now.

[dklymenk]

@iwiznia I have submitted a PR #3805. Seems like pullerbear has assigned you already.

[iwiznia]

Nice, thank you so much for this!

Seems like pullerbear has assigned you already.

Yep, we updated it so that it assigns the expensify engineer that's assigned to the issue 😄

[iwiznia]

The PR is merged and in staging now.

[arielgreen]

Paid.