Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.54k stars 2.89k forks source link

[HOLD App#51475] [$250] Request money component is jumpy when entering the digits #19961

Open kavimuru opened 1 year ago

kavimuru commented 1 year ago

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 a DM
  2. Tap + in the compose box > Request money
  3. Enter the amount

    Expected Result:

    No jumpy behavior when entering the digits

Actual Result:

Request money component is jumpy

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

Version Number: 1.3.22 Reproducible in staging?: needs reproduction Reproducible in production?: needs reproduction 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: Any additional supporting documentation

https://github.com/Expensify/App/assets/43996225/c8c41359-c830-486a-8b17-ae78f89e0631

Expensify/Expensify Issue URL: Issue reported by: @mountiny Slack conversation: https://expensify.slack.com/archives/C9YU7BX5M/p1685628667713269

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0115f73500419bbdad
  • Upwork Job ID: 1665685007250124800
  • Last Price Increase: 2024-08-21
Issue OwnerCurrent Issue Owner: @hannojg
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @arielgreen (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

hannojg commented 1 year ago

Hey, I was bringing up that issue originally. It seems that not every device is able to reproduce the issue. However, I tested it with 5 different phones in our office, among it high end phones like the newest iPhones and the issue is reproducible on all of them ๐Ÿค” (so the issue is on android and iOS). The thing is, its hard to see while using that there is a jumpiness, but I think you can definitely feel that its not smooth. That's why I did a screen recording and when you step through the recording frame by frame you will see states where the input is not adjusting its size fast enough (see attached video).

(In addition I'd like to work on that issue once QA can confirm its reproducible).

hannojg commented 1 year ago

Also on Web it's not jumping, but you can see, if you look very closely, that the number shows with one frame delay.

This is due to the fact that we first put the text inside a Text view, measure it, and then change the size of the input.

I think it makes sense to rework the autoGrow functionality, so it isn't delayed by a frame (and on mobile doesn't jump).

https://github.com/Expensify/App/assets/16821682/93d19683-1846-4d86-a907-3c7808aab970

mountiny commented 1 year ago

@hannojg Let us know what the next steps on this one would be

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~0115f73500419bbdad

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

mountiny commented 1 year ago

Not overdue

hannojg commented 1 year ago

We could put this on hold for https://github.com/Expensify/App/issues/16696?notification_referrer_id=NT_kwDOAQCtsrM2MDA4NTM4NzgwOjE2ODIxNjgy#issuecomment-1576650890 until we settled on a decision there.

However, in the meantime, I am already experimenting with some solutions for a better implementation of the autoGrow behaviour. Will share more information once I find a good working solution ๐Ÿ˜Š

hannojg commented 1 year ago

A little update. My plan is to remove the separate text view for mobile, as react-native supports auto growing for text inputs. (For web I still have to see for the best solution).

However, I just stumbled into a native bug.

On iOS when the input is focused and you update the textinput using value the issue just happens as well:

https://github.com/Expensify/App/assets/16821682/b8ddd035-a25e-4408-94d9-50a7ddf1d635

On android the issue just happens on a regular text input regardless of focussed or not:

https://github.com/Expensify/App/assets/16821682/5d5b620c-50f1-4fc0-aec2-13c851859c56

So we'd have to investigate a fix for react native first.

mountiny commented 1 year ago

@hannojg you do have a sensitive eye, Hanno. Can you even look at <120fps screens? :trollface:

hannojg commented 1 year ago

Just a little update: we are investigating for a native fix, that will again make a great contribution to react native itself in the name of Expensify. Also with the fixes I have currently in place this issue also gets fixed:

arielgreen commented 1 year ago

Reassigning, team change

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

mountiny commented 1 year ago

@hannojg Are you saying we should put https://github.com/Expensify/App/issues/15856 on hold for this issue?

Thanks for the update

hannojg commented 1 year ago

Not necessarily, its just a nice side effect ๐Ÿ˜…

mountiny commented 1 year ago

okok thanks!

mountiny commented 1 year ago

Still on hold, I will make this weekly

aimane-chnaif commented 1 year ago

https://github.com/Expensify/App/pull/20186 which fixes https://github.com/Expensify/App/issues/16696 is likely to be merged soon. Still waiting for it to hit production.

mountiny commented 1 year ago

it was merged lets wait for deploys

mountiny commented 1 year ago

@hannojg @aimane-chnaif the PR which this was on hold for has been merged, can you confirm if this has been resolved or needs more work? Thanks!

hannojg commented 1 year ago

This needs more work. There is a react-native native bug which causes part of the issue. Ideally we'd need to fix that bug first, or find another solution that circumvents this bug. I still have this on my to do list for in ~1-2 weeks, however I am blocked at the minute with other projects.

mountiny commented 1 year ago

Thanks for an update, I think we can keep this weekly

zanyrenney commented 1 year ago

hey @hannojg do you think you'll get to this next week?

mountiny commented 1 year ago

@hannojg is currently focused on wave1 performance issues, I think we can til that gets some update.

mountiny commented 1 year ago

Still lower priority

zanyrenney commented 1 year ago

@mountiny still waiting as per wave 1 priorities?

mountiny commented 1 year ago

Correct

zanyrenney commented 1 year ago

still on hold

zanyrenney commented 1 year ago

What's the latest here @mountiny @hannojg ? Are we thinking of picking this up soon?

mountiny commented 1 year ago

I think this is deprioritized at the moment for wider performance improvements.

zanyrenney commented 1 year ago

I also now still can't reproduce this, closing.

aimane-chnaif commented 1 year ago

Still reproducible to me

https://github.com/Expensify/App/assets/96077027/06927ef3-7cbb-4052-b379-5122b9e871ff

hannojg commented 1 year ago

Agree, it's still there. Status is that this is still a native bug in react native and thus fairly complex to solve. Haven't had the time to work on it yet. We could make it open for proposals / help wanted, but only accept proposals that include a fix within react native

hannojg commented 2 months ago

https://github.com/user-attachments/assets/8dfc3e36-f368-4858-b1a9-f284834677fe

@zanyrenney I can still reproduce that issue on the latest production app. We now have capacity to work on that issue again - can we reopen?

zanyrenney commented 2 months ago

sure we can reopen but the price will be $250.

melvin-bot[bot] commented 2 months ago

Upwork job price has been updated to $250

mountiny commented 1 month ago

This is not on hold anymore @hannojg, will you still work on this one?

hannojg commented 1 month ago

Oh absolutely, thanks for the ping! @mountiny

hannojg commented 1 month ago

Okay, i started to look into this again, the solution for mobile is to remove the manual text input measurement growth. A text input can automatically grow based on the input length on mobile.

I was starting to implement a PR here:

However, the code has changed a lot since then so I will open a new PR ๐Ÿ˜Š

hannojg commented 1 month ago

I created a PR that removed the manual autoGrow calculation and let the platform components handle the width growth:

The effect is already less visible, but its not entirely gone.

There seems to be an underlying issue in react native's iOS text input implementation causing the problem. In a clean iOS app using UIKit/UITextField everything works fine, but in a clean react native app just using TextInput you can see the numbers getting cut off:

https://github.com/user-attachments/assets/9160df88-9b18-4268-b311-8b444e79d539

Will open an upstream issue ticket + PR!

hannojg commented 1 month ago

Theoretically we could already move on with the PR and add the react native fix after. or we wait first for the react native fix and add that to the PR. Not sure

hannojg commented 1 month ago

Upstream issue:

mountiny commented 1 month ago

Thanks @hannojg Do you think you could try to fix this upstream? then we can apply the fix in a patch and removed once its in a new RN version

hannojg commented 1 month ago

Yes, working on an upstream fix right now ๐Ÿ˜Š ๐Ÿ‘

hannojg commented 1 month ago

Update: i think i found a viable fix that makes sense and i don't see any other way to fix this:

However, i first want to check with meta if they are cool with this change / if they have any other solution in mind.

Currently waiting to receive an invite to the discord channel with meta (can you help with that @mountiny ?) as this seems to be the preferred procedure when trying to make changes in react native for expensify.

mountiny commented 1 month ago

I think @AndrewGable or maybe @tomekzaw might be able to ask someone? Who are good people to reach out about the discord invite?