[HOLD for payment 2024-12-03] [$250] Composer - Emoji picker does not open when opening context menu and clicking emoji picker

[IuliiaHerets]

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.51-1
Reproducible in staging?: Y
Reproducible in production?: N
Issue was found when executing this PR: #50974
Email or phone of affected tester (no customers): applausetester+kh081006@applause.expensifail.com
Issue reported by: Applause Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Open report.
3. Send a message.
4. Right click on the message.
5. Click on the emoji picker in the composer.
6. Right click on the message.
7. Click on the emoji picker in the composer.

Expected Result:

Emoji picker will open.

Actual Result:

Emoji picker does not open.

Workaround:

Unknown

Platforms:

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6639177_1729286544151.20241019_051934.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021848230166570821208
• Upwork Job ID: 1848230166570821208
• Last Price Increase: 2024-10-28
• Automatic offers:
• shubham1206agra | Reviewer | 104727454

Issue Owner

Current Issue Owner: @MitchExpensify

[melvin-bot]

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

[melvin-bot]

💬 A slack conversation has been started in #expensify-open-source

[melvin-bot]

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

[github-actions]

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

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

[bernhardoj]

Proposal

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

Emoji picker doesn't show when we open emoji picker first, and then context menu, and then emoji picker button again.

What is the root cause of that problem?

When we open the emoji picker and then context menu, you can notice that the emoji picker button tooltip won't show. It's because isPopoverRelatedToTooltipOpen is not updated properly.

App/src/components/Tooltip/PopoverAnchorTooltip.tsx

Lines 11 to 32 in 66cf824

	const isPopoverRelatedToTooltipOpen = useMemo(() => {
	// eslint-disable-next-line @typescript-eslint/dot-notation
	const tooltipNode = (tooltipRef.current?.['_childNode'] as Node | undefined) ?? null;
	if (
	isOpen &&
	popover?.anchorRef?.current &&
	tooltipNode &&
	((popover.anchorRef.current instanceof Node && tooltipNode.contains(popover.anchorRef.current)) || tooltipNode === popover.anchorRef.current)
	) {
	return true;
	}
	return false;
	}, [isOpen, popover]);
	if (!shouldRender || isPopoverRelatedToTooltipOpen) {
	return children;
	}
	return (
	<BaseTooltip

isPopoverRelatedToTooltipOpen tells whether the tooltip element contains/is the same as the visible/active popover anchor. It's to prevent the tooltip of a button that shows the popover from showing while the popover is active. For example, if we open the emoji picker, the emoji picker button tooltip won't show.

So, currently, isPopoverRelatedToTooltipOpen for the emoji picker button is true. When we press the emoji picker button again, isPopoverRelatedToTooltipOpen will update from true to false (because the context menu is closed) which will re-render the emoji picker button, so the press/click event is never received.

App/src/components/Tooltip/PopoverAnchorTooltip.tsx

Lines 27 to 38 in 66cf824

	if (!shouldRender || isPopoverRelatedToTooltipOpen) {
	return children;
	}
	return (
	<BaseTooltip
	// eslint-disable-next-line react/jsx-props-no-spreading
	{...props}
	ref={tooltipRef}
	>
	{children}
	</BaseTooltip>

isPopoverRelatedToTooltipOpen doesn't update properly because it depends on a (active popover) ref (other than the isOpen state).

App/src/components/Tooltip/PopoverAnchorTooltip.tsx

Line 25 in 66cf824

}, [isOpen, popover]);

When we open an emoji picker and then the context menu, the isOpen state of the popover stays true, so the memo is not recalculated.

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

We need to create a new state to store the active popover to re-trigger the isPopoverRelatedToTooltipOpen memo. Because we only want to get the anchor ref, we can store the anchor ref only to the state instead of the whole popover element.

App/src/components/Tooltip/PopoverAnchorTooltip.tsx

Line 19 in 66cf824

((popover.anchorRef.current instanceof Node && tooltipNode.contains(popover.anchorRef.current)) || tooltipNode === popover.anchorRef.current)

First, in PopoverProvider, create the new state.

App/src/components/PopoverProvider/index.tsx

Lines 21 to 24 in 66cf824

	function PopoverContextProvider(props: PopoverContextProps) {
	const [isOpen, setIsOpen] = useState(false);
	const activePopoverRef = useRef<AnchorRef | null>(null);

const [activePopoverAnchor, setActivePopoverAnchor] = useState(null);

Then, set the popover anchor state when a popover is shown.

App/src/components/PopoverProvider/index.tsx

Lines 105 to 111 in 66cf824

	const onOpen = useCallback(
	(popoverParams: AnchorRef) => {
	if (activePopoverRef.current && activePopoverRef.current.ref !== popoverParams?.ref) {
	closePopover(activePopoverRef.current.anchorRef);
	}
	activePopoverRef.current = popoverParams;
	setIsOpen(true);

setActivePopoverAnchor(popoverParams.anchorRef.current);

Clear it here too.

App/src/components/PopoverProvider/index.tsx

Lines 25 to 32 in 66cf824

	const closePopover = useCallback((anchorRef?: RefObject<View | HTMLElement | Text>): boolean => {
	if (!activePopoverRef.current || (anchorRef && anchorRef !== activePopoverRef.current.anchorRef)) {
	return false;
	}
	activePopoverRef.current.close();
	activePopoverRef.current = null;
	setIsOpen(false);

Then, pass the state to the context value,

App/src/components/PopoverProvider/index.tsx

Lines 116 to 124 in 66cf824

	const contextValue = useMemo(
	() => ({
	onOpen,
	close: closePopover,
	popover: activePopoverRef.current,
	isOpen,
	}),
	[onOpen, closePopover, isOpen],
	);

const contextValue = useMemo(
    () => ({
        onOpen,
        close: closePopover,
        popover: activePopoverRef.current,
        popoverAnchor: activePopoverAnchor,
        isOpen,
    }),
    [onOpen, closePopover, isOpen, activePopoverAnchor],
);

Last, replace all popover.anchorRef.current to popoverAnchor.

App/src/components/Tooltip/PopoverAnchorTooltip.tsx

Lines 8 to 25 in 66cf824

	const {isOpen, popover} = useContext(PopoverContext);
	const tooltipRef = useRef<BoundsObserver>(null);
	const isPopoverRelatedToTooltipOpen = useMemo(() => {
	// eslint-disable-next-line @typescript-eslint/dot-notation
	const tooltipNode = (tooltipRef.current?.['_childNode'] as Node | undefined) ?? null;
	if (
	isOpen &&
	popover?.anchorRef?.current &&
	tooltipNode &&
	((popover.anchorRef.current instanceof Node && tooltipNode.contains(popover.anchorRef.current)) || tooltipNode === popover.anchorRef.current)
	) {
	return true;
	}
	return false;
	}, [isOpen, popover]);

const {isOpen, popoverAnchor} = useContext(PopoverContext);

const isPopoverRelatedToTooltipOpen = useMemo(() => {
    // eslint-disable-next-line @typescript-eslint/dot-notation
    const tooltipNode = (tooltipRef.current?.['_childNode'] as Node | undefined) ?? null;

    if (
        isOpen &&
        popoverAnchor &&
        tooltipNode &&
        ((popoverAnchor instanceof Node && tooltipNode.contains(popoverAnchor)) || tooltipNode === popoverAnchor)
    ) {
        return true;
    }

    return false;
}, [isOpen, popoverAnchor]);

[bernhardoj]

This doesn't happen in prod because, in prod, you can't open the chat context menu while an emoji picker is visible. But this issue happens in prod if you open the LHN context menu. So, I posted my proposal above because it's an existing issue.

web.mp4

[MonilBhavsar]

@bernhardoj looks like it is coming from regression from this PR #50974?

[bernhardoj]

It happens after #50974, yes, but it's an existing issue which I explained in my comment above.

[MonilBhavsar]

Thanks for the context! So this is not really a blocker

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

@MonilBhavsar, @MitchExpensify, @shubham1206agra Whoops! This issue is 2 days overdue. Let's get this updated quick!

[MitchExpensify]

@shubham1206agra, how does @bernhardoj 's proposal look to you?

[shubham1206agra]

So, currently, isPopoverRelatedToTooltipOpen for the emoji picker button is true.

@bernhardoj Can you tell me why is it true?

[bernhardoj]

PR is ready

cc: @shubham1206agra

[MitchExpensify]

How is the PR looking @shubham1206agra ?

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

• Fix pressing emoji button while a context menu is shown doesn't open the emoji picker #51938

If no regressions arise, payment will be issued on 2024-12-03. 🎊

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

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

[melvin-bot]

@shubham1206agra @MitchExpensify @shubham1206agra The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[melvin-bot]

@MonilBhavsar, @MitchExpensify, @bernhardoj, @shubham1206agra Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[MitchExpensify]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
• 2b. Reported on staging (eg. found during regression or PR testing)
• 2d. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] 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:

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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:

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

Regression Test Proposal Template

• [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue:

Regression Test Proposal

Precondition:

Test:

Do we agree 👍 or 👎

[MitchExpensify]

@shubham1206agra please complete the BZ steps above and accept this offer: https://www.upwork.com/nx/wm/offer/105238174

[bernhardoj]

Requested in ND.

[JmillsExpensify]

Waiting on the payment summary.

[melvin-bot]

@MonilBhavsar, @MitchExpensify, @bernhardoj, @shubham1206agra Whoops! This issue is 2 days overdue. Let's get this updated quick!

[MitchExpensify]

Payment summary:

• $250 @bernhardoj requires payment through NewDot Manual Requests
• $250 @shubham1206agra requires payment automatic offer (Reviewer)

Reminder about the BZ steps, @shubham1206agra : #51114 (comment)

[shubham1206agra]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
• 2b. Reported on staging (eg. found during regression or PR testing)
• 2d. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] 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 when emoji picker is opened, right-clicking on message opens and closes context menu #50974 (comment)

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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: Not Required

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

• [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue:

Regression Test Proposal

Precondition:

• Test should only be on large screen view (Web/Desktop)

Test:

1. Open App
2. Open any report.
3. Send a message.
4. Right-click on the message.
5. Click on the emoji picker in the composer.
6. Right-click on the message.
7. Click on the emoji picker in the composer.
8. Observe that the popover is opening correctly.

Do we agree 👍 or 👎

[JmillsExpensify]

$250 approved for @bernhardoj

[shubham1206agra]

@JmillsExpensify Can you pay me here https://www.upwork.com/nx/wm/offer/105238174?

[MitchExpensify]

Paid and contract ended @shubham1206agra