[$500] Compose Box - Focus not lost from composer on LHN popup open

lanitochka17 commented 11 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 1.4.11-6 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 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

Open the app
Open any report and send any message
Focus on composer
Click on plus besides the composer to open Actions popup
Right on any chat in LHN to open popup on LHN
Observe that even though popup is open in LHN, app still focuses on composer

Expected Result:

App should not focus back on composer whenever any popup is open

Actual Result:

App focuses back on composer when we open actions popup and open any right click popup in LHN

Workaround:

Unknown

Platforms:

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

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/74137c5f-18a4-4e16-8ee5-d8ccfda09552

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0123ed583fdc5774f0
Upwork Job ID: 1734947870117650432
Last Price Increase: 2023-12-13
Automatic offers:

bernhardoj | Contributor | 28067465

Issue Owner

Current Issue Owner: @mallenexpensify

melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

Job added to Upwork: https://www.upwork.com/jobs/~0123ed583fdc5774f0

melvin-bot[bot] commented 11 months ago

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[bot] commented 11 months ago

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

dukenv0307 commented 11 months ago

Proposal

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

App focuses back on composer when we open actions popup and open any right click popup in LHN

What is the root cause of that problem?

When popover is closed, onPopoverMenuClose is called. https://github.com/Expensify/App/blob/d6755d68444d5d8e9e2c7593c7eb3f1228a20c67/src/pages/home/report/ReportActionCompose/AttachmentPickerWithMenuItems.js#L166

And then restoreKeyboardState is called which makes the composer focused. https://github.com/Expensify/App/blob/d6755d68444d5d8e9e2c7593c7eb3f1228a20c67/src/pages/home/report/ReportActionCompose/ReportActionCompose.js#L193

We already the logic here to blur the active element but the time this function is called, the active element isn't composer. https://github.com/Expensify/App/blob/8fa65ec4de74f22f7c700eb78b4ee83eda96de58/src/components/LHNOptionsList/OptionRowLHN.js#L195

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

We should pass event to onPopoverMenuClose and pass it to onMenuClosed. And then in restoreKeyboardState, we should check if the event.target is a clickable element, we will not focus the composer.

What alternative solutions did you explore? (Optional)

We can add InteractionManager.runAfterInteractions here to wait the composer focus and then blur it.

https://github.com/Expensify/App/blob/8fa65ec4de74f22f7c700eb78b4ee83eda96de58/src/components/LHNOptionsList/OptionRowLHN.js#L195

bernhardoj commented 11 months ago

Proposal

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

The composer is focused if we close the add action popover by opening another popover.

What is the root cause of that problem?

Every time the add action popover closes, we call restoreKeyboardState which will refocus the keyboard. https://github.com/Expensify/App/blob/d6755d68444d5d8e9e2c7593c7eb3f1228a20c67/src/pages/home/report/ReportActionCompose/ReportActionCompose.js#L193-L199

When we right-click the LHN item or chat report message, it will:

Close the add action popover
Open the lhn or report popover
Refocus the composer

The composer focus will call focusComposerWithDelay https://github.com/Expensify/App/blob/d6755d68444d5d8e9e2c7593c7eb3f1228a20c67/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.js#L399-L401

In focusComposerWithDelay, we already have a logic to prevent the focus from happening if the emoji picker is visible. That's why this doesn't happen when we open the emoji picker and then open the report popover. https://github.com/Expensify/App/blob/d6755d68444d5d8e9e2c7593c7eb3f1228a20c67/src/libs/focusComposerWithDelay.ts#L14-L19

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

Don't focus if the report/LHN popover is visible. Both use the same component, PopoverReportActionContextMenu.

Expose a new variable from PopoverReportActionContextMenu called isVisible https://github.com/Expensify/App/blob/d6755d68444d5d8e9e2c7593c7eb3f1228a20c67/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js#L272-L282
```
isVisible: isPopoverVisible
```

In focusComposerWithDelay, early return if PopoverReportActionContextMenu.isVisible() is true.


import * as ReportActionContextMenu from '@pages/home/report/ContextMenu/ReportActionContextMenu';

if (!textInput || EmojiPickerAction.isEmojiPickerVisible() || ReportActionContextMenu.isVisible()) { return; }


### What alternative solutions did you explore? (Optional)
I notice that this also happens if we:
1. Open add action (+) popover
2. Press the three-dots menu
3. The composer will be focused

To solve this for all popover, we can prevent the focus if any of the popover is showing.

To do that, in ComposerWithSuggestions, get the popover context visible state.

const {isOpen} = React.useContext(PopoverContext);

const focus = useCallback((shouldDelay = false) => { if (isOpen) { return; }

mananjadhav commented 10 months ago

I liked @bernhardoj's proposal to fix this with ComposerWithSuggestions's popover context visible state.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 10 months ago

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

mallenexpensify commented 10 months ago

@jasperhuangg when you have a min can you review @bernhardoj 's proposal above? https://github.com/Expensify/App/issues/32970#issuecomment-1854133465

melvin-bot[bot] commented 10 months ago

📣 @bernhardoj 🎉 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 📖

jasperhuangg commented 10 months ago

Sorry for the delay, NewDot deployer chore has taken a lot of time this week. Looks good to me, assigned!

bernhardoj commented 10 months ago

PR is ready

cc: @mananjadhav

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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

https://github.com/Expensify/App/pull/33483

If no regressions arise, payment will be issued on 2024-01-11. :confetti_ball:

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:

@mananjadhav requires payment through NewDot Manual Requests
@bernhardoj requires payment automatic offer (Contributor)

melvin-bot[bot] commented 10 months ago

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:

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

bernhardoj commented 10 months ago

@mananjadhav hi, we have a regression here. The issue is that the isPopoverOpen state is late to update, so the focus always fails. More info is explained on the proposal there.

After looking at the linked proposal, I remember we have a PR that will do a big refactor to the composer focus logic and when I look around the PR, I found that they will handle this issue too.

I don't think there is an easy way to handle this globally, so, sadly I think we should revert our PR and wait for the above PR. (except we want to handle this issue for LHN (report context menu) only, which means my main solution)

Let me know what you think!

melvin-bot[bot] commented 10 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

mallenexpensify commented 10 months ago

@mananjadhav can you review @bernhardoj 's comment above and propose the next best steps? Should I put this on hold pending https://github.com/Expensify/App/pull/29199 ?

mananjadhav commented 10 months ago

I can see the PR is already reverted. Yes we should put this on Hold pending #29199

melvin-bot[bot] commented 10 months ago

Issue is ready for payment but no BZ is assigned. @anmurali you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

mallenexpensify commented 10 months ago

Put on hold pending https://github.com/Expensify/App/pull/29199 Removed 'hold for payment' too

anmurali commented 9 months ago

Held, nothing to do here!

anmurali commented 9 months ago

https://github.com/Expensify/App/pull/29199 is still being worked on and this issue is on hold.

anmurali commented 8 months ago

Still held, started a convo in C+ channel here

Also adding a BZ backup for my OOO

melvin-bot[bot] commented 8 months ago

Current assignee @mallenexpensify is eligible for the Bug assigner, not assigning anyone new.

melvin-bot[bot] commented 8 months ago

@mananjadhav, @anmurali, @mallenexpensify, @jasperhuangg, @bernhardoj Whoops! This issue is 2 days overdue. Let's get this updated quick!

jasperhuangg commented 8 months ago

Still on HOLD for https://github.com/Expensify/App/pull/29199

jasperhuangg commented 8 months ago

Same ^

jasperhuangg commented 8 months ago

Same ^^

melvin-bot[bot] commented 8 months ago

@mananjadhav, @anmurali, @mallenexpensify, @jasperhuangg, @bernhardoj Whoops! This issue is 2 days overdue. Let's get this updated quick!

jasperhuangg commented 8 months ago

^^^ Same

anmurali commented 8 months ago

Still on hold

anmurali commented 7 months ago

Refactor is still in progress so this issue is still on hold.

mallenexpensify commented 7 months ago

Commented on the PR this is held on

https://github.com/Expensify/App/pull/29199#issuecomment-2026256884

Also.. are we sure that the behavior isn't intended? Whenever I click on a chat in LHN, my assumption is that the chat opens and the cursor is in the compose box. I understand that there was a pop up open previously but we w ouldn't expect that pop up to stay open if we navigated to another chat then immediately back again, right?

Maybe there are other examples I'm unaware of where we keep the pop up while taking other actions?

mallenexpensify commented 7 months ago

Putting on #vip-vsb for now, since this doesn't have to do with money

bernhardoj commented 7 months ago

The issue we have here is that the composer is focused when we open a second popover.

Click on plus besides the composer to open Actions popup Right (click) on any chat in LHN to open popup on LHN Observe that even though popup is open in LHN, app still focuses on composer

mallenexpensify commented 7 months ago

Thanks @bernhardoj , I def missed that part :ohnothing: I was able to reproduce

I feel like it's a pretty rare edge case and it doesn't affect functionality. BUT... having that second popup from the LHN hang around isn't idea. Let's wait for this to come off hole then I'll retest then decide what to do with it.

jasperhuangg commented 7 months ago

Still on HOLD

anmurali commented 6 months ago

Still held

anmurali commented 6 months ago

@ntdiary will resume working on https://github.com/Expensify/App/pull/29199 This issue is still blocked on https://github.com/Expensify/App/pull/29199

jasperhuangg commented 5 months ago

Still blocked on https://github.com/Expensify/App/pull/29199

mallenexpensify commented 5 months ago

https://github.com/Expensify/App/pull/29199 Held on above and it looks like it's still a draft

ntdiary commented 5 months ago

Try to refactor the logic regarding refocusing when the modal is closed #29199 Held on above and it looks like it's still a draft

We split that large PR into several smaller ones. phase1 #35572 has already been merged, and phase2 #42423 is under review. :)

jasperhuangg commented 5 months ago

@ntdiary Awesome! Sounds like a good idea to split up the PR. Thanks for the update.

mallenexpensify commented 5 months ago

https://github.com/Expensify/App/pull/42423

Above merged so we're getting close

mallenexpensify commented 4 months ago

The PR we were holding on closed without being fixed. I threw a retest-weekly on here to see if the bug exists six months later.

Try to refactor the logic regarding refocusing when the modal is closed

jasperhuangg commented 4 months ago

@mallenexpensify @lanitochka17 Are we still retesting this on weekly cadence?

ntdiary commented 4 months ago

This bug still exists. I remember there was another PR that added a focus trap feature, which confines the focus within the popover(or RHP?), but it seems to be ineffective in this situation. 🤔

https://github.com/Expensify/App/assets/8579651/d5f9bfa1-91f1-4fcf-8905-0d4288896d39

mallenexpensify commented 4 months ago

@jasperhuangg retests take a weekish. @ntdiary and I are both able to reproduce so the bug still exists. I think we need to decide between

Close it cuz it's a minor bug that doesn't affect functionality (I might actually prefer to be able to type and have the text fill the compose box vs nothing happening if I type)
Review existing proposals to see if we want to accept any
Add Help Wanted to get more proposals
Something.

I don't think we need/want to increase the price though

ntdiary commented 4 months ago

I remember our app has an interesting feature: even if the main input box isn't focused, pressing the keyboard will still automatically focus it, allowing for smooth typing. 😄

https://github.com/Expensify/App/assets/8579651/c26c19cb-7f9d-424a-a3fc-b28df85b8fc4

Expensify / App