[$500] Chat - Unable to close Emoji-reacted popover when click outside, near the bottom of popover

[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. Go to any chat
2. Hover on any chat message and send a any Emoji
3. Right click on an emoji reacted of a message
4. Verify that and Emoji-reacted popover is displaying
5. Click outside, but near the bottom of popover, horizontally

Expected Result:

The Emoji-reacted popover should be closed if we click outside.

Actual Result:

The Emoji-reacted popover is not closed, until we click other outside area.

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.57-5

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

Screen.Recording.2023-08-16.At.07.19.46.mp4

Recording.1342.mp4

Expensify/Expensify Issue URL:

Issue reported by: @hoangzinh

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a318eb91f3f42c7c
• Upwork Job ID: 1699231586202316800
• Last Price Increase: 2023-09-06
• Automatic offers:
• mollfpr | Reviewer | 26614588
• samh-nl | Contributor | 26614590
• hoangzinh | Reporter | 26614591

[melvin-bot]

Triggered auto assignment to @anmurali (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

[samh-nl]

Proposal

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

Unable to close Emoji-reacted popover when click outside, near the bottom of popover

What is the root cause of that problem?

We are preventing the popover from closing when clicking in the popover, but also the anchor. In this case, the entire View component where the reactions sit in is used as the anchor, causing clicks to be ignored in this area.

App/src/components/Reactions/ReportActionItemEmojiReactions.js

Lines 114 to 116 in f2a952d

	totalReactionCount > 0 && (
	<View
	ref={popoverReactionListAnchor}

App/src/components/PopoverProvider/index.js

Lines 31 to 47 in f2a952d

	const listener = (e) => {
	if (
	!activePopoverRef.current ||
	!activePopoverRef.current.ref ||
	!activePopoverRef.current.ref.current ||
	activePopoverRef.current.ref.current.contains(e.target) ||
	(activePopoverRef.current.anchorRef && activePopoverRef.current.anchorRef.current && activePopoverRef.current.anchorRef.current.contains(e.target))
	) {
	return;
	}
	const ref = activePopoverRef.current.anchorRef;
	closePopover(ref);
	};
	document.addEventListener('click', listener, true);
	return () => {
	document.removeEventListener('click', listener, true);
	};

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

We can redefine the anchor to be more specific. Instead of the entire View component, we in this case can use only the EmojiReactionBubble component, which is the button that the user clicks to open the popover.

In ReportActionItemEmojiReactions, this would entail the following changes:

- const popoverReactionListAnchor = useRef(null);
+ const popoverReactionListAnchors = useRef({});

const onReactionListOpen = (event) => {
-    reactionListRef.current.showReactionList(event, popoverReactionListAnchor.current, reactionEmojiName, props.reportActionID);
+    reactionListRef.current.showReactionList(event, popoverReactionListAnchors.current[reactionEmojiName], reactionEmojiName, props.reportActionID);
};

<EmojiReactionBubble
-    ref={props.forwardedRef}
+    ref={(ref) => (popoverReactionListAnchors.current[reaction.reactionEmojiName] = ref)}

(we do not use props.forwardedRef)

Fixed result:

chrome_jpNYSrJnZT.mp4

What alternative solutions did you explore? (Optional)

Option 1. A variation of this is to move showReactionList into EmojiReactionBubble so that we don't have to keep track of different references but just 1. However, this component is also used independently without popover functionality, so it makes less sense to put this functionality in here.
Option 2. We can remove this condition, so clicking on the anchor is not ignored and also results in closing the popover.

-                 (activePopoverRef.current.anchorRef && activePopoverRef.current.anchorRef.current && activePopoverRef.current.anchorRef.current.contains(e.target))

However, we would have to check other popovers to ensure that removing this condition does not have unintended effects.

[jeet-dhandha]

Proposal

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

• Reaction List is not closing when clicking on ReportActionItemEmojiReactions section.

What is the root cause of that problem?

• The issue is that when pressing on "emoji render" view the ref which is used to open the popover is not the same as the ref which is used to close the popover. So to fix this we should have same ref for both open and close. Like we did for MoneyRequestAction and ReportPreview in ReportActionItem.

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

• In ReportActionItem pass contextMenuAnchor ref to ReportActionItemEmojiReactions as a prop with value as popoverAnchorRef.

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

Lines 410 to 411 in 6b1a667

	<ReportActionItemEmojiReactions
	reportActionID={props.action.reportActionID}

• In ReportActionItemEmojiReactions remove popoverReactionListAnchor ref from whole file and use props.contextMenuAnchor.current in onReactionListOpen instead of popoverReactionListAnchor.current. Also remove popoverReactionListAnchor from <View> too.

App/src/components/Reactions/ReportActionItemEmojiReactions.js

Lines 97 to 99 in 6b1a667

	const onReactionListOpen = (event) => {
	reactionListRef.current.showReactionList(event, popoverReactionListAnchor.current, reactionEmojiName, props.reportActionID);
	};

Now we will need to make some changes in BasePopoverReactionList:

• We will create a new ref called this.reactionListContextMenuAnchor = React.createRef();.

• In showReactionList function we will use this.reactionListContextMenuAnchor instead of this.reactionListAnchor.

App/src/pages/home/report/ReactionList/PopoverReactionList/BasePopoverReactionList.js

Line 161 in 6b1a667

this.reactionListAnchor.current = reactionListAnchor;

• The we will update ref in getReactionListMeasuredLocation we will use this.reactionListContextMenuAnchor instead of this.reactionListAnchor.

App/src/pages/home/report/ReactionList/PopoverReactionList/BasePopoverReactionList.js

Lines 112 to 113 in 6b1a667

	if (this.reactionListAnchor.current) {
	this.reactionListAnchor.current.measureInWindow((x, y) => resolve({x, y}));

What alternative solutions did you explore? (Optional)

• N/A

[melvin-bot]

@anmurali Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[anmurali]

Will reproduce and slap the label on tomorrow

[jeet-dhandha]

Bump @anmurali

[melvin-bot]

@anmurali Eep! 4 days overdue now. Issues have feelings too...

[anmurali]

It looks like the area immediately below the popover is some kinda "dead" zone!

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[thepranays]

What's the issue with @samh-nl solution ? Why it is not merged ?

[melvin-bot]

📣 @thepranays! 📣
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:

1. 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.
2. 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.
3. 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>

[mollfpr]

There are some context ref issues with the ReportActionItemEmojiReactions component.

@jeet-dhandha What is the issue? Please put more explanation for the RCA so that it can be aligned with your proposed solution and help to understand your proposal.

[mollfpr]

I tested on @samh-nl proposal, and it's working well.

I agree with the RCA; the solution is fixing the root cause. Let's go with @samh-nl proposal 🚀

🎀 👀 🎀 C+ reviewed!

[melvin-bot]

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

[jeet-dhandha]

adding

[mollfpr]

@jeet-dhandha I don't see any relation to using the context menu ref. Why do we need to use that for the emoji component?

So to fix this we should have same ref for both open and close.

I also don't understand the above. So currently the ref for open the emoji reaction are wrong?

[jeet-dhandha]

I don't have much knowledge on how ref's work (so will need your help in learning that) but i just re-checked my code and it could have created a regression so yeah we can go with @samh-nl 's solution. 👍

[mollfpr]

Friendly bump @madmax330 for the review.

[melvin-bot]

📣 @mollfpr 🎉 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]

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

[melvin-bot]

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

Offer link
Upwork job

[samh-nl]

PR is ready for review: #27231

On mobile devices we show the popover in a different way, so in the tests I have indicated to click anywhere outside the popover and verify that it closes. Feel free to suggest a better test step.

[melvin-bot]

🎯 ⚡️ Woah @mollfpr / @samh-nl, great job pushing this forwards! ⚡️

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

• when @samh-nl got assigned: 2023-09-11 17:09:12 Z
• when the PR got merged: 2023-09-13 15:32:33 UTC

On to the next one 🚀

[samh-nl]

It seems that the status of this issue is stuck at 'Reviewing'.

[samh-nl]

Bump

[samh-nl]

@madmax330 @anmurali Friendly bump

[mollfpr]

Weird Melvin doesn't add the payment title here. I'll add the checklist soon.

[mollfpr]

The PR that introduced the bug has been identified. Link to the PR:

#15685

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:

https://github.com/Expensify/App/pull/15685/files#r1367226768

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:

The regression step should be enough.

Determine if we should create a regression test for this bug.
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.

Precondition: a report must contain a message that has at least one emoji reaction.

1. Go to a report
2. Go to a message that has at least one emoji reaction and right-click (web/desktop) or long-press (mobile) an emoji reaction.
3. Press on the empty area directly below the popover (web/desktop) or anywhere outside the popover (mobile).
4. Verify that the popover is closed.
5. 👍 or 👎

---

Friendly bump @anmurali. The PR has been in production since Sept 18th, and this is eligible for the urgency bonus.

[melvin-bot]

This issue has not been updated in over 15 days. @madmax330, @anmurali, @mollfpr, @samh-nl eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[anmurali]

Everyone's been paid with speed bonus where applicable since this is an old pending payment.
Regression test proposed https://github.com/Expensify/Expensify/issues/336286