[$250] Web/Desktop - Chat - Focus is lost after deleting a message from edit mode

lanitochka17 commented 2 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: 9.0.7-4 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4725448 Email or phone of affected tester (no customers): fischer9966@gmail.com Issue reported by: Applause - Internal Team

Action Performed:

Start ND on Desktop App or go to staging.new.expensify.com
Login with any account
Open any chat and send a message
Press Arrow Up key on keyboard
Select All - Ctrl / Cmd + A
Press 'Delete' key
Press 'Enter' key
Confirm message deletion with 'Enter' key

Expected Result:

Message deleted. Focus is on composer field

Actual Result:

Focus lost after deleting message in edit mode

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/user-attachments/assets/67231f2f-46a8-4f10-93cf-7389de640c18

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01fe43add27dc504ef
Upwork Job ID: 1826277990269229690
Last Price Increase: 2024-09-18
Automatic offers:

brunovjk | Reviewer | 104052891

Issue Owner

Current Issue Owner: @brunovjk

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @johnmlee101 (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

github-actions[bot] commented 2 months ago

:wave: 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:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

lanitochka17 commented 2 months ago

We think that this bug might be related to #vip-vsp

thienlnam commented 2 months ago

This is a pretty edge case bug - don't think it needs to be a blocker

melvin-bot[bot] commented 1 month ago

@johnmlee101 Still overdue 6 days?! Let's take care of this!

melvin-bot[bot] commented 1 month ago

@johnmlee101 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

melvin-bot[bot] commented 1 month ago

@johnmlee101 12 days overdue. Walking. Toward. The. Light...

melvin-bot[bot] commented 1 month ago

This issue has not been updated in over 14 days. @johnmlee101 eroding to Weekly issue.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

code-with-shahzad commented 4 weeks ago

There can be something like handle by state while deleting due to re-render focus can change. or there can be something handle through searchparams...

melvin-bot[bot] commented 4 weeks ago

📣 @code-with-shahzad! 📣 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:

Make sure you've read and understood the contributing guidelines.
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.
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.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

ardent-dev commented 4 weeks ago

Hi All, this one was reproducible on staging and prod, but I was not able to reproduce it on dev main branch. Could this already be resolved?

brunovjk commented 4 weeks ago

You're right, I just tested it and at the last main (v9.0.23-0) it seems to no longer be reproducible:

https://github.com/user-attachments/assets/904b7bff-560d-410b-b5c7-db57b6b9261c

lanitochka17 commented 3 weeks ago

Issue is still reproducible

https://github.com/user-attachments/assets/4fe03dee-f213-40b2-8d6a-eb43f07a0a1d

brunovjk commented 3 weeks ago

Bumped into Slack for proposals.

melvin-bot[bot] commented 3 weeks ago

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

brunovjk commented 2 weeks ago

I can still reproduce it in staging. Bumped into Slack for proposals.

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 1 week ago

@johnmlee101, @brunovjk Whoops! This issue is 2 days overdue. Let's get this updated quick!

Shahidullah-Muffakir commented 1 week ago

Edited by proposal-police: This proposal was edited at 2023-10-11T18:27:00Z.

Proposal

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

When a user is in edit mode and deletes a message, the composer field loses focus. Note: This behavior is a new feature, not a regression.

What is the root cause of that problem?

The onConfirm callback (deleteDraft) that was passed to showDeleteModal is not being called correctly. https://github.com/Expensify/App/blob/89b9226fd6fa0884b8a9c9761759a787e820fcf8/src/pages/home/report/ReportActionItemMessageEdit.tsx#L305 You can see the issue in the code: https://github.com/Expensify/App/blob/1e8e4f480a038c0883adddad40f444e4b1090b6a/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.tsx#L267

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

We should change the code to:

callbackWhenDeleteModalHide.current = runAndResetCallback(onConfirmDeleteModal.current); because : In the original code we are wrapping the execution of runAndResetCallback in an anonymous function. This means the assignment to callbackWhenDeleteModalHide.current is now a function that, when executed, will:

Call runAndResetCallback(onConfirmDeleteModal.current). Assign the result of runAndResetCallback to onConfirmDeleteModal.current. However, the result of runAndResetCallback(onConfirmDeleteModal.current) is a noop function (() => {}), and we are assigning this noop function back to onConfirmDeleteModal.current. As a result:

onConfirmDeleteModal.current will get replaced with () => {}

In my suggested changes, we are directly invoking runAndResetCallback and assigning its result to callbackWhenDeleteModalHide.current. This ensures that the callback runs immediately, and the reset behavior works as intended.

Additionally, we can correct a few typos in the same file to prevent any potential errors in the future. In three places, onComfirmDeleteModal is written instead of onConfirmDeleteModal Here are the typos:

brunovjk commented 1 week ago

Thanks for the proposal @Shahidullah-Muffakir. However, I’m concerned that the root cause analysis isn't fully clear, and the proposed solution might cause regressions by triggering unnecessary focus() calls. Could you please provide more details on the RCA? I'll investigate deeper tomorrow, but can you confirm if this is a regression or has it always been like this (new feat)? Thanks.

Shahidullah-Muffakir commented 1 week ago

Thank you @brunovjk. I believe this is a new feature, not a regression.

Looking at the code, we manually focus the input in a few specific cases. For example, when a user tries to submit an empty message while editing, a delete confirmation modal appears. If the user cancels, the input is refocused because it lost focus when the modal was shown.

You can see that here: https://github.com/Expensify/App/blob/89b9226fd6fa0884b8a9c9761759a787e820fcf8/src/pages/home/report/ReportActionItemMessageEdit.tsx#L302-L303

However, when a message is actually deleted, we don’t refocus the input, which is why the behavior feels different. We can’t simply add the refocusing logic when the user confirms the deletion, because two different components are involved: one for writing a comment and another for editing a comment.

Since we don’t have direct access to the comment input from within the edit comment file, we need to handle refocusing by tracking when the comment value updates after a deletion. That can be done in this file: https://github.com/Expensify/App/blob/aaa60b9103af83aaa58222e3f36e8cbfbd57e5c3/src/pages/home/report/ReportActionCompose/ReportActionCompose.tsx#L124

There’s another place where we manually refocus the input when hiding the emoji picker modal: https://github.com/Expensify/App/blob/89b9226fd6fa0884b8a9c9761759a787e820fcf8/src/pages/home/report/ReportActionCompose/ReportActionCompose.tsx#L560-L567

I suggest we follow a similar approach to the one I outlined in my proposal. Alternatively, we could create a new key in Onyx, like commentDelete, and update it when the user confirms deletion. We could then track that value in useEffect and use it to refocus the input.

raza-ak commented 1 week ago

Proposal

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

Web/Desktop - Chat - Focus is lost after deleting a message from edit mode

What is the root cause of that problem?

The problem lies in the setUpComposeManagerFocus function, where the focus logic doesn't account for scenarios when returning to the Composer after closing the delete message modal. As a result, the Composer field loses focus when the modal is closed and the component becomes visible again.

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

To solve the issue, we need to adjust the setUpComposeManagerFocus function. The main goal is to:

Ensure the focus is properly restored when the Composer is visible after closing the modal.
Optimize how focus and re-renders are triggered, avoiding unnecessary operations.

Following changes ensure that when the modal is closed, the Composer field regains focus and provides a better user experience.

Above code changes are necessary due to following reasons. - Contextual Focus Management: Before: The focus management logic for context menu actions was mixed with the visibility logic, leading to reduced clarity. After: We separated the context menu focus management into a clean, modular callback function. This makes the code easier to maintain.

- Visibility Check Optimization: Before: There was no proper visibility check before restoring focus, which caused unnecessary focus attempts. After: We introduced a clear conditional check for isComposerVisible, ensuring that focus is only restored if the Composer is actually visible. This eliminates redundant focus attempts.

- Efficient Focus Handling for Editable Div: Before: The contentEditableDiv logic was scattered and unoptimized. After: We streamlined this by ensuring the contentEditable div is only focused if it exists. This simplifies the logic while ensuring focus is restored properly.

- Reduced Unnecessary Re-renders: Before: Re-renders were triggered regardless of necessity, potentially leading to performance inefficiencies. After: Re-renders (setShouldRerender) are now only triggered if a focus change occurs. This reduces unnecessary UI updates, improving performance.

I've tested all use cases in the following video. Following video shows that issue is fixed.

https://github.com/user-attachments/assets/f1c04741-3a02-492e-b59a-741ac0d5db90

brunovjk commented 1 week ago

Thanks for the proposal @raza-ak, I believe your RCA is not very correct, I tested your proposed solution, and it is triggering focus in the main composer unnecessarily:

https://github.com/user-attachments/assets/4312ae4f-99b3-4025-9915-f53baf34b09b

brunovjk commented 1 week ago

Thanks for clarifying @Shahidullah-Muffakir, although it makes sense to me, I'm still not 100% sure, could you put everything together in your proposal? That way I can ping an internal eng to take a look. Thanks.

Shahidullah-Muffakir commented 1 week ago

@brunovjk, I’ve updated my original proposal. I combined my earlier explanation with it. Let me know if you need anything else. Thanks!

brunovjk commented 1 week ago

@bernhardoj sorry to bother you, but can you please take a quick look at this issue here, do you think it might be related to your PR here somehow? Thanks a lot.

bernhardoj commented 1 week ago

I don't think it's related to that PR since that PR fix an auto-focus issue on mount.

raza-ak commented 1 week ago

@brunovjk I've updated my proposal and fixed all focus issues. Please review it again.

brunovjk commented 1 week ago

Thank you very much for confirming @bernhardoj.

brunovjk commented 1 week ago

Thanks for the update and your time @raza-ak, but I believe we still don't have the correct cause of the issue.

@Shahidullah-Muffakir I've been investigating and I believe that your RCA is also not adequate, sorry about that. When delete or edit a comment it seems to me that we have already taken care to update the composer focus after: https://github.com/Expensify/App/blob/e9642dc7829f912bbb20be6c0d89b3e6529df7b6/src/pages/home/report/ReportActionItemMessageEdit.tsx#L273-L278

When we edit the focus it is updated successfully (call deleteDraft): https://github.com/Expensify/App/blob/e9642dc7829f912bbb20be6c0d89b3e6529df7b6/src/pages/home/report/ReportActionItemMessageEdit.tsx#L308-L309

However, by leaving the comment empty, we should also update the focus (call deleteDraft): https://github.com/Expensify/App/blob/e9642dc7829f912bbb20be6c0d89b3e6529df7b6/src/pages/home/report/ReportActionItemMessageEdit.tsx#L303-L305

However deleteDraft is not being called in delete case

https://github.com/user-attachments/assets/9ede165a-c6d1-4850-9d6e-96528ee112c2

I haven't figured out why yet, @raza-ak and @Shahidullah-Muffakir Thoughts?

raza-ak commented 1 week ago

@brunovjk Did you watch my updated video in the proposal and did you test my latest code changes locally? I've fixed that issue and i believe i've shared the correct root cause of issue with you.

brunovjk commented 1 week ago

@raza-ak Yes, I have reviewed it, and although the changes seem to solve the problem I want to be sure of the root cause. I understand, I will take a closer look at your proposal and let you know if I find anything different.

melvin-bot[bot] commented 1 week ago

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

Shahidullah-Muffakir commented 1 week ago

@brunovjk I've identified the root cause of the bug. The issue was that the onConfirm callback (deleteDraft) which should be called when a user confirms the deletion of a message in the showDeleteModal, was not being invoked as expected.

The problem stemmed from a typo in the file: https://github.com/Expensify/App/blob/b76d91ca3b2f973da1bd0a76871d3b6f9ef588bf/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.tsx#L33

Here are the typos:

The variable onConfirmDeleteModal was incorrectly spelled as onComfirmDeleteModal in three instances. After correcting these typos, the functionality is now working as intended, and focus is properly regained after the deletion action.

I've tested the solution, and it resolved the issue. Thank you.

Shahidullah-Muffakir commented 1 week ago

@brunovjk Thanks for telling me about the deleteDraft function not working. At first, I tried to fix it but couldn't, so I stopped trying. But when you mentioned it again, it made me look harder. Because of that, I found out what was really wrong

brunovjk commented 1 week ago

Hi @Shahidullah-Muffakir, Thank you for identifying the typo in onComfirmDeleteModal. I corrected it across the file, but after testing, it doesn't seem to affect the functionality or resolve the issue. Upon further review, I couldn't find any references where this ref is actually used. Were you able to resolve it just by fixing the typo?

Shahidullah-Muffakir commented 1 week ago

@brunovjk I am sorry I forgot to mention one more small issue int the line https://github.com/Expensify/App/blob/b76d91ca3b2f973da1bd0a76871d3b6f9ef588bf/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.tsx#L267

it should be callbackWhenDeleteModalHide.current = runAndResetCallback(onConfirmDeleteModal.current);

You can test it with these changes , Thank you.

Shahidullah-Muffakir commented 1 week ago

@brunovjk, In the original code we are wrapping the execution of runAndResetCallback in an anonymous function. This means the assignment to callbackWhenDeleteModalHide.current is now a function that, when executed, will:

Call runAndResetCallback(onConfirmDeleteModal.current). Assign the result of runAndResetCallback to onConfirmDeleteModal.current. However, the result of runAndResetCallback(onConfirmDeleteModal.current) is a noop function (() => {}), and we are assigning this noop function back to onConfirmDeleteModal.current. As a result:

onConfirmDeleteModal.current will get replaced with () => {}

in my suggested changes, we are directly invoking runAndResetCallback and assigning its result to callbackWhenDeleteModalHide.current. This ensures that the callback runs immediately, and the reset behavior works as intended. Thank you.

brunovjk commented 1 week ago

Hi @Shahidullah-Muffakir, Thank you for following up and for your persistence in addressing the issue. I tested the latest change you suggested, and it seems like this indeed resolves the problem. The root cause appears to be how the callback was handled in callbackWhenDeleteModalHide, specifically assigning a reset function incorrectly. Could you please update your proposal with these changes? Once that's done, I'll submit it for internal review and assign you if they approve. Thanks again for your efforts!

Shahidullah-Muffakir commented 1 week ago

@brunovjk , Thank you for your feedback! I’ll update the proposal with the suggested changes. Regarding the typos in "onConfirmation," do you want to keep them as they are, as they are irrelevant now, or should I correct those as well, ? Thank you.

brunovjk commented 1 week ago

@brunovjk , Thank you for your feedback! I’ll update the proposal with the suggested changes. Regarding the typos in "onConfirmation," do you want to keep them as they are, as they are irrelevant now, or should I correct those as well, ? Thank you.

I believe we can fix them yes :D

Shahidullah-Muffakir commented 1 week ago

@brunovjk proposal https://github.com/Expensify/App/issues/45472#issuecomment-2336825317 updated, Thank you.

brunovjk commented 1 week ago

I think we can go with @Shahidullah-Muffakir's proposal now.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 week ago

Current assignee @johnmlee101 is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] commented 2 days ago

@johnmlee101, @brunovjk Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 2 days ago

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

melvin-bot[bot] commented 23 hours ago

@johnmlee101, @brunovjk 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

brunovjk commented 19 hours ago

Bumped on slack for help

Edit: 🎀👀🎀

Expensify / App