[HOLD for payment 2023-10-24] [$500] Web - App keeps both right click LHN popup and delete message popup open together

[izarutskaya]

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. Open the app
2. Open any report
3. Focus on composer
4. Right on any report in LHN to open right click LHN popup
5. Type anything and press enter to send
6. Click up arrow to edit the sent message
7. Remove all content using backspace and click enter
8. Observe that app does not close right click LHN popup and opens delete popup too along with it.

Expected Result:

App should close existing open popup on opening any other popup in the app

Actual Result:

App keeps both right click LHN popup and delete popup open together

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: v1.3.74-2

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

windows.chrome.both.right.click.LHN.and.delete.popup.open.together.mp4

mac.chrome.desktop.right.click.LHN.and.delete.popup.open.together.mov

20230927_195554.mp4

Expensify/Expensify Issue URL:

Issue reported by: @dhanashree-sawant

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695654750958139

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b7d70b5aeb6fba4a
• Upwork Job ID: 1707062073662357504
• Last Price Increase: 2023-09-27
• Automatic offers:
• jjcoffee | Reviewer | 27017549
• Dhanashree-Sawant | Reporter | 27017552

Issue Owner

Current Issue Owner: @dylanexpensify

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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]

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

[ikevin127]

Proposal

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

App keeps both right click LHN popup and delete popup open together.

What is the root cause of that problem?

The problem is that within the PopoverReportActionContextMenu.js component, the LHN popup is not being closed before the delete message popup is opened.

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

Within the PopoverReportActionContextMenu.js component I called the hideContextMenu function within the showDeleteModal function which makes sure the LHN popup is closed before the delete message popup is being opened:

   const showDeleteModal = (reportID, reportAction, shouldSetModalVisibility = true, onConfirm = () => {}, onCancel = () => {}) => {
        onCancelDeleteModal.current = onCancel;
        onComfirmDeleteModal.current = onConfirm;

        reportIDRef.current = reportID;
        reportActionRef.current = reportAction;

-        setShouldSetModalVisibilityForDeleteConfirmation(shouldSetModalVisibility);
-        setIsDeleteCommentConfirmModalVisible(true);
+        hideContextMenu(() => {
+            setShouldSetModalVisibilityForDeleteConfirmation(shouldSetModalVisibility);
+            setIsDeleteCommentConfirmModalVisible(true);
+        });
    };

What alternative solutions did you explore? (Optional)

A different way to go about this would be to add an event listener for keydown such that whenever specific keyboard interactions are performed outside of LHN and the LHN popup is open we would close it by calling hideContextMenu.

Videos

web.mp4

[astrohunter62]

Proposal

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

The LHN does not receive focus when clicking on it.

What is the root cause of that problem?

In the OptionRowLHN, default actions are prevented when the mouse is clicked.

App/src/components/LHNOptionsList/OptionRowLHN.js

Lines 162 to 169 in a65e62a

	onMouseDown={(e) => {
	if (!e) {
	return;
	}
	// Prevent losing Composer focus
	e.preventDefault();
	}}

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

We can add DomUtils.blurActiveElement() into the onSecondaryInteraction within OptionRowLHN.

App/src/components/LHNOptionsList/OptionRowLHN.js

Line 170 in 24aa7c1

onSecondaryInteraction={(e) => showPopover(e)}

And adding ReportActionComposeFocusManager.focus(); to this point to ensure that the ReportActionComposer is in focus.

App/src/components/PopoverProvider/index.js

Lines 21 to 28 in a65e62a

	const closePopover = React.useCallback((anchorRef) => {
	if (!activePopoverRef.current || (anchorRef && anchorRef !== activePopoverRef.current.anchorRef)) {
	return;
	}
	activePopoverRef.current.close();
	activePopoverRef.current = null;
	setIsOpen(false);
	}, []);

Results:

28324.safari.webm

28324.chrome.webm

What alternative solutions did you explore? (Optional)

If you prefer not to have this behavior, you have the option to call the hideContextMenu function within the showDeleteModal function to ensure the LHN popup is closed before opening the delete message popup.

[kaushiktd]

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

The LHN popup is still open after opening the delete message popup.

What is the root cause of that problem?

The root cause of the issue is in the file PopoverReportActionContextMenu.js, within the showDeleteModel function. The issue occurs because the delete modal is opened without closing it first, resulting in both the delete modal and any previously open popups remaining visible simultaneously.

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

Add this line of code which closes LHN popup before opening the delete popup
https://github.com/Expensify/App/blob/main/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js#L264

if (this.state.isPopoverVisible) {
            this.hideContextMenu();
     }

Video:-

https://drive.google.com/file/d/1ifmq4_JOlrPy7gMaT7ahxPTgGQuQRFMl/view?usp=sharing

[melvin-bot]

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

[dylanexpensify]

@jjcoffee mind giving these proposals a review today?

[jjcoffee]

Oh I'm sorry, not sure how this one slipped past me! Reviewing today!

[jjcoffee]

Looks like the correct RCA here is in @astrohunter62's proposal, as we are always keeping focus on the composer when we should be defocusing since we're opening the right click menu.

It looks like this is a regression from this PR, which was attempting to ensure that the composer refocuses when clicking an already-open chat in the LHN. So, we want to keep that behaviour but right clicking LHN should lose focus from composer.

Could you update your proposal based on the above @astrohunter62? Also a side-note, can you please link to the code snippet you mention in your proposal - makes it much easier to review!

The other two proposals are decent, but miss out because of not identifying the correct RCA.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[astrohunter62]

@jjcoffee
I updated my proposal #28324 (comment)

[jjcoffee]

@astrohunter62 Perfect, thanks! I think the code block you're proposing to remove would cause a regression from this PR though.

[astrohunter62]

@jjcoffee Thank you.
If you prefer not to remove it, you can alternatively insert DomUtils.blurActiveElement(); into the onSecondaryInteraction within OptionRowLHN.

onSecondaryInteraction={(e) => {
    showPopover(e);
    DomUtils.blurActiveElement();  // add
}}

[melvin-bot]

📣 @jjcoffee 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @astrohunter62 You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

[astrohunter62]

@jjcoffee Can you review this PR?

[jjcoffee]

Fix is merged!

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

• Fix/28324 : App keeps both right click LHN popup and delete message popup open together #28846

If no regressions arise, payment will be issued on 2023-10-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

For reference, here are some details about the assignees on this issue:

• @jjcoffee requires payment offer (Reviewer)
• @astrohunter62 requires payment
• @dhanashree-sawant requires payment offer (Reporter)

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:

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

[astrohunter62]

@jjcoffee Hi,
When can I get an offer?

[jjcoffee]

• The PR that introduced the bug has been identified. Link to the PR: Fix: Composer does not refocus on clicking the same chat in LHN #21552 (also prevent focusing on composer while popover is open #28953 which attempted to fix it)
• 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: Fix: Composer does not refocus on clicking the same chat in LHN #21552 (comment) (original PR) and prevent focusing on composer while popover is open #28953 (comment) (attempted fix PR)
• 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: N/A - just needed more thorough testing
• Determine if we should create a regression test for this bug. Yes
• 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.

Regression test proposal

1. Open any report and click on the composer to focus it.
2. Right click on any report in LHN to open the context menu.
3. Verify that the composer loses focus.
4. Click on the same chat in LHN and verify that the composer regains focus.

Do we agree 👍 or 👎

[jjcoffee]

@astrohunter62 Payment will be coming soon as I've now completed the checklist! cc @dylanexpensify 🙇

[dylanexpensify]

Payment summary:

• Reporter: @dhanashree-sawant $50
• Contributor: @astrohunter62 $250
• Contributor+: @jjcoffee $250

Please request money or apply to upwork job!

[dylanexpensify]

@astrohunter62 sent offer!

@dhanashree-sawant and @jjcoffee payment sent!

[jjcoffee]

@dylanexpensify Oops! Looks like you overpaid (I received $500) - is there anything I can do my end?

[astrohunter62]

@dylanexpensify
I have sent my work. Could you approve it?

[melvin-bot]

@MariaHCD, @jjcoffee, @dylanexpensify, @astrohunter62 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[dylanexpensify]

@jjcoffee ah I requested half back for you to refund! It was easier than sending a new offer, waiting for you to accept, etc. haha! @astrohunter62 looking now!

[dylanexpensify]

payments sent!

[jjcoffee]

@dylanexpensify Oooh, interesting, for some reason Upwork didn't send a notification with the refund request. Refunded now!