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

[HOLD for payment 2023-10-21] [$500] Focus freeze on add contact method magic code on coming back from search #28340

Closed
2 of 6 tasks
lanitochka17 opened this issue Sep 27, 2023 · 34 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 External Added to denote the issue can be worked on by a contributor

Comments

@lanitochka17
Copy link

lanitochka17 commented Sep 27, 2023

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 the app
  2. Open settings->profile->contact method
  3. Open pending contact method or add new contact method and open the contact method
  4. Observe that currently on all digits, I cursor is visible and we can change focus to other digits
  5. Focus back to first digit and press CTRL+K/CMD+K to open search
  6. Click on back and now observe that normal cursor is displayed over digits and we cannot click and change focus

Expected Result:

Focus should not freeze when we come back to add contact method magic code page from search

Actual Result:

Focus freezes when we come back to add contact method magic code page from search

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.74-2

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

mac.chrome.desktop.focus.freeze.on.back.from.search.magic.code.mov
Recording.98.mp4

Expensify/Expensify Issue URL:

Issue reported by: @dhanashree-sawant

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695744610116319

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ba4effd8bd80f395
  • Upwork Job ID: 1707096387145940992
  • Last Price Increase: 2023-09-27
  • Automatic offers:
    • mollfpr | Reviewer | 26975575
    • tienifr | Contributor | 26975578
    • dhanashree-sawant | Reporter | 26975582
@lanitochka17 lanitochka17 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 Sep 27, 2023
@melvin-bot melvin-bot bot changed the title Focus freeze on add contact method magic code on coming back from search [$500] Focus freeze on add contact method magic code on coming back from search Sep 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2023

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

@redpanda-bit
Copy link
Contributor

Proposal

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

Input loses the ability to focus after going to search and coming back to input.

What is the root cause of that problem?

The behavior of the MagicCodeInput relies on the method onEntryTransitionEnd to be called in order to function as expected. However, navigating to search while the input is visible blurs the input, and going back to the MagicCodeInput does not cause the onEntryTransitionEnd method to be called, and therefore, the expected behavior goes away when we navigate to search from the MagicCodeInput since the focus is lost and not recovered.

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

We can add a new event listener to ScreenWrapper to listen for onFocus instead of onEntryTransitionEnd. OnFocus gets called every time the screens goes from not visible->visible regardless of the screen already being present in the navigation history or not. This would allow to call focus on the input whenever the input goes into view. This would have the MagicCodeInput restored to its expected behavior.

Make use of the new onFocus prop instead of onEntryTransitionEnd.

return (
<ScreenWrapper
onEntryTransitionEnd={() => this.validateCodeFormRef.current && this.validateCodeFormRef.current.focus()}
testID={ContactMethodDetailsPage.displayName}
>

And add the code below along with unsubscribe to https://github.com/Expensify/App/blob/main/src/components/ScreenWrapper/index.js:

        if (this.props.onFocus) {
            this.unsubscribeOnFocus = this.props.navigation.addListener('focus', () => {
                this.props.onFocus();
            });
        }

What alternative solutions did you explore? (Optional)

  1. Adding onFocusEffect to https://github.com/Expensify/App/blob/main/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js and manually focus on the input. Additionally, remove the onEntryTransitionEnd from the ScreenWrapper component.
    Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@Krishna2323
Copy link
Contributor

Proposal

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

Focus freeze on add contact method magic code on coming back from search .

What is the root cause of that problem?

  1. When the MagicCodeInput is active and you navigate to the search, the input loses focus. Upon returning, the onEntryTransitionEnd method isn't triggered. This disrupts the expected behavior since the focus on the input isn't automatically restored.
  2. There is also a height glitch that sets the input height to '0px' even if the height style is set to '52px'.

Height Glitch:

height_glitch.mp4

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

We should add a useFocusEffect hook inside BaseValidateCodeForm to focus on the input whenever the screen is focused.

    useFocusEffect(
        useCallback(() => {
            if (!inputValidateCodeRef.current) {
                return;
            }
            inputValidateCodeRef.current.focus();
        }, []),
    );

For the height glitch we can pass a prop to BaseTextInput called transparent and we will only set the height if the tranaparent is a falsy value or we can directly set the height to 100% or auto on the input.

!isMultiline && !props.transparent && {height, lineHeight},

Result:

fix_demo.mp4

@tienifr
Copy link
Contributor

tienifr commented Sep 28, 2023

Proposal

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

In

const layout = event.nativeEvent.layout;
setWidth((prevWidth) => (props.autoGrowHeight ? layout.width : prevWidth));
setHeight((prevHeight) => (!props.multiline ? layout.height : prevHeight));

we're calculating the height of the input based on onLayout function, but when we navigate to the new page, the layout.height will return 0. Ref: facebook/react-native#10743

After come back to 2FA page, the layout.height is 52px, but we're using transitionDelay:99999s for 2FA input here, that makes the input height is still 0

-> we can't interactive with the input

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

  1. Here is the PR that add transitionDelay: 99999s, and I'm sure that we just need transitionDelay for background-color so we shouldn't apply transitionDelay for all transition

-> we need to add transitionProperty: 'background-color' to inputTransparent

  1. After back to 2FA page, we need to focus on the input as well, in order to achieve that we can call inputValidateCodeRef.current.focus(); in useFocusEffect hook
    useFocusEffect(
        useCallback(() => {
            if (!inputValidateCodeRef.current) {
                return;
            }
            setTimeout(() => inputValidateCodeRef.current.focus(), CONST.ANIMATED_TRANSITION);
        }, []),
    );

we need to use setTimeout here to wait for navigation end. We already discuss this problem in this conversation

Or we can use useAutoFocusInput after this solution is merged

What alternative solutions did you explore? (Optional)

in onLayout function we can apply the condition to prevent updating the height to 0

@mollfpr
Copy link
Contributor

mollfpr commented Sep 28, 2023

@redpanda-bit Your main solution is not working. So the issue is not about the input not just focusing after the back from the search page.


For the height glitch we can pass a prop to BaseTextInput called transparent and we will only set the height if the tranaparent is a falsy value or we can directly set the height to 100% or auto on the input.

@Krishna2323 I don't understand this solution. What is defining the value of transparent? Does the auto focus is work without setTimeout?


@tienifr We can use the same pattern https://expensify.slack.com/archives/C01GTK53T8Q/p1694660990479979 for auto focus an input.

Could you confirm it's working before #21482? Removing the transitionDelay does fixing the issue, so I think you got the RCA correct.

@tienifr
Copy link
Contributor

tienifr commented Sep 28, 2023

@mollfpr yes it worked before, but we can not revert this PR. What do you think about my proposal above? Thanks

@mollfpr
Copy link
Contributor

mollfpr commented Sep 28, 2023

@tienifr It's looks good. Looking at the offending PR, they aim to delay the background-color transition in the first place.

Could you check if the issue of #21482 is still fixed?

@tienifr
Copy link
Contributor

tienifr commented Sep 29, 2023

@mollfpr It still works after applying transitionProperty

Screen.Recording.2023-09-29.at.09.43.39.mov

@mollfpr
Copy link
Contributor

mollfpr commented Sep 29, 2023

Thanks @tienifr

The proposal from @tienifr looks good to me!

For the autofocus input, we can follow the recommendation https://expensify.slack.com/archives/C01GTK53T8Q/p1694660990479979

🎀 👀 🎀 C+ reviewed!

@melvin-bot
Copy link

melvin-bot bot commented Sep 29, 2023

Triggered auto assignment to @grgia, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot added the Overdue label Oct 2, 2023
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 2, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

📣 @mollfpr 🎉 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
Copy link

melvin-bot bot commented Oct 2, 2023

📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

📣 @dhanashree-sawant 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

@grgia
Copy link
Contributor

grgia commented Oct 2, 2023

Assigned @tienifr!

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 13, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.83-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-10-20. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Oct 13, 2023

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:

  • [@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
  • [@mollfpr] 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:
  • [@mollfpr] 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:
  • [@mollfpr] Determine if we should create a regression test for this bug.
  • [@mollfpr] 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.
  • [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 13, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-10-20] [$500] Focus freeze on add contact method magic code on coming back from search [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] Focus freeze on add contact method magic code on coming back from search Oct 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 13, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.83-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-10-20. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Oct 13, 2023

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:

  • [@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
  • [@mollfpr] 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:
  • [@mollfpr] 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:
  • [@mollfpr] Determine if we should create a regression test for this bug.
  • [@mollfpr] 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.
  • [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 16, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] Focus freeze on add contact method magic code on coming back from search [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] Focus freeze on add contact method magic code on coming back from search Oct 16, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 16, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.84-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-10-23. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Oct 16, 2023

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:

  • [@grgia] The PR that introduced the bug has been identified. Link to the PR:
  • [@grgia] 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:
  • [@grgia] 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:
  • [@mollfpr / @tienifr] Determine if we should create a regression test for this bug.
  • [@mollfpr / @tienifr] 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.
  • [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 16, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] Focus freeze on add contact method magic code on coming back from search [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] Focus freeze on add contact method magic code on coming back from search Oct 16, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 16, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.84-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-10-23. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Oct 16, 2023

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:

  • [@grgia] The PR that introduced the bug has been identified. Link to the PR:
  • [@grgia] 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:
  • [@grgia] 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:
  • [@mollfpr / @tienifr] Determine if we should create a regression test for this bug.
  • [@mollfpr / @tienifr] 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.
  • [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 20, 2023
@mollfpr
Copy link
Contributor

mollfpr commented Oct 20, 2023

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:

#21482

[@mollfpr] 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:

https://github.com/Expensify/App/pull/21482/files#r1367256050

[@mollfpr] 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:

I don't think we can prevent this kind of bug with the checklist since it's very specific things to check. So the regression step should be enough.

[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] 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.

  1. Open the app
  2. Open settings->profile->contact method
  3. Open pending contact method or add new contact method and open the contact method
  4. Observe that currently on all digits, I cursor is visible and we can change focus to other digits
  5. Focus back to first digit and press CTRL+K/CMD+K to open search
  6. Click on back
  7. Verify that the input is focus and we can use it without being freeze

@jliexpensify Could you confirm which date payment is the correct one?

@jliexpensify
Copy link
Contributor

This is the PR right?

if so, it was deployed to prod on October 14th, so payment date should be 21st. But I do see there were 2 failures - this doesn't matter right?:

image

@jliexpensify jliexpensify changed the title [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] Focus freeze on add contact method magic code on coming back from search [HOLD for payment 2023-10-21] [$500] Focus freeze on add contact method magic code on coming back from search Oct 22, 2023
@jliexpensify
Copy link
Contributor

jliexpensify commented Oct 22, 2023

Payment Summary:

Upwork job

@mollfpr
Copy link
Contributor

mollfpr commented Oct 22, 2023

if so, it was deployed to prod on October 14th, so payment date should be 21st. But I do see there were 2 failures - this doesn't matter right?:

Probably, yes!

@jliexpensify I'll request the payment in NewDot, thank you!

@jliexpensify
Copy link
Contributor

Oh didn't realise you qualify for ND payments. Have adjusted above and will cancel your offer @mollfpr

@jliexpensify
Copy link
Contributor

Paid and job closed!

@JmillsExpensify
Copy link

$500 payment approved for @mollfpr based on summary above.

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 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

8 participants