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
2.99k stars 2.5k forks source link

[LOW] [Splits] [$500] IOU - App returns to participant selection page when hitting Save with Tab on Description page #37464

Closed m-natarajan closed 2 weeks ago

m-natarajan commented 2 months 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!


Version Number: 1.4.45-0 Reproducible in staging?: y Reproducible in production?: n 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 Expensify/Expensify Issue URL: Issue reported by: Applause internal team Slack conversation:

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to FAB > Request money > Manual.
  3. Enter amount and select participant.
  4. On confirmation page, press Tab.
  5. Navigate to Description and hit Enter.
  6. Enter description and save it with Tab and Enter key.

Expected Result:

The description will be saved and app will land on confirmation page.

Actual Result:

The description is saved and app returns to participant selection page.

Workaround:

unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/38435837/0fb64f28-a879-431f-9c7c-73f8545689d9

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~010769f678e16c288e
  • Upwork Job ID: 1767859361668382720
  • Last Price Increase: 2024-03-27
  • Automatic offers:
    • akinwale | Reviewer | 0
dragnoir commented 1 month ago

Proposal

Updated

mkhutornyi commented 1 month ago

@dragnoir's proposal looks good to me. cc: @techievivek πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed

melvin-bot[bot] commented 1 month ago

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

mallenexpensify commented 1 month ago

@mkhutornyi is unexpectedly going to be OOO for a while, @akinwale , can you please take over as C+ here? If it doesn't work for you, please post in #contributor-plus

nkdengineer commented 1 month ago

I see that the selected solution does not work in case three dot header button and cause regression: can not press enter to choose any option in request money confirmation screen

nkdengineer commented 1 month ago

@akinwale I updated my main solution (you can ignore alternative solution) in proposal

bernhardoj commented 1 month ago

@akinwale let me know if you are interested in fixing this upstream instead of a workaround.

dragnoir commented 1 month ago

I see that the selected solution does not work in case three dot header button and cause regression: can not press enter to choose any option in request money confirmation screen

@nkdengineer three dot header is not a Button component. My proposal here is for only the Button component that fixes the issue.

let me know if you are interested in fixing this upstream instead of a workaround.

@bernhardoj Your upstream PR has been rejected. Also, we proved that it's not an upstream issue. You can check here for details https://github.com/Expensify/App/issues/37464#issuecomment-2004939788

bernhardoj commented 1 month ago

Also, we proved that it's not an upstream issue. You can check here for details https://github.com/Expensify/App/issues/37464#issuecomment-2004939788

The proof isn't valid at all. The test that you provide doesn't try to replicate the reproduction behavior. In this issue, pressing the description save button navigates the user back, while the test code you wrote just logs a text. Try adding a navigateBack and you will see the issue.

The button component uses Pressable under the hood, so the issue happens on every component that uses Pressable. So, we need to fix it globally. It's not about event propagation either.

Do you have any other proof that I might be missed?

Your upstream PR has been rejected

Just because the PR will cause a regression doesn't mean I don't have a chance to fix it πŸ˜„

dragnoir commented 1 month ago

Do you have any other proof that I might be missed?

@bernhardoj Yes for sure. This is another proof for you to understand that the issue is not upstream.

Try adding a navigateBack and you will see the issue.

I think you have not tested it, so I created the branch for you https://github.com/dragnoir/App/commit/7e30d9ee25d0ebe6e94a7589d033667e228d21df Inside IOURequestStepDescription I added a native Pressable with navigateBack call. Please check the video carefully and test it on your own. NO ISSUE ==> NOT UPSTREAM

https://github.com/Expensify/App/assets/12425932/c8bbd8fa-8584-4b98-a463-517e9c55defd

bernhardoj commented 1 month ago

https://github.com/Expensify/App/assets/50919443/19450efe-8fc0-41a8-9fde-e851168a7d4e

Btw, let's wait for @akinwale

dragnoir commented 1 month ago

@bernhardoj I tried to reproduce your video and I discovered the issue.

Can you pls try again with this:

<View>
-  <Pressable accessibilityRole="button" onPress={navigateBack}>
+  <Pressable onPress={navigateBack}>
    <Text>Native Pressable</Text>
  </Pressable>
</View>

The issue is gone :)

bernhardoj commented 1 month ago

But the accessibilityRole is also gone which will render it as a div instead of a button. You can read on the upstream PR for the short explanation of why the issue happens with a button role.

techievivek commented 1 month ago

Gentle bump @akinwale, can you please take a look at the above comments and see if we really have a solution here. Thanks.

akinwale commented 1 month ago

After reviewing, I believe we can still move ahead with @dragnoir's proposal.

@bernhardoj While an upstream fix would be ideal, there's already been some friction in the process, and it looks like it may delay things further if we keep waiting for it to get merged, so I think it makes sense to just move forward with a fix local to our codebase.

cc @techievivek

nkdengineer commented 1 month ago

@akinwale Do you have any feedback about my proposal?

nkdengineer commented 1 month ago

@akinwale My proposal also fixes the case when we focus on "Three dot" button then press on "ENTER"

bernhardoj commented 1 month ago

@akinwale we need a solution to fix both this and https://github.com/Expensify/App/issues/33502. It's not a critical issue, so I don't think waiting the upstream is a big problem instead of using a workaround. There is already a progress on the upstream PR.

dragnoir commented 1 month ago

I already have a solution for https://github.com/Expensify/App/issues/33502, I will post after we fix this. I managed to fix all the issues related to the keyboard enter key.

melvin-bot[bot] commented 1 month ago

πŸ“£ @akinwale πŸŽ‰ An offer has been automatically sent to your Upwork account for the Reviewer role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 1 month ago

πŸ“£ @dragnoir You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing πŸ“–

MitchExpensify commented 1 month ago

What's our ETA on the PR @dragnoir ?

dragnoir commented 1 month ago

PR ready https://github.com/Expensify/App/pull/39924

techievivek commented 4 weeks ago

Hmm, not sure why automation didn't work here. @MitchExpensify Can we look into the payments here? Thanks.

mallenexpensify commented 3 weeks ago

Still hasn't hit production, so payment isn't due for at least a week.

MitchExpensify commented 3 weeks ago

Still holding on PR to hit production to start the 7 day countdown to payment

bernhardoj commented 3 weeks ago

Since we are not fixing this globally, can I request to reopen https://github.com/Expensify/App/issues/33502, please? I already have an accepted proposal there but gets closed. The upstream PR is already merged, so the plan is to:

  1. Update rn-web to 0.19.11
  2. Remove the workaround

The fix will fix this GH, https://github.com/Expensify/App/issues/40602, https://github.com/Expensify/App/issues/33502, and other unrecorded keyboard enter issues.

@techievivek @akinwale

cc: @roryabraham as the assigned engineer for the tab accessibility issue

roryabraham commented 3 weeks ago

@bernhardoj why don't you post your proposal on https://github.com/Expensify/App/issues/40602 ?

bernhardoj commented 3 weeks ago

@roryabraham I'm hoping to get the bounty from that issue πŸ˜„. I really don't like how it's handled in that issue. The first discussion decided to fix the issue, I got assigned, then the 2nd discussion decided to close the issue 😞.

roryabraham commented 3 weeks ago

@bernhardoj I reopened https://github.com/Expensify/App/issues/33502 - let's proceed with the RNW upgrade

dragnoir commented 2 weeks ago

@MitchExpensify can you pls check payment here?

MitchExpensify commented 2 weeks ago

Payment summary:

C $500 @dragnoir C+ $500 @akinwale

Both paid via Upwork, contracts ended also