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-05-16] [$1000] Request/Split/Send - There is NO cursor in the description input field #17579

Closed
6 tasks done
kbecciv opened this issue Apr 18, 2023 · 63 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

@kbecciv
Copy link

kbecciv commented Apr 18, 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!


**Issue found when executing PR ** #16919

Action Performed:

  1. Go to URL https://staging.new.expensify.com/
  2. Login with any account
  3. Click on the FAB button -> Send money
  4. Select an amount, and recipient and get to the final confirmation page
  5. Click on the description

Expected Result:

There is a cursor in the description input field

Actual Result:

There is NO cursor in the description input field

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.1.0

Reproducible in staging?: Yes

Reproducible in production?: New feature

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

Bug6022399_Recording__4211.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team / @Natnael-Guchima

Slack conversation:
https://expensify.slack.com/archives/C049HHMV9SM/p1681775845071829
View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01c4252f7d1c570de7
  • Upwork Job ID: 1649500398080548864
  • Last Price Increase: 2023-04-28
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 18, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

MelvinBot commented Apr 18, 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

@kbecciv kbecciv changed the title REquest/Split/Send - There is NO cursor in the description input field Request/Split/Send - There is NO cursor in the description input field Apr 18, 2023
@allroundexperts
Copy link
Contributor

allroundexperts commented Apr 18, 2023

Proposal

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

Request/Split/Send - There is NO cursor in the description input field

What is the root cause of that problem?

The root cause of this issue is that we do not have the autoFocus property set on the input here.

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

We can add autoFocus property to the text input here.

However, as mentioned by @tienifr in his proposal, adding a plain autofocus property would sometimes cause the field to be focused without the keyboard being opened in Android. We thus need to apply the autoFocus property only after the screen transition is complete. This can be done simply by providing a callback to the ScreenWrapper component that focuses the input once the transition is complete. This is currently done at several places right now and adding this here again would be a clear violation of the DRY principle.

We can remove this logic by creating a Context that wraps the ScreenWrapper here. It will look like the following:

            <ScreenWrapperContext.Provider didScreenTransitionEnd={this.state.didScreenTransitionEnd}>
                      .
                      .
                      .
            </ScreenWrapperContext.Provider>

Then we can use the above context in the TextInput component and focus only if the didScreenTransitionEnd is true. This logic can be added here:

    componentDidUpdate(prevProps) {
        if (prevProps.didScreenTransitionEnd) {
            return;
        }

        if (this.props.didScreenTransitionEnd) {
            this.textInput.focus();
        }
    }

We'll need to create a HOC that consumes the ScreenWrapperContext and injects the value of the context inside the TextInput component.

By using the above approach, we'll essentially make sure that the TextInput auto focus in Android is fixed once and for all.

What alternative solutions did you explore? (Optional)

None

@melvin-bot melvin-bot bot added the Overdue label Apr 21, 2023
@arielgreen
Copy link
Contributor

Reproduced, I agree that we should have the cursor automatically in the description field here.

@melvin-bot melvin-bot bot removed the Overdue label Apr 21, 2023
@arielgreen arielgreen added the External Added to denote the issue can be worked on by a contributor label Apr 21, 2023
@melvin-bot melvin-bot bot changed the title Request/Split/Send - There is NO cursor in the description input field [$1000] Request/Split/Send - There is NO cursor in the description input field Apr 21, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

Current assignee @arielgreen is eligible for the External assigner, not assigning anyone new.

@MelvinBot
Copy link

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

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

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

@Dev-Demons
Copy link

Dev-Demons commented Apr 21, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.
Request/Split/Send - There is NO cursor in the description input field

What is the root cause of that problem?

The root cause of this issue is that we do not have the autoFocus property set on the input here.

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

We can add autoFocus property in MoneyRequestDescriptionPage.js line 76

	<TextInput
                            inputID="moneyRequestComment"
                            name="moneyRequestComment"
                            defaultValue={this.props.iou.comment}
                            label={this.props.translate('moneyRequestConfirmationList.whatsItFor')}
                            autoFocus
                      />

Result1

first

What alternative solutions did you explore?

MoneyRequestDescriptionPage.js line 76
change to

 <TextInput
                            inputID="moneyRequestComment"
                            name="moneyRequestComment"
                            defaultValue={this.props.iou.comment} 
                            placeholder={this.props.translate('moneyRequestConfirmationList.whatsItFor')}
                            autoFocus
                        />

Result2:

We can select one from these 2 methods.

Upwork profile: https://www.upwork.com/freelancers/~0134bf9909f3e4b438
Email: rajdeveloperpet@outlook.com

@MelvinBot
Copy link

📣 @Dev-Demons! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@Dev-Demons
Copy link

Contributor details
Your Expensify account email: rajdeveloperpet@outlook.com
Upwork profile Link: https://www.upwork.com/freelancers/~0134bf9909f3e4b438

@MelvinBot
Copy link

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@jte0711
Copy link

jte0711 commented Apr 22, 2023

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

Request/Split/Send - There is NO cursor in the description input field

What is the root cause of that problem?

TextInput on line 76 in MoneyRequestDescriptionPage doesn't have autoFocus as its property.

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

Adding autoFocus to the TextInput on line 76 in MoneyRequestDescriptionPage. Example:

...
<TextInput
    inputID="moneyRequestComment"
    name="moneyRequestComment"
    defaultValue={this.props.iou.comment}
    label={this.props.translate('moneyRequestConfirmationList.whatsItFor')}
    autoFocus
/>
...

What alternative solutions did you explore? (Optional)

@MelvinBot
Copy link

📣 @jte0711! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@jte0711
Copy link

jte0711 commented Apr 22, 2023

Contributor details
Your Expensify account email: james.dev2195@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01b6e80d3e91bebe9c

@MelvinBot
Copy link

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@tienifr
Copy link
Contributor

tienifr commented Apr 22, 2023

Proposal

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

There is no autofocus in the description input field even though there's only 1 input element.

What is the root cause of that problem?

The root cause is that we don't have auto-focusing logic for this TextInput

.

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

This is a common use case that has been addressed in many places.

The correct way to implement it is to use onEntryTransitionEnd, see explanation here.

I don't think we should be relying on shouldDelayFocus for autofocusing. In the past, the normal logic was to just autofocus. It was causing some issues with animations then we explored various things and picked transitionEnd as the best approach. For the reason that we want a reliable way to know when the navigation animation is finished. Due to shouldDelayFocus uses a fixed delay, it will not always work reliably.
I am also against shouldDelayFocus implementation on the TextInput but it might be added to handle some edge cases.

That onEntryTransitionEnd implementation to autofocus has been used in many places with the same use cases, like here and here.

Here's what we need to do:

  1. Add a ref for the description text input here.
ref={el => this.descriptionInputRef = el}
  1. In ScreenWrapper here, focus on the text input inside onEntryTransitionEnd callback
onEntryTransitionEnd={() => this.descriptionInputRef && this.descriptionInputRef.focus()}

This is a simple 1-line change and it doesn't repeat any logic here (it simply calls .focus) so should be enough. If we try to move the logic down to the TextInput component it will create unnecessary overhead and re-renders.

What alternative solutions did you explore? (Optional)

We can also use autoFocus and shouldDelayFocus for the text input here. Using shouldDelayFocus is a must because without it the keyboard will not open in Android.

That's also ok but I prefer using onEntryTransitionEnd as per the above explanation.

@MelvinBot
Copy link

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

@situchan
Copy link
Contributor

situchan commented Apr 23, 2023

I suggest to fix all auto-focus issues in one GH and in BZ checklist stage, add it to PR checklist along with trim when we implement new page with text inputs.
There are many minor issues currently reported related to autofocus, trim.
i.e.
autofocus: #17579 #17714 #17833
trim: #17663 #17770 #17485
All of these are came from NewContactMethodPage, MoneyRequestDescriptionPage implementation.
cc: @cristipaval @yuwenmemon

Edit: Another trim issue: #17725

@tgolen
Copy link
Contributor

tgolen commented May 8, 2023

I think that we should fix all cases in this issue, and we can increase the price of the issue to account for the change in scope. Can we get a full and updated proposal?

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels May 9, 2023
@melvin-bot melvin-bot bot changed the title [$1000] Request/Split/Send - There is NO cursor in the description input field [HOLD for payment 2023-05-16] [$1000] Request/Split/Send - There is NO cursor in the description input field May 9, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label May 9, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 9, 2023

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot
Copy link

melvin-bot bot commented May 9, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.12-0 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-05-16. 🎊

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.

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 May 9, 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:

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

@arielgreen
Copy link
Contributor

@Natnael-Guchima to answer your question, the two issues are separate but related and given you reported them both, I think the reporting bonus is valid for both.

Can you apply to the Upwork posting so I can hire you for the reporting bonus?

@arielgreen
Copy link
Contributor

@sobitneupane @tienifr I have sent offers over in Upwork

@Natnael-Guchima
Copy link

@Natnael-Guchima to answer your question, the two issues are separate but related and given you reported them both, I think the reporting bonus is valid for both.

Can you apply to the Upwork posting so I can hire you for the reporting bonus?

Thanks @arielgreen. I have applied for the two reporting bonus on the Upwork job. We can close the other report (#17833) since I have applied for that one too here.

cc: @maddylewis

@arielgreen
Copy link
Contributor

@Natnael-Guchima excellent, offer sent.

@Natnael-Guchima
Copy link

@Natnael-Guchima excellent, offer sent.

Accepted. Thanks, @arielgreen.

@situchan
Copy link
Contributor

situchan commented May 11, 2023

I suggest to fix all auto-focus issues in one GH and in BZ checklist stage, add it to PR checklist along with trim when we implement new page with text inputs. There are many minor issues currently reported related to autofocus, trim. i.e. autofocus: #17579 #17714 #17833 trim: #17663 #17770 #17485 All of these are came from NewContactMethodPage, MoneyRequestDescriptionPage implementation. cc: @cristipaval @yuwenmemon

Edit: Another trim issue: #17725

Just a reminder to add these to PR checklist I suggest.
Especially, auto-focus issues keep growing. They are easily missed when implement new pages
i.e. #18660 #18371

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels May 15, 2023
@arielgreen
Copy link
Contributor

Calculating payments:

1 business days = 50% bonus

$500 to @Natnael-Guchima
$1000 + 50% = $1500 to @tienifr
$1000 + 50% = $1500 to @sobitneupane

All payments have been issued.

@arielgreen
Copy link
Contributor

@sobitneupane can you please review and complete the checklist? Once that's done we are good to close this.

@sobitneupane
Copy link
Contributor

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:

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

#16919

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

#16919 (comment)

@sobitneupane
Copy link
Contributor

We have an issue on mWeb/Safari (reproducible only on real device). Those issues exists in these places. @tgolen had suggested all those cases to be handled here. I am little confused on the next step now.

@tgolen
Copy link
Contributor

tgolen commented May 19, 2023

Ah, so it looks like none of those other places were fixed in the current PR (I kinda forgot about them). At this point, we'll need to open up a new issue to have those other places fixed.

@melvin-bot melvin-bot bot added the Overdue label May 22, 2023
@tgolen
Copy link
Contributor

tgolen commented May 22, 2023

@kbecciv How do you want to handle fixing the other places?

@melvin-bot melvin-bot bot removed the Overdue label May 22, 2023
@arielgreen
Copy link
Contributor

Discussing in Slack here.

@arielgreen
Copy link
Contributor

Creating a new issue for this, we're done here.

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