[$1000] [HOLD for payment 2023-02-08] mWeb/Chrome - Focus is lost and the keyboard does not reopen after pressing "+" button

[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. Go to URL https://staging.new.expensify.com/

2. Login with any account

3. Go to any chat

4. Focus in the input field

5. Click on "+" button

Expected Result:

The focus is not lost, and the keyboard opens again

Actual Result:

Focus is lost and the keyboard does not reopen

Workaround:

unknown

Platform:

Where is this issue occurring?

• Mobile Web

Version Number: 1.2.37-2
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Bug5854946_Record_2022-12-08-18-23-18.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/~01e1c94921fb1cc24b
• Upwork Job ID: 1623468033919070208
• Last Price Increase: 2023-02-08

[melvin-bot]

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

[bondydaa]

Okay after looking into this it does seem like this is only happening on mobile web. Web, iOS, Android all properly give focus back to the composer input after closing this modal.

I've chased it down I think to something with detecting if touch/focus exists here:

App/src/pages/home/report/ReportActionCompose.js

Line 138 in a75a01e

this.shouldFocusInputOnScreenFocus = canFocusInputOnScreenFocus();

Because we have native file src/libs/canFocusInputOnScreenFocus/index.native.js for the counter-part src/libs/canFocusInputOnScreenFocus/index.js

So it could be something within src/libs/canUseTouchscreen/index.js that causes the Web src/libs/canFocusInputOnScreenFocus/index.js to do something strange and not give focus back.

I ran out of time today with trying to confirm the value of what canFocusInputOnScreenFocus on my phone b/c of local dev headaches so I'll pick it back up on Monday.

[bondydaa]

got stuck fighting with my VM yesterday, will finish off investigation today. dropping this to weekly since I don't think it's a major issue.

[bondydaa]

okay yep figured this out.

Using this diff

diff --git a/src/pages/home/report/ReportActionCompose.js b/src/pages/home/report/ReportActionCompose.js
index 7811d1c25e..082aba68a1 100644
--- a/src/pages/home/report/ReportActionCompose.js
+++ b/src/pages/home/report/ReportActionCompose.js
@@ -153,6 +153,8 @@ class ReportActionCompose extends React.Component {
     }

     componentDidMount() {
+        const thing = this.shouldFocusInputOnScreenFocus ? 'yes' : 'no';
+        console.log('bondydaa ' + thing);
         ReportActionComposeFocusManager.onComposerFocus(() => {
             if (!this.shouldFocusInputOnScreenFocus || !this.props.isFocused) {
                 return;
(END)

To just console log the value of this.shouldFocusInputOnScreenFocus we can see that in a non-mobile web browser that variable is true

But on mobile safari that is false

And for sake of completeness iOS app show false as well

After adding more logs I can tell that this logic is what is making it so that non-mobile web gets the focus back on the input https://github.com/Expensify/App/blob/1c60740ac8/src/pages/home/report/ReportActionCompose.js#L176-L179

that isn't triggered on the iOS app so the app must have some native code somewhere controlling this and is why we get the difference in behavior but I'm pretty confused.

looks like @parasharrajat did both #2022 and #4687 so do you have any insights here?

[kidroca]

IMO there's no BUG here

I remember this behavior was discussed extensively in multiple tickets in GH (year ago) and (I think) taken and resolved internally

AFAIK It was decided to

• restore focus to input for devices with hardware keyboard - laptops/desktops
• don't restore focus to input to prevent showing the software keyboard - it was considered unwanted user experience

Since mobile web is a case using software keyboard the expected behavior would be not to display the software keyboard after closing a popup

The case with the Emoji picker is an exception as that's tied to composing a message, therefore interacting with it moves focus back to the composer

With the (+) button users are typically moving away from composing long enough to warrant NOT expanding the keyboard automatically

[bondydaa]

Forgot to link the slack thread we started but here where I summarized what we've discussed https://expensify.slack.com/archives/C01GTK53T8Q/p1671127362759299?thread_ts=1671056016.060879&cid=C01GTK53T8Q will post a summary once I make sure we're all on the same page - I think we are but just want to double check 😅

[bondydaa]

The summary of that thread is that I think we need to take this back to the product team and come to an agreement on how we want the app to handle giving focus to/from various modals, composer component, and keyboards.

I think we kind of got to this state b/c we sort of just went with something the last time we had this discussion so hopefully we can make a decision holistically and then clean up the composer component UX and code with that guidance.

[bondydaa]

was OOO last week and only on for IFR shifts this week, will kick off discussions in product about this after the new year.

[JmillsExpensify]

Heads up, this issue will prevent us hitting WAQ starting the second week of January. While I'm hopeful that we can achieve WAQ for the first time next week, that's ultimately just a hope. It'd be good to kick this problem/solution off as soon as you're back from OOO.

[JmillsExpensify]

Also I went ahead and proactively assigned myself as BZ as we'll like have C+ review of the eventual PR.

[JmillsExpensify]

Also realized that this issue should be daily with the 30 day threshold creeping up on us.

[melvin-bot]

@JmillsExpensify, @bondydaa Eep! 4 days overdue now. Issues have feelings too...

[bondydaa]

okay I've asked in #product here about this https://expensify.slack.com/archives/C03U7DCU4/p1672788774056869

[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.2.63-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix mWeb Compose Focus bug #14230

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

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:

• [@bondydaa] The PR that introduced the bug has been identified. Link to the PR:
• [@bondydaa] 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:
• [@bondydaa] 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:
• [@bondydaa] Propose regression test steps to ensure the same bug will not reach production again.
• [@JmillsExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[bondydaa]

I don't think there is a single PR that added this, I think there was a slow build up of a few PRs from over a year ago so I'm not sure there are any additional regression tests or anything needed.

Ultimately the hope is we can refactor the compose focus logic a bit to be more streamlined and if/when we do that then we'll be sure to include updated testing steps.

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[JmillsExpensify]

Looking at the linked PR, looks like @eVoloshchak helped so I'm added external and triggering an Upwork job.

[JmillsExpensify]

Let me know when you've applied to the job above and we can get the payment for C+ PR review squared away!

[JmillsExpensify]

As for the regression test, we have an existing test for focus in the creation flow, so I think we're good on this consideration. Checking that item off the list.

[eVoloshchak]

@JmillsExpensify, applied!

[JmillsExpensify]

Offer sent!

[JmillsExpensify]

Paid out via Upwork, closing this one out!