[HOLD for payment 2023-02-24] [$1000] Android -New Room- Keyboard do NOT open automatically when clicking New Room

[kbecciv]

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

---

Action Performed:

1. Open the App
2. Login with applause account
3. Click on FUB button -> New Room

Expected Result:

Keyboard opens automatically

Actual Result:

Keyboard do NOT open automatically

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

Reproducible in staging?: Yes

Reproducible in production?: No

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

Bug5936574_Record_2023-02-14-00-19-39_4f9154176b47c00da84e32064abf1c48.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0183dd087e937877e5
• Upwork Job ID: 1625584939490725888
• Last Price Increase: 2023-02-14

[OSBotify]

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

[MelvinBot]

Triggered auto assignment to @francoisl (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[francoisl]

I can reproduce the issue, looking into possible regressions now.

[francoisl]

Still looking into this, but I can also reproduce the issue on the current production version (1.2.70-1), so I don't know if it needs to be a blocker.

newDotBug12701.mov.mp4

[francoisl]

I can also reproduce on 1.2.69-2, so it doesn't seem like something we missed in the last checklist. Going to remove the deploy blocker label and treat this is a regular issue.

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~0183dd087e937877e5

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[eternal-hero]

Hello. I am Nicolas North form Chicago, IL, USA.
I have checked the bug in project.
I can fix this issues asap correctly.
So far, I have worked as a senior full stack developer in several IT companies.
I really would like to contribute to your company with my skills and make my life more busy.        
I am looking forward to hearing from you.

[daraksha-dk]

Proposal

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

The keyboard doesn't open in android when room name field gets focused

What is the root cause of that problem?

In android the keyboard doesn't open because of the transition so we have to delay it. We are doing the same on other pages

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

We need to pass shouldDelayFocus only for android and for that we can import https://github.com/Expensify/App/tree/main/src/libs/shouldDelayFocus and pass it below

App/src/pages/workspace/WorkspaceNewRoomPage.js

Line 141 in 526c7aa

<RoomNameInput

then we need to implement it in the RoomNameInput and pass that prop to

App/src/components/RoomNameInput/index.native.js

Line 35 in 526c7aa

<TextInput

What alternative solutions did you explore? (Optional)

None

[flaviadefaria]

@sobitneupane @francoisl is this a good solution? If so please assign the contributor to the GH!

[situchan]

This is a duplicate of #11507 and PR which fixed this: #11587.
But later auto focus with delay was replaced with simple autoFocus in #13713 (refactoring new room page to use Form) so I would say this PR caused regression to make android auto focus not work.
Especially these lines:
https://github.com/Expensify/App/pull/13713/files#diff-a0824ee84997063d0c4f516b64966d3700554457f1ba0be2dc984c527b2bcb7fR126
https://github.com/Expensify/App/pull/13713/files#diff-a0824ee84997063d0c4f516b64966d3700554457f1ba0be2dc984c527b2bcb7fR142
cc: @puneetlath @mollfpr @fedirjh

[sobitneupane]

@maebe-nicolas Thanks for your interest.

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

[tienifr]

Thanks @sobitneupane for the quick review, but we might have overlooked a more robust solution.

Proposal

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

Keyboard do not open automatically when clicking new room.

What is the root cause of that problem?

The root cause is due to this PR, it's removing the correct use of onTransitionEnd to autofocus the input, which is the correct way to autofocus on an input upon opening the page, as explained here by @parasharrajat.

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.

The reason it's removed is because of a bug in the Form component where the ref of a form child element is overridden, you can see this line:
https://github.com/fedirjh/App/blob/2faa8b62c809384cfc778a5079e225ecccad684c/src/components/Form.js#L221

This causes the ref that we pass to RoomNameInput not to work (because it's overridden by the Form element), I think that's the reason why the PR author (@fedirjh) removed the use of onTransitionEnd and replace it by the autofocus.

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

We need to fix the bug in the form and restore the use of onTransitionEnd.

To fix the bug in the form, we need to update the Form component logic to not override the ref of the child component, only add the new ref that it needs.

This can be done by changing
https://github.com/fedirjh/App/blob/2faa8b62c809384cfc778a5079e225ecccad684c/src/components/Form.js#L221
to something like this:

ref: (node) => {
    if (child.ref) {
        child.ref(node);
    }
    this.inputRefs[inputID] = node;
},

Then we need to restore the use of onTransitionEnd, the change will be similar to in this PR where it was added.

If any other place is using shouldDelayFocus due to the that bug in the Form, I think we should refactor it as well.

What alternative solutions did you explore? (Optional)

We can use shouldDelayFocus as suggested above, but it's more like a workaround (it used timeout) rather than a solution that addresses the root cause.

Result

Working well after the fix:

Screen.Recording.2023-02-15.at.12.54.53.mov

[MelvinBot]

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.

[flaviadefaria]

@francoislif if you're happy with one of the proposals can you assign the contributor to the issue? Thanks!

[francoisl]

shouldDelayFocus will be consistent with what we do in other components, let's go with @daraksha-dk's proposal.

[MelvinBot]

📣 @daraksha-dk You have been assigned to this job by @francoisl!
Please apply to this job in Upwork 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 📖

[tienifr]

@francoisl I think a solution should be chosen because it's better and more robust, not because it's consistent with other places, since that might be an anti-pattern hidden there. I've raised a discussion on Slack to align on this.

Thanks!

cc @sobitneupane

[daraksha-dk]

@sobitneupane, PR is ready for review.

[MelvinBot]

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

[MelvinBot]

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

• Add shouldDelayFocus for RoomNameInput #15184

If no regressions arise, payment will be issued on 2023-02-24. 🎊

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

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

[flaviadefaria]

I'll pay this tomorrow!

[flaviadefaria]

@daraksha-dk submitted the PR on Feb 15 and the PR was merged on Feb 16th. This means +$500 bonus for @daraksha-dk and @sobitneupane. I'll pay you both now.

[flaviadefaria]

All paid, closing this GH.