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.35k stars 2.77k forks source link

[HOLD for #37437][$2000] Pressing `space` 2 times `.` appears in display name #17153

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. Go to iOS settings > iPhone Settings >> General >> Keyboards > toggle on "." Shortcut
  2. Open the ios mobile app or iOS Safari or Mac Desktop Chrome
  3. Tap Setting > Profile
  4. Tap Display Name remove name
  5. Write new text and press space button 2 times and see the result (the "." shortcut doesn't work even though it's enabled)

Expected Result:

if "." shortcut is enabled, the shortcut should work when tapping the space 2 times

Actual Result:

if "." shorcut is enabled, the shortcut doesn't work when tapping the space 2 times

Workaround:

unknown

Platforms:

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

Version Number: 1.2.96-4 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: Any additional supporting documentation

https://user-images.githubusercontent.com/43996225/230680037-97e38841-3890-4671-a905-6757633be3f0.mp4

https://user-images.githubusercontent.com/43996225/230680041-a8b718bc-2575-43d1-9d83-dc2662c3094a.mp4

https://user-images.githubusercontent.com/43996225/230680087-d94f2354-2a45-498a-9705-f9f691ea13e3.MP4

Expensify/Expensify Issue URL: Issue reported by: @harshad2711 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1680851400066889

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0194127eb40345b2ea
  • Upwork Job ID: 1646595046183731200
  • Last Price Increase: 2023-04-20
gedu commented 1 year ago

@0xmiroslav

Proposal

Please restate the problem we are addressing in this issue:

The issue is that on iOS native apps, the double-tapping feature, which should add a dot when tapping the space key twice, is not working as expected. Identifying the root cause of the problem: The “TextInput” component on the iOS side is managed by the RCTSinglelineTextInputView module ( multiline=false ). When the value prop is not used, the component behaves as an uncontrolled component, where the system handles the text updates. However, when the value prop is used, the component becomes a controlled component, relying on the provided value to update the text. This controlled component setup involves increased communication between the JS and Native sides. To ensure the native state remains synchronized, there is a method called updateState that handles updating the native event after a text change. However, in the case of a controlled component, the additional back-and-forth interaction between JavaScript and Native can cause the “updateState” method to be executed multiple times. During these multiple updates, the text may be replaced, even with the same text content. This discrepancy in the number of state updates disrupts the expected behavior of the double-tapping feature, which relies on the detection of consecutive spaces.

Proposed changes to resolve the problem:

To resolve this issue, I propose adding a flag named _stateUpdated to control the execution of the “updateState” method. By setting this flag to ensure that “updateState” is executed only once, this will align the behavior with that of an uncontrolled component. The flag will be cleared after any new text change in the textField:(_unused UITextField *)textField shouldChangeCharactersInRange:(NSRange)range replacementString:(NSString *)string method. Since this method is always executed before “updateState” and not affected by the back-and-forth sync between JS and Native realms, it provides a reliable point to control the state updates.

This mean a fix at react-native framework

Exploration of alternative solutions (optional):

During the investigation, I explored a couple of alternative solutions to address the issue:

0xmiros commented 1 year ago

Thanks for the deep research. I also confirmed that it happens only when single line and uncontrolled input. I am inclined to root cause analysis and proposed fix. As this should be upstream fix, will keep watching upstream issue you created and following PR. Meanwhile, we can hold the issue here since it will take some time for upstream PR to be reviewed, merged and deployed.

chiragsalian commented 1 year ago

Nice, great work gedu. I'll demote the issue and place HOLD on it so that melvin doesn't make it overdue each day. Feel free to remove HOLD and make issue daily once upstream issue is live.

gedu commented 1 year ago

Sounds great, will take care another task for a couple of days and come back to create a Draft PR on react-native framework

0xmiros commented 1 year ago

Not overdue. On hold

gedu commented 1 year ago

Working on another tickets that has more priority, as soon as I can have them in a good position will continue here

gedu commented 1 year ago

This week or next top I will start working this issue

Christinadobrzyn commented 1 year ago

still on hold

gedu commented 1 year ago

Still on hold, will try to take this bug as soon as possible, writting some big features documentation

gedu commented 1 year ago

Still on hold

Christinadobrzyn commented 1 year ago

I'm going to be ooo until July 31st so going to unassign and assign a new teammate.

@tjferriss At this time, we're on hold. I'll take this back if it's still open when I return

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @Christinadobrzyn (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)

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @tjferriss (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)

melvin-bot[bot] commented 1 year ago

@gedu, @tjferriss, @chiragsalian, @Christinadobrzyn, @0xmiroslav Whoops! This issue is 2 days overdue. Let's get this updated quick!

tjferriss commented 1 year ago

Still on hold

melvin-bot[bot] commented 1 year ago

@gedu, @tjferriss, @chiragsalian, @Christinadobrzyn, @0xmiroslav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

tjferriss commented 1 year ago

Still on hold, MelvinBot.

melvin-bot[bot] commented 1 year ago

@gedu, @tjferriss, @chiragsalian, @Christinadobrzyn, @0xmiroslav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Christinadobrzyn commented 1 year ago

back from ooo - thanks for handling this @tjferriss - I can take it off your plate.

Still on hold for https://github.com/facebook/react-native/issues/37437 - going to move this to weekly while we wait

tjferriss commented 1 year ago

Sounds good, thanks

gedu commented 1 year ago

Hey, my Mac broke a couple of weeks ago, had most of the code there, I will have it back in 2 weeks, so I can keep working on this issue, sorry for the delay

Christinadobrzyn commented 1 year ago

Hi @gedu - do you have an update on this?

gedu commented 1 year ago

Next week I will have my Mac back and can start again with this one, sorry for the delay

chiragsalian commented 1 year ago

Hi @gedu, any update or an ETA on this? No pressure, just asking for planning purposes.

gedu commented 1 year ago

I have my Mac back, I've been setting up so it is in good shape, so beginning of next week I can keep working on this. I had a solid solution I was mainly testing so I didn't break anything else @chiragsalian

Christinadobrzyn commented 1 year ago

Hi @gedu do you have an update on this for us?

gedu commented 1 year ago

Hey, I finished a couple of tickets, today I start again to look into this, I will check were I left everything and continue doing any changes, updates and let you know

gedu commented 1 year ago

I created a PR for this issue on react-native repository: https://github.com/facebook/react-native/pull/39385

gedu commented 1 year ago

Working on the PR, I got asked to add some integration tests, so working on it

gedu commented 12 months ago

Started with integration tests for the InputText

gedu commented 12 months ago

Ok could set the first test, and is working, will keep adding more, and will try to add as much as I can.

Testing RewriteExample

input: test space replace
output: test_space_replace
Screenshot 2023-09-29 at 17 13 25
gedu commented 11 months ago

Tested Integration tests with and without my fix and Test just pass with my fix in place:

https://github.com/facebook/react-native/pull/39385#issuecomment-1747408586

Christinadobrzyn commented 11 months ago

Hi @gedu can you add an update here for us? Thanks!

Christinadobrzyn commented 11 months ago

Hi @gedu just checking on this - can you leave an update when possible?

gedu commented 11 months ago

Hey, sorry I had to jump among other tickets, I updated the code and seems there were a lot of changes, making my fix not work anymore, so I need to investigate again. Left a comment a couple of days ago https://github.com/facebook/react-native/pull/39385#issuecomment-1767657525. Will try to resume as soon as possible but the next weeks will be kind of complicated for me given that I have to work on another feature.

Christinadobrzyn commented 10 months ago

Hi @gedu do you have any update on this job? Thanks!

Christinadobrzyn commented 10 months ago

Hi @gedu please let us know of an update when you can!

Christinadobrzyn commented 10 months ago

asking in Slack for an update to this GH - https://expensify.slack.com/archives/C03UK30EA1Z/p1699661800466839

gedu commented 10 months ago

Hey, sorry, I didn't have the time to resume this bug, I will try to dedicate some time this week, had to focus on other features.

chiragsalian commented 10 months ago

any update here @gedu?

gedu commented 10 months ago

Working on this, they changed a lot the native side, I'm debugging all over again

gedu commented 9 months ago

Still working on this

gedu commented 9 months ago

Was trying to find a fix with fabric=false, but I couldn't find a clear pattern, but since fabric is true by default, I'm trying to find a fix just for fabric enabled

gedu commented 9 months ago

I found the fix, not that different as the last time, but seems that it only works with fabric, I'm double checking and creating integration tests

harshad2711 commented 9 months ago

Hey @gedu When will i get paid???

Christinadobrzyn commented 9 months ago

Hi @harshad2711 I can help with that question! Reporters are paid when the job is complete and it's been 7 days in production (details in our guidelines)That's the standard payment timeframe for all participants (reporters, contributors, C+) on a job. Let me know if you have any questions!

gedu commented 8 months ago

Integration tests are working on Android, but not on iOS, seems that webdriver or appium isn't getting the text from the Input, looking into it

gedu commented 8 months ago

Integration tests working for Android and iOS, pinged the reviewer so he can take a look