Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$500] IOU - Save button does not save and return to details page, only dismisses keyboard #36218

Closed
1 of 6 tasks
izarutskaya opened this issue Feb 9, 2024 · 28 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Monthly KSv2 Reviewing Has a PR in review

Comments

@izarutskaya
Copy link

izarutskaya commented Feb 9, 2024

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.39.0
Reproducible in staging?: Y
Reproducible in production?: N
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal Team
Slack conversation:

Action Performed:

Pre-requisite: the user must be logged in.

  1. Go to any chat.
  2. Initiate a manual money request, enter the amount.
  3. On the details page, tap on "merchant" or "Description".
  4. Enter any data.
  5. Tap on the "Save" button.

Expected Result:

After tapping on the "Save" button, the data should be saved and the user should be taken back to the request details.

Actual Result:

After tapping on the "Save" button the keyboard is dismissed.

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6373088_1707463542484.Klwh0756_1_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01dac536754e07ef50
  • Upwork Job ID: 1755902587474792448
  • Last Price Increase: 2024-02-09
@izarutskaya izarutskaya added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 9, 2024
@melvin-bot melvin-bot bot changed the title IOU - Save button does not save and return to details page, only dismisses keyboard [$500] IOU - Save button does not save and return to details page, only dismisses keyboard Feb 9, 2024
Copy link

melvin-bot bot commented Feb 9, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01dac536754e07ef50

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 9, 2024
@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment and removed Help Wanted Apply this label when an issue is open to proposals by contributors labels Feb 9, 2024
Copy link

melvin-bot bot commented Feb 9, 2024

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

Copy link

melvin-bot bot commented Feb 9, 2024

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

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Feb 9, 2024
Copy link
Contributor

github-actions bot commented Feb 9, 2024

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

Copy link

melvin-bot bot commented Feb 9, 2024

Triggered auto assignment to @Julesssss (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Feb 9, 2024

Looks like a regression from #34254

I delete this new code
And the button started working

@Julesssss
Copy link
Contributor

I just built that PR locally and I can't reproduce 😕 But I'll try once more from when it was merged to main

Screenshot 2024-02-09 at 11 09 18

@Julesssss
Copy link
Contributor

I actually can't reproduce this on main. I believe this has been fixed.

@situchan
Copy link
Contributor

situchan commented Feb 9, 2024

This is reproducible on mSafari. After reverting #34254 locally, not reproduced.

@Julesssss
Copy link
Contributor

Thanks, yeah was just able to repro on iOS mWeb. I thought it was just a case of not filling all the checkboxes 😭

@Julesssss Julesssss added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment External Added to denote the issue can be worked on by a contributor Hourly KSv2 labels Feb 9, 2024
@Julesssss
Copy link
Contributor

Okay, the workaround is simple (press the button again), so this isn't a blocker. I'll revert the PR when I have a moment, but won't cherry-pick it

@shubham1206agra
Copy link
Contributor

@suneox Can you see this, and find the problem in this?

@ishpaul777
Copy link
Contributor

ishpaul777 commented Feb 9, 2024

Hey, i think the fix is straightforward we just need to add 'BUTTON' in tagNamesOpenKeyboard

const tagNamesOpenKeyboard = ['INPUT', 'TEXTAREA'];

Screen.Recording.2024-02-09.at.5.09.42.PM.mov

@Julesssss
Copy link
Contributor

Hey, i think the fix is straightforward we just need to add 'BUTTON' in tagNamesOpenKeyboard

Okay great, I'll close my revert if you'd like to raise the PR 👍

@ishpaul777
Copy link
Contributor

okay i'll open a PR in 10-15 mins

@shubham1206agra
Copy link
Contributor

@ishpaul777 @Julesssss Can you please wait? Let @suneox confirm this solution and then proceed.

@Julesssss
Copy link
Contributor

Are they online though? I'd rather just get this resolved ASAP.

@Julesssss
Copy link
Contributor

Actually sure, we can wait a bit as we won't deploy today and it's no longer a blocker 👍

@suneox
Copy link
Contributor

suneox commented Feb 9, 2024

@ishpaul777 @Julesssss Can you please wait? Let @suneox confirm this solution and then proceed.

@shubham1206agra
To avoid release we can revert this PR I'm still investigating the Button inside FormWrapper, due to solution in PR still work with Button outside FormWrapper. i'll feedback asap

@suneox
Copy link
Contributor

suneox commented Feb 11, 2024

@Julesssss @shubham1206agra
This issue only happens on mWeb/Safari when we try to apply cached virtual viewport height, after focus-out input hide keyboard we set the height quickly it conflict with the view event so we just change the code below

    const [cachedViewportHeight, setCachedViewportHeight] = useState(windowHeight);

to

    const [, cachedViewportHeight, setCachedViewportHeight] = useDebouncedState(windowHeight, CONST.TIMING.RESIZE_DEBOUNCE_TIME);

PR is already to change status

Result

Screen.Recording.2024-02-11.at.11.17.10.mov

@Julesssss
Copy link
Contributor

Looks good @suneox thanks.

@melvin-bot melvin-bot bot removed the Overdue label Feb 12, 2024
@ishpaul777
Copy link
Contributor

I am sorry if i am not able to understand the root cause correctly, but i have tested the solution thoroghly and it works 100% of the time, do you mind providing a feedback on any edge case you find I might have missed. Also, i am not able to understand the root cause from #36218 (comment) but as far i know use debounce is avoided in codebase it often not solves the root cause directly but a workaround

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Feb 12, 2024
@suneox
Copy link
Contributor

suneox commented Feb 13, 2024

I am sorry if i am not able to understand the root cause correctly, but i have tested the solution thoroghly and it works 100% of the time

@ishpaul777 Your change will against fix cached the viewport height and the button is still delayed when the keyboard hiding

i am not able to understand the root cause from #36218 (comment)

@ishpaul777 Can you provide the root cause?

The conflict event I found when we applied cached height of the virtual viewport behavior (change immediately instead of default is delay) combined with flex: 1 at FormWrapper due to another button still work when keyboard open (Exam: back button) or removing flex:1 the button also work

as far i know use debounce is avoided in codebase it often not solves the root cause directly but a workaround

Have you got any reference documents use debounce is avoided in codebase? as I know we can use debounce to limit the rate event/value

@DylanDylann
Copy link
Contributor

DylanDylann commented Feb 13, 2024

@Julesssss this is a regression from #34254 , could you assign this issue to @shubham1206agra and unassign me? Or close this one and bring it to the original issue

@ishpaul777
Copy link
Contributor

Your #36218 (comment) will against fix cached the viewport height and the button is still delayed when the keyboard hiding

Ah, but i dont see any delay using this see video in #36218 (comment) when button is clicked it instantly redirect users but yeah i'll let you guys take care of this one as you have most context 👍

Have you got any reference documents use debounce is avoided in codebase?

I have read this many times in past on other issues, many proposals rejected by c+ because of this.

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Mar 7, 2024
Copy link

melvin-bot bot commented Mar 7, 2024

This issue has not been updated in over 15 days. @Julesssss, @jliexpensify, @shubham1206agra eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@jliexpensify
Copy link
Contributor

I think @shubham1206agra is still working on this?

@shubham1206agra
Copy link
Contributor

This has been fixed already. You can close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Monthly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants