[HOLD for payment 2023-06-02] [$2000] Edit box input immediately loses focus

[kavimuru]

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. Long press on the message
2. Press on the edit message

Expected Result:

Input should be focused, and the keyboard should appear

Actual Result:

The input immediately loses focus and the keyboard disappears
Sometimes on mobile iOS, it might work as expected for only the very first time

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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.8.8
Reproducible in staging?: y
Reproducible in production?: y
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

RPReplay_Final1683010782.MP4

RPReplay_Final1683010473.MP4

SKVH7218.1.MP4

Expensify/Expensify Issue URL:
Issue reported by: @terrysahaidak
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1683011194146829

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018d878cb02be4bcab
• Upwork Job ID: 1653543158120652800
• Last Price Increase: 2023-05-16

[MelvinBot]

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

[MelvinBot]

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

[twisterdotcom]

Wow yes, this is infuriating actually! Works fine on Android. Not on iOS. Wonder whether this might actually be a regression though.

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~018d878cb02be4bcab

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[chychkoi]

Proposal

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

Edit box immediately loses focus after getting focus by edit comment action on iOS.

What is the root cause of that problem?

On iOS, if text input is focused while an animation is happening (popover getting dismissed), it immediately loses focus and onBlur is called.

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

We can add delay before setting focus on textinput, just like we did on android.
Create index.ios.js file for focusTextInputAfterAnimation and copy contents from android counterpart.

const focusTextInputAfterAnimation = (inputRef, animationLength = 0) => {
    // This setTimeout is necessary because there are some animations that are just impossible to listen to in order to determine when they are finished (like when items are added to
    // a FlatList).
    setTimeout(() => {
        inputRef.focus();
    }, animationLength);
};

I saw some comments regarding user interaction and focus in the file, but this solution worked for me so far.

What alternative solutions did you explore? (Optional)

Alternatively we can just remove index.android.js and copy its contents to index.js, so that the behavior is consistent on all devices.

[twisterdotcom]

Any thoughts on this today @fedirjh or @PauloGasparSv?

[fedirjh]

I will review it today.

[fedirjh]

@chychkoi Using setTimeout is a little bit tricky in this cases, will wait for more proposals, if we have no other option then we can consider that solution.

[terrysahaidak]

There is no obvious problem, I looked it already while working on another ticket related to this. On Android there is setTimeout but I think this issue requires fix for both, since on Android it's also just a workaround.

[PauloGasparSv]

Keeping this open for a bit more time in case there is a solution!

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[fedirjh]

No satisfactory proposals yet, waiting for more proposals

[terrysahaidak]

After some more playing around I found a potential fix.
It looks like wrapping this hide callback with a requestAnimatedFrame fixes the problem:

https://github.com/Expensify/App/blob/main/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js#L245

    runAndResetCallback(callback) {
        requestAnimatedFrame(() => {
			callback()
		});
        return () => {};
    }

We run editBoxInputRef.focus() logic in the callback.

RPReplay_Final1683725937.mov

Looks like the callback is being called on the same frame the modal is still closing on the native side, going to investigate that.

[terrysahaidak]

It looks like the problem is not with the native Modal from react-native, but react-native-modal which wraps it.
Replacing react-native-modal with just native Modal fixes the problem, so something inside it interrupts our focus event.

[terrysahaidak]

According to this line of code, yes, the callback can be called even when the modal is still visible on the native side:
https://github.com/react-native-modal/react-native-modal/blob/master/src/modal.tsx#L680-L687

They basically change state, wait for the rerender and call onModalHide callback. But setState callback does not guarantee the update on the native side, since we use serialized batched bridge, all the events are batched and send to the native side on the end of each event loop.

The ultimate solution is to use the original onDismiss callback from Modal, but it might break other random modals in a random place/time.

So the proposed solution is to wrap onModalHide callback with requestAnimatedFrame to ensure the modal was closed and removed on the native side in this particular frame.

Also, found this PR in react-native-modal repo: react-native-modal/react-native-modal#733

[PauloGasparSv]

since we already have a workaround on Android and applying the same workaround to iOS fixes the issue

Oh so sorry @terrysahaidak, I misread part of the discussion and thought requestAnimationFrame was the only workaround we had because setTimeout wasn't working on all platforms.

I agree with you and @fedirjh on adding the setTimeout workaround here. That would take us back to this proposal though right @fedirjh?

[fedirjh]

That would take us back to this proposal though right

@PauloGasparSv Yes, I was referring to the setTimeout workaround . We simply need to rename focusTextInputAfterAnimation/index.android.js to focusTextInputAfterAnimation/index.native.js similar to what was suggested in this proposal .

[PauloGasparSv]

We simply need to rename focusTextInputAfterAnimation/index.android.js to focusTextInputAfterAnimation/index.native.js similar to what was suggested in #18300 (comment) .

Thks for confirming. I wasn't sure if we would create a new file like that proposal mentions, should have seen that index.android.js has no other logic than focusTextInputAfterAnimation. I wanted to confirm that to determine the assignee here.

Change makes sense to me so I'm assigning @terrysahaidak here

[melvin-bot]

📣 @terrysahaidak You have been assigned to this job by @PauloGasparSv!
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 📖

[terrysahaidak]

I created a PR as we discussed, it's ready for review.

[melvin-bot]

🎯 ⚡️ Woah @fedirjh / @terrysahaidak, great job pushing this forwards! ⚡️

The pull request got merged within 2 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

• when @terrysahaidak got assigned: 2023-05-22 21:00:14 Z
• when the PR got merged: 2023-05-24 18:33:11 UTC

On to the next one 🚀

[melvin-bot]

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

[melvin-bot]

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

• Fix focus edit box input on iOS #19477

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

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

[melvin-bot]

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:

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

[twisterdotcom]

Offer sent to you @fedirjh.

@terrysahaidak I couldn't find you in Upwork, so please apply for $1250 ($1k + $250 for reporting): https://www.upwork.com/ab/applicants/1653543158120652800/job-details

@fedirjh please complete the checklist too.

[fedirjh]

❌ Link to the PR: N/A , This issue is with an external lib, it was present from long time. We fixed it by applying same existing solution for android.
❌ Link to comment: N/A
❌ Link to discussion: N/A
🟢 Determine if we should create a regression test for this bug :

---

Regression Test Proposal

1. Open App
2. Open any chat
3. Long press on any sent messages
4. Press on the edit message
5. Verify that Input is focused, and the keyboard appears

• Do we agree 👍 or 👎

[twisterdotcom]

Regression test internal: https://github.com/Expensify/Expensify/issues/288963

[twisterdotcom]

Ah, sorry. I see @terrysahaidak is from Margelo. No worries then.

[twisterdotcom]

All paid.