[HOLD for payment 2024-07-25] [$250] mWeb - Chat - The emoji picker is closed and user unable to select emoji

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.5-3
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/4701464
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to https://staging.new.expensify.com/home
2. Tap on a report
3. Send a message
4. Long press the message sent and tap emoji plus icon

Expected Result:

The emoji picker must not be closed and user must be able to select emoji

Actual Result:

The emoji picker is closed and user unable to select emoji

Workaround:

Unnown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6535856_1720444503614.Screenrecorder-2024-07-08-18-40-05-542.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012d2a5157510d29ca
• Upwork Job ID: 1811800348003336181
• Last Price Increase: 2024-07-12
• Automatic offers:
• ikevin127 | Reviewer | 103132063

Issue Owner

Current Issue Owner: @muttmuure

[melvin-bot]

Triggered auto assignment to @muttmuure (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

@muttmuure FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

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

[bernhardoj]

Proposal

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

The emoji picker is closed immediately when opening it from the context menu.

What is the root cause of that problem?

In the modal component, we set a popstate listener which will close the modal when triggered if shouldHandleNavigationBack is true (it's only true for emoji picker).

App/src/components/Modal/index.tsx

Lines 46 to 49 in dd96852

	if (shouldHandleNavigationBack) {
	window.history.pushState({shouldGoBack: true}, '', null);
	window.addEventListener('popstate', handlePopStateRef.current);
	}

It's triggered when we call back here when the modal is closed if the state contains shouldGoBack property that we push above.

App/src/components/Modal/index.tsx

Lines 23 to 29 in dd96852

	const hideModal = () => {
	setStatusBarColor(previousStatusBarColor);
	onModalHide();
	if ((window.history.state as WindowState)?.shouldGoBack) {
	window.history.back();
	}
	};

However, the back is triggered even though we don't close the emoji picker. That's because it's triggered from the context menu hide. When we show the emoji picker, the context menu is closed which triggers the back. shouldGoBack exists in the history state because the emoji picker is shown first which already pushes the new state that sets shouldGoBack to true.

Long-press comment -> context menu shown -> press add emoji reaction -> push state, shouldGoBack: true -> context menu hides -> history.back -> triggers popstate -> emoji picker closes

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

We can wait until the context menu is closed before showing the emoji picker using Modal.close. We can apply it here so it's applied to every component that shows an emoji picker, or we can just apply it in context menu case.

App/src/libs/actions/EmojiPickerAction.ts

Lines 52 to 66 in dd96852

	function showEmojiPicker(
	onModalHide: OnModalHideValue,
	onEmojiSelected: OnEmojiSelected,
	emojiPopoverAnchor: EmojiPopoverAnchor,
	anchorOrigin?: AnchorOrigin,
	onWillShow: OnWillShowPicker = () => {},
	id?: string,
	activeEmoji?: string,
	) {
	if (!emojiPickerRef.current) {
	return;
	}
	emojiPickerRef.current.showEmojiPicker(onModalHide, onEmojiSelected, emojiPopoverAnchor, anchorOrigin, onWillShow, id, activeEmoji);
	}

Modal.close(() => emojiPickerRef.current.showEmojiPicker(onModalHide, onEmojiSelected, emojiPopoverAnchor, anchorOrigin, onWillShow, id, activeEmoji));

What alternative solutions did you explore? (Optional)

Only call back if shouldHandleNavigationBack is true, but I think it won't work if in the future multiple modal enable shouldHandleNavigationBack.

App/src/components/Modal/index.tsx

Lines 26 to 28 in dd96852

	if ((window.history.state as WindowState)?.shouldGoBack) {
	window.history.back();
	}

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~012d2a5157510d29ca

[melvin-bot]

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

[ikevin127]

@bernhardoj Thanks for your proposal.

We can wait until the context menu is closed before showing the emoji picker using Modal.close. We can apply it here so it's applied to every component that shows an emoji picker, or we can just apply it in context menu case.

Before I can decide on the RCA / solution, can you please expand on the main solution mentioned above with code example ?
I need enough specifics to be able to apply the solution myself locally and test the fix.

[bernhardoj]

@ikevin127 added the code example

[ikevin127]

Thank you for the update!

@bernhardoj's updated proposal looks good to me. I verified the RCA as correct and the proposed main solution fixes the issue as per the expected result (the emoji modal will open when the button is tapped).

Video

Android: mWeb
android-mweb.mp4

I think it's safe to go with the version which adds Modal.close() within the showEmojiPicker function, such that this issue is covered in all places where showEmojiPicker is currently used.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

📣 @ikevin127 🎉 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

[aldo-expensify]

No overdue, issue assigned

[bernhardoj]

PR is ready

cc: @ikevin127

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

• Fix emoji picker closes when open it from report context menu add reaction button #45449

If no regressions arise, payment will be issued on 2024-07-25. 🎊

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

• @bernhardoj requires payment through NewDot Manual Requests
• @ikevin127 requires payment automatic offer (Reviewer)

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

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

[ikevin127]

• [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: 35756 close emoji when navigation back #38693
• [@ikevin127] 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/38693/files#r1685518680
• [@ikevin127] 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.
• [@ikevin127] Determine if we should create a regression test for this bug.
• [@ikevin127] 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. Send a message to any chat.
2. Long-press on the message.
3. Press the add reaction button.
4. Verify that the emoji picker opens and remains open.

Do we agree 👍 or 👎.

[ikevin127]

cc @muttmuure

[bernhardoj]

Requested payment in ND.

[muttmuure]

Handling

[muttmuure]

$250 - @ikevin127 C+
$250 - @bernhardoj C

[muttmuure]

Kevin has been paid in Upwork

[JmillsExpensify]

$250 approved for @bernhardoj