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

[BZ checklist] [$500] [Held requests] IOS - Hold Request - Keyboard is up but the field is not focused with error message showing #36948

Closed
1 of 6 tasks
lanitochka17 opened this issue Feb 20, 2024 · 48 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Feb 20, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.43-3
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. Launch New Expensify app
  2. Create a manual request from any user
  3. Navigate to request details page
  4. Tap 3-dot menu > Hold request
  5. Close the menu
  6. Tap 3-dot menu > Hold request

Expected Result:

The "Reason" field will be focused

Actual Result:

Keyboard is up but the field is not focused with error message showing up

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

Add any screenshot/video evidence

Bug6385952_1708450041705.RPReplay_Final1708393963.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~014db792b47c7bf65e
  • Upwork Job ID: 1760046429209067520
  • Last Price Increase: 2024-02-20
  • Automatic offers:
    • situchan | Reviewer | 0
    • Krishna2323 | Contributor | 0
@lanitochka17 lanitochka17 added DeployBlockerCash This issue or pull request should block deployment External Added to denote the issue can be worked on by a contributor labels Feb 20, 2024
@melvin-bot melvin-bot bot changed the title IOS - Hold Request - Keyboard is up but the field is not focused with error message showing [$500] IOS - Hold Request - Keyboard is up but the field is not focused with error message showing Feb 20, 2024
Copy link

melvin-bot bot commented Feb 20, 2024

Job added to Upwork: https://www.upwork.com/jobs/~014db792b47c7bf65e

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 20, 2024
Copy link

melvin-bot bot commented Feb 20, 2024

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

@melvin-bot melvin-bot bot added the Daily KSv2 label Feb 20, 2024
@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Feb 20, 2024
Copy link
Contributor

👋 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 20, 2024

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

@lanitochka17
Copy link
Author

We think that this bug might be related to #vip-bills
CC @davidcardoza

@Krishna2323
Copy link
Contributor

Krishna2323 commented Feb 20, 2024

Proposal

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

IOS - Hold Request - Keyboard is up but the field is not focused with error message showing

What is the root cause of that problem?

We are not using useAutoFocusInput and due to the input gets focused before the transition ends and blurred when transition ends.

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

We need to use useAutoFocusInput inside HoldReasonPage for comment InputWrapper and remove autoFocus prop from InputWrapper.

const {inputCallbackRef} = useAutoFocusInput();
ref={inputCallbackRef}

Result

ios_focus_bug.mp4

Alternative

Instead of using useEffect to focus input on mount we can use const {inputCallbackRef} = useAutoFocusInput(); and call inputCallbackRef(ref) when autofocus is true. We can also remove the shouldDelayFocus prop.

// AutoFocus which only works on mount:
useEffect(() => {
// We are manually managing focus to prevent this issue: https://github.com/Expensify/App/issues/4514
if (!autoFocus || !input.current) {
return;
}
if (shouldDelayFocus) {
const focusTimeout = setTimeout(() => input?.current?.focus(), CONST.ANIMATED_TRANSITION);
return () => clearTimeout(focusTimeout);
}
input.current.focus();
// We only want this to run on mount
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

<RNTextInput
ref={(element) => {
    if (typeof ref === 'function') {
        ref(element);
    } else if (ref && 'current' in ref) {
        // eslint-disable-next-line no-param-reassign
        ref.current = element;
    }

    if (autoFocus) {
        inputCallbackRef(element);
    }

    input.current = element as HTMLInputElement | null;
}}

@situchan
Copy link
Contributor

This is polish than bug. Not deploy blocker

@Krishna2323
Copy link
Contributor

@situchan, visiting the form second time of afterwards shows error without any interactivity.

@neil-marcellini
Copy link
Contributor

neil-marcellini commented Feb 20, 2024

I agree that it's not a blocker because this is a new feature that isn't on production yet, and it's a very minor issue. @situchan please start reviewing the proposals.

@neil-marcellini neil-marcellini added Bug Something is broken. Auto assigns a BugZero manager. and removed DeployBlockerCash This issue or pull request should block deployment labels Feb 20, 2024
Copy link

melvin-bot bot commented Feb 20, 2024

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

@situchan
Copy link
Contributor

@Krishna2323's proposal looks good to me.
🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Feb 27, 2024

Current assignee @neil-marcellini is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@situchan
Copy link
Contributor

@Krishna2323 in this issue, let's fix other similar pages with the same root cause.
i.e. I found WorkspaceProfileDescriptionPage

@situchan
Copy link
Contributor

@situchan, WorkspaceProfileDescriptionPage autofocus works fine, should we change it? We are using autoFocus prop in multiple places, do we want to change it in all places?

It's ideal to fix all occurrences but too many. It's definitely increased scope.
@neil-marcellini what do you think? Should we fix only reported pages or all throughout the app?

@Krishna2323
Copy link
Contributor

@neil-marcellini bump incase you missed the question above ^

@Krishna2323
Copy link
Contributor

@neil-marcellini, friendly bump ^

@robertjchen
Copy link
Contributor

In the interest of time, let's just move forward with just the reported pages for now 🙏

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Mar 21, 2024
@Krishna2323
Copy link
Contributor

@situchan, PR ready for review.

@trjExpensify trjExpensify changed the title [$500] [Held requests] IOS - Hold Request - Keyboard is up but the field is not focused with error message showing [Awaiting Payment 3rd April] [$500] [Held requests] IOS - Hold Request - Keyboard is up but the field is not focused with error message showing Apr 2, 2024
@trjExpensify trjExpensify added the Awaiting Payment Auto-added when associated PR is deployed to production label Apr 2, 2024
@trjExpensify
Copy link
Contributor

Automation didn't run. Needs payment on 3rd April.

@Krishna2323
Copy link
Contributor

@trjExpensify, friendly bump for payments :)

@trjExpensify
Copy link
Contributor

Offers sent!

@Krishna2323
Copy link
Contributor

@trjExpensify accepted, thanks!

@trjExpensify
Copy link
Contributor

Cool! Payment summary as follows:

@trjExpensify trjExpensify changed the title [Awaiting Payment 3rd April] [$500] [Held requests] IOS - Hold Request - Keyboard is up but the field is not focused with error message showing [Awaiting C+ payment] [$500] [Held requests] IOS - Hold Request - Keyboard is up but the field is not focused with error message showing Apr 4, 2024
@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 8, 2024
@bfitzexpensify
Copy link
Contributor

bfitzexpensify commented Apr 9, 2024

Payment to @situchan done.

@situchan, adding a manual BZ checklist here - please complete! Thanks

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@situchan] The PR that introduced the bug has been identified. Link to the PR:
  • [@situchan] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@situchan] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@situchan] Determine if we should create a regression test for this bug.
  • [@situchan] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@bfitzexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@bfitzexpensify bfitzexpensify changed the title [Awaiting C+ payment] [$500] [Held requests] IOS - Hold Request - Keyboard is up but the field is not focused with error message showing [BZ checklist] [$500] [Held requests] IOS - Hold Request - Keyboard is up but the field is not focused with error message showing Apr 11, 2024
Copy link

melvin-bot bot commented Apr 16, 2024

@neil-marcellini, @bfitzexpensify, @Krishna2323, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

@bfitzexpensify
Copy link
Contributor

Bump on the BZ checklist @situchan - thank you

@bfitzexpensify
Copy link
Contributor

Bumping again @situchan - thank you!

@bfitzexpensify
Copy link
Contributor

Ah, Situ is OOO. Will check back for when he's returned and bump this in Slack to be completed.

Copy link

melvin-bot bot commented Apr 29, 2024

@neil-marcellini, @bfitzexpensify, @Krishna2323, @situchan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Copy link

melvin-bot bot commented May 1, 2024

@neil-marcellini, @bfitzexpensify, @Krishna2323, @situchan Eep! 4 days overdue now. Issues have feelings too...

Copy link

melvin-bot bot commented May 3, 2024

@neil-marcellini, @bfitzexpensify, @Krishna2323, @situchan 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@bfitzexpensify
Copy link
Contributor

Still OOO, will bump when back.

@bfitzexpensify
Copy link
Contributor

I'm going to close this one out myself.

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • The PR that introduced the bug has been identified. Link to the PR: polish, not a bug - [BZ checklist] [$500] [Held requests] IOS - Hold Request - Keyboard is up but the field is not focused with error message showing #36948 (comment)
  • The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
  • A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
  • Determine if we should create a regression test for this bug. No need, minor bug
  • If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again. N/A
  • Link the GH issue for creating/updating the regression test once above steps have been agreed upon: N/A

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
No open projects
Archived in project
Development

No branches or pull requests

8 participants