[HOLD - Waiting on RN PRs to merge][$2000] Flickering is seen when entering first character

[kavimuru]

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

---

Action Performed:

1. Open any workspace
2. Click on Manage members
3. Click on Invite button
4. Clear the personal message & write a single character and you'll notice the flickering

Alternative steps:

1. Open the app
2. Go to Setting
3. Go to Security
4. Click on close account
5. Write a single character in the message box and you'll notice the flickering

Expected Result:

No flickering should happen

Actual Result:

The first character will flicker

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [ ] Android / native

Version Number: 1.2.64-2 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/216621627-ee3048e3-9a79-4487-94b6-af4eb2b3e679.mp4

https://user-images.githubusercontent.com/43996225/216621639-6ff88751-abbb-4f4a-a866-e6c8cc028a2a.mp4

Expensify/Expensify Issue URL: Issue reported by: @daraksha-dk Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1674823661904079

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01068656221cc9737e
• Upwork Job ID: 1627704099913342976
• Last Price Increase: 2023-02-27

[MelvinBot]

Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[MelvinBot]

@johncschuster Whoops! This issue is 2 days overdue. Let's get this updated quick!

[MelvinBot]

@johncschuster Whoops! This issue is 2 days overdue. Let's get this updated quick!

[MelvinBot]

@johncschuster Eep! 4 days overdue now. Issues have feelings too...

[johncschuster]

@daraksha-dk I tried reproducing this behavior on a Google Pixel 7 (via browserstack) and was unable to get the first character to flicker. Can you help me dial in the reproduction steps a bit more?

[johncschuster]

from @daraksha-dk (thank you!)

I can reproduce this consistently on Pixel 3a and 4a. Here are the steps & a video for the same. Open the app Go to Setting Go to Security Click on close account Write a single character

[johncschuster]

Ok, I've reproduced the behavior using a Pixel 3a (v. 1.2.73-1). To me, it seems more like the first letter is "jumping" ever so slightly, but I do agree that the first letter is behaving differently than all following letters in the input field.

https://user-images.githubusercontent.com/5579997/219700680-a723dda6-0153-46be-83e9-8b0bd5d6dace.mp4

[MelvinBot]

Triggered auto assignment to @techievivek (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[MelvinBot]

@johncschuster @techievivek this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[techievivek]

Will look into it by today.

[techievivek]

I can reproduce this on my OnePlus 7T, and this can be worked externally.

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~01068656221cc9737e

[MelvinBot]

Triggered auto assignment to @zanyrenney (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[MelvinBot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif (External)

[MelvinBot]

Current assignee @techievivek is eligible for the External assigner, not assigning anyone new.

[allroundexperts]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The text being entered in a multiline input causes it to jump.

What is the root cause of that problem?

The root cause of this issue is using line height in the style for the text input here: https://github.com/allroundexperts/Expensify/blob/feedecf0c6d218bff94abbf4943b9a69bbad374a/src/styles/styles.js#L774 When text is entered, the line height gets applied and the text gives a jumping effect.

The same issue happens on report composer as well once it gets switched to multiline input. bug.webm

What changes do you think we should make in order to solve the problem?

We can remove the line height on android for multiline text input. This will solve our issue.

Works well after applying the fix. bug 2.webm

What alternative solutions did you explore? (Optional)

As mentioned here, we can add some temporary hacks such as making sure vertical padding is set to 0 and wrapping text inside input using the <Text> tag. I however, think that we shouldn't go with workarounds and remove the line height from text input which isn't very well supported on native platforms. cc @shawnborton for your opinion.

[shawnborton]

From a technical perspective I'm not entirely sure what opinion I have to offer, as long as we can fix the bug and keep things visually looking as they should be. Perhaps we want to have one of our engineers take a look through the proposals? cc @techievivek

[allroundexperts]

From a technical perspective I'm not entirely sure what opinion I have to offer, as long as we can fix the bug and keep things visually looking as they should be. Perhaps we want to have one of our engineers take a look through the proposals? cc @techievivek

@shawnborton You can confirm the result by looking at this video: bug 2.webm

[shawnborton]

Looks good from what I can tell, but curious what Vivek thinks too.

[Litande]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The text is jumping on first character

What is the root cause of that problem?

The issue happens on android due to the fact the android placeholder line height doesn't respect the assigned line height, even if it isn't displayed (like in many if not most places in the app), those on first character the line height is readjusted to the one being set and the "jump" happens.

here is an example with a placeholder to show the issue. https://www.loom.com/share/71fde36c70404fe4b137e37c5659798a

What changes do you think we should make in order to solve the problem?

it is most likely the best solution to remove the line-height for android so it will use the native line height size as mentioned by @allroundexperts, without intervention into native implementation.

What alternative solutions did you explore? (Optional)

None

[techievivek]

@allroundexperts The proposed change looks good to me. Can you please share some more screenshots of different pages where we use the TextInput component on Android Native? I just want to make sure we don't break anything existing before moving forward with the proposed solution.

CCing @aimane-chnaif to have a look at the proposal.

[allroundexperts]

@techievivek I could find the multiline text input being used at two pages (close account and composer on the report page). I have attached the video that contains both:

https://user-images.githubusercontent.com/30054992/220361885-ec852a3a-c4fe-4cba-947b-a5ce15fd2742.mov

(For single line input, we're already disabling line height)

[aimane-chnaif]

For single line input, we were fine to remove line height because there was no visual change. But if we remove line height for multiline input, there will be visual change and clearly noticeable to users, especially when device font size is extremely small or height. But there's no impact like text is clipped because if we remove line height, text height is set automatically, respecting font family and size. And for platform consistency, if we remove line height for android, I think it should be applied to all other platforms as well.

@shawnborton do you think it's fine to remove line height everywhere (including chat composer) for any text input?

https://user-images.githubusercontent.com/96077027/220824677-b11cb996-ff56-4300-b2d7-9a13b3e36405.mov

In video: 1 - device large font with lineHeight set 2- device large font with lineHeight removed 3 - device small font with lineHeight set 4- device small font with lineHeight removed

[MelvinBot]

@johncschuster @techievivek @zanyrenney @aimane-chnaif this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[techievivek]

@shawnborton Can you please have a look at the comment posted here https://github.com/Expensify/App/issues/14799#issuecomment-1441271035 by @aimane-chnaif

[shawnborton]

I'm not sure what the ramifications would be for removing the line height. I know we looked at this quite a bit for the new text input improvements, cc @parasharrajat @Luke9389 if you have any thoughts there?

[Luke9389]

We had some problems with consistency of text inputs after removing line height, but if @allroundexperts is able to remove line height only for multiline inputs on android, then I think it'll be fine. I'm curious though, why is the line height value changing from when the box is empty to when there is text. In other words, if the line height was constant, there would be no "jumping" or "flickering", right?

[allroundexperts]

@Luke9389 This is a specific issue with RN. I've prepared a snack which you can use to play around with this behaviour. A video of the same snack is attached as well.

https://user-images.githubusercontent.com/30054992/221685352-275b43fa-a274-4081-9b2b-191e731b6dcb.mov

[aimane-chnaif]

We had some problems with consistency of text inputs after removing line height

@Luke9389 can you please link that discussion?

[Luke9389]

@aimane-chnaif, this issue was caused by line-height.

[Luke9389]

@allroundexperts Thanks for that, I'll check that out today.

[Luke9389]

Thanks for the demo @allroundexperts, that was cool.

I think the main thing to consider is how this change will effect all of our text inputs. We need a proposal that fixes this issue for multiline inputs and doesn't break single line inputs.

I saw this article that says RN will trim descending text (like j q and p) but iOS will trim ascending text like Ñ.

[allroundexperts]

Thanks for the demo @allroundexperts, that was cool.

I think the main thing to consider is how this change will effect all of our text inputs. We need a proposal that fixes this issue for multiline inputs and doesn't break single line inputs.

I saw this article that says RN will trim descending text (like j q and p) but iOS will trim ascending text like Ñ.

@Luke9389 Thank you for the reply. My understanding is that this proposal will apply to Android Multiline Inputs only. We won't be touching other platforms or single line inputs. Can you please shed some more light on why this won't work?

[Luke9389]

Hey @allroundexperts, if your proposal changes Android multiline inputs only, then I think we should proceed.

Thoughts @aimane-chnaif?

[aimane-chnaif]

@Luke9389 lineHeight was already removed for single line input on all platforms (this is on main). I don't find any impacts of removing lineHeight for multiline input on all platforms including iOS. So still not convinced that this fix should be applied to android multiline inputs only.

But there's no impact like text is clipped because if we remove line height, text height is set automatically, respecting font family and size.

Please check my comment here

[parasharrajat]

Line height changes have a direct impact on native inputs. Let's make sure that this small change does not cause big issues for us. Also, we should not repeat history if someone added it to fix another issue.

[allroundexperts]

@parasharrajat Are you suggesting to use it in android only?

[parasharrajat]

I am suggesting to be careful about these changes which look small but have a big impact. I won't suggest jumping into it unless we have full coverage of the effects. Also, implemented should be fully aware of the grey areas.

[allroundexperts]

It seems like the reason we added this was this bug. However, after reading this comment from the author of the accepted proposal, it looks like that there were multiple ways to fix the issue. The issue could have been fixed by either removing the line height completely or increasing it a little more. The author decided to go with the second approach ie increasing the line height. That introduced this bug. Looks like we can safely remove the line height.

[aimane-chnaif]

That introduced this bug

That's not true. This bug existed before that fix (increasing lineHeight) but flickering was in reverse (large -> small). After that PR, flickering is small -> large. As long as lineHeight is not exactly same as text height grown by font family and size, this issue won't be fixed. This issue was reported on Feb 3 and that PR was merged on Feb 22

[allroundexperts]

As long as lineHeight is not exactly same as text height grown by font family and size, this issue won't be fixed.

You're correct @aimane-chnaif. I did not notice that. Emojis seem to work fine after removing this 😕

[allroundexperts]

@aimane-chnaif This seems to have been added with this PR.

[zanyrenney]

From reading the comments and clicking the linked PRs I am a little lost on where we have landed here. If we think a fix for lineHeight and ensuring it works small -> large was merged on Feb 22nd. Are we saying this proposal would impact that fix? Or, that we don't need this proposal here as well? cc. @allroundexperts @techievivek @aimane-chnaif Thanks!

[allroundexperts]

@zanyrenney As far as I've tested, removing the line height does not cause any other bug to resurface (At least on android). @aimane-chnaif might have more context here though.

[aimane-chnaif]

https://github.com/Expensify/App/pull/12447#issuecomment-1340156656 (the fix was removing lineHeight) https://codeburst.io/react-native-quirks-2fb1ae0bbf80 All concerns so far are based on lineHeight set (either small or big). There's direct impact of course when lineHeight is removed but not bad impact at all.

[situchan]

@kbecciv reported issue on iOS composer ~which happens because of not enough lineHeight. The fix requires increasing lineHeight a bit more. So I don't think removing lineHeight on all platforms is ideal solution.~ I confirmed that https://github.com/Expensify/App/issues/15640 is not related to lineHeight. Even if it's very large, next line cursor starts at the bottom of first line.

[allroundexperts]

Thanks for jumping in @situchan!

I can see that we're already using lineHeightXLarge. In my opinion, It does not make sense to go beyond this. Also, I think this issue is not related to line height. You can remove the line height and the same issue still exists. In fact, I just tested it in expo and it exists there as well. You can check this as well here.

[MelvinBot]

@johncschuster @techievivek @zanyrenney @aimane-chnaif this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[MelvinBot]

Current assignee @aimane-chnaif is eligible for the Internal assigner, not assigning anyone new.

[techievivek]

I have removed the internal label because it looks like we have a potential fix in the discussion. @aimane-chnaif @allroundexperts Looking at the linked issue https://github.com/Expensify/App/issues/14674, I feel like it should be safe to get rid of line height for multiline inputs.