[HOLD for payment 2024-04-05] [$500] Chat - 3 dot menu overlaps with the emoji menu

[izarutskaya]

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: 1.4.44-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): [email protected]
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal Team

Action Performed:

1. Access staging.new.expensify.com
2. Sign into a valid account
3. Go to any chat
4. Send any message
5. Hover over the message and tap on the 3 dot menu

Expected Result:

User expects a modal to open but to be able to still see the previous modal

Actual Result:

The two modals are overlapping

Workaround:

Unknown

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017349bd168876c6ff
• Upwork Job ID: 1762967904272871424
• Last Price Increase: 2024-03-13
• Automatic offers:
• DylanDylann | Reviewer | 0
• dukenv0307 | Contributor | 0

[melvin-bot]

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

[izarutskaya]

@jliexpensify 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.

[izarutskaya]

We think that this bug might be related to #vip-vsb
CC @quinthar

[jliexpensify]

Yep, can repro and I think this should go in #vip-vsb - https://expensify.slack.com/archives/C066HJM2CAZ/p1709093964920179

[dubielzyk-expensify]

Hold off on fixing this until we get final confirmation.

My comment was that I don't think this is a bug because you can still hover other comments below and see the comment action bar. So unless we wanted to disable that, the overlay should stay as is.

Though if we wanted to we could lower the overlay in the Y position to make sure it covers the comment action menu of the comment you've clicked on. I'm checking with @Expensify/design to see if they have strong feelings.

[jliexpensify]

Sweet, thanks @dubielzyk-expensify - have dropped a hold on the title!

[shawnborton]

This is what I wrote in Slack: Oh interesting... I would expect this context menu to launch below the overflow icon if there is space, and if not, above the overflow icon in a way that wouldn't obscure the menu behind it at all. So I personally do think this is a bug.

[dannymcclain]

I agree with Shawn. I would expect it to sit in the same position as the emoji picker (but on the bottom in there's room).

Weren't we just talking about this in another issue a week or so ago?

Might be worth showing that like the emoji menu, this menu should have automatic viewport collision detection (which it seems like it already does).

[melvin-bot]

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

[melvin-bot]

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

[nexarvo]

Proposal

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

3 dot menu overlaps with the emoji menu.

What is the root cause of that problem?

Right now the location of overflow menu is determined by click position of cursor that's why it is not consistent.

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

We need to first pull the reference of 3 dot button in props. Through that reference we can determine the exact location of 3 dot button and can then align the overflow menu relatively and consistently. We can determine the position of 3 dot menu like this:

App/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.tsx

Lines 82 to 93 in be716dd

	/** Get the Context menu anchor position. We calculate the anchor coordinates from measureInWindow async method */
	const getContextMenuMeasuredLocation = useCallback(
	() =>
	new Promise<Location>((resolve) => {
	if (contextMenuAnchorRef.current && typeof contextMenuAnchorRef.current.measureInWindow === 'function') {
	contextMenuAnchorRef.current.measureInWindow((x, y) => resolve({x, y}));
	} else {
	resolve({x: 0, y: 0});
	}
	}),
	[],
	);

We can use the default anchor position as:

const DEFAULT_ANCHOR_ORIGIN = {
    horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.RIGHT,
    vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM,
};

First we need to check if we have space below the 3 dot menu for the overflow menu. This will help us determine whether we can anchor the overflow menu in bottom:

  // Check if the is space available from bottom of screen.
  const isSpaceAvailableBelow = (positionY: number, popOverMenuSize: number): boolean => {
      // Get the height of the viewport
      const viewportHeight: number = window.innerHeight;
      
      // Calculate the distance from the bottom of the viewport to the position
      const distanceToBottom: number = viewportHeight - positionY;
      
      // Check if there is enough space below the position
      return distanceToBottom >= popOverMenuSize;
  }

We also need to determine the size of overflow / popover menu. We can get this through the child component in:

App/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx

Lines 224 to 227 in d22196a

	<View
	ref={contentRef}
	style={wrapperStyle}
	>

function to calculate the height of popover menu:

PopOverMenuRef.current.measureInWindow()

Now that we have everything in place i.e. popOver menu height, 3 dot menu Button location. We can now use these to determine the location of the popOver menu location. This will give use consistent location of the popOver menu each time relative to 3 dot menu button:

calculateAnchorPosition(contextMenuAnchorRef.current, DEFAULT_ANCHOR_ORIGIN).then((position) => {
      const haveSpace = isSpaceAvailableBelow(position.vertical, popOverMenuHeight);
      if(haveSpace) {
          anchorOrigin = {
              horizontal: DEFAULT_ANCHOR_ORIGIN.horizontal,
              vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM
          }
      }
      else {
          anchorOrigin = {
              horizontal: DEFAULT_ANCHOR_ORIGIN.horizontal,
              vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP
          }
      }
      
      popoverAnchorPosition.current = position;
      resolve();
  });

What alternative solutions did you explore? (Optional)

Results

[dubielzyk-expensify]

Wonderful work @usman-ghani564, that looks great to me. Just to confirm that the spacing between the overflow menu is 8px?

Keen to get @Expensify/design feedback as well before settling in on this.

[dubielzyk-expensify]

Also I do see that the overflow menu is completely flush with the edge of the screen on the right hand side. Could we make sure that it's at least 20px from the edge? Looks a bit tight in the screenshot:

[dukenv0307]

Proposal

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

The two modals are overlapping

What is the root cause of that problem?

According to this expectation, there're 2 problems:

1. For the overflow context menu opened via 3 dots button, we're relying on the cursor position as the anchor, this will cause the opened context menu to overlap with the existing MiniEmojiContextMenu, because the cursor position is in vertically in the middle of the MiniEmojiContextMenu
2. The overflow context menu is being opened on the top by default instead of the bottom as expected result.

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

1. Rely on the three dots button component as the anchor, just like what we did with the EmojiPicker here (using emojiPopoverAnchor.current instead of the current cursor position)

This will require quite many changes but here're the main steps:

• In here, if the current context menu is the three dots button, pass a buttonRef, which will be passed as the ref of BaseMiniContextMenuItem here
• In here, if it's three dots button, pass an anchorRef param to onPress, which will be the buttonRef above
• In here, use the anchorRef?.current (the anchorRef above instead of the anchor which is the full ReportActionItem component)
• In here, pass another isOverflowMenu boolean to indicate that it's the overflow menu
• Allow using calculateAnchorPosition here if it's overflow menu using the boolean above
• In here, if it's overflow menu, pass the anchorOrigin as

{vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP, horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT}

so that the popover menu will have a fixed relative position at the bottom of the three dots button

2. Set the anchorAlignment -> vertical to CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM by default for the overflow context menu

And inside PopoverWithMeasuredContent, we'll check if the verticalShift will be !== 0 for the current anchorAlignment.vertical, if yes change the anchorAlignment.vertical to TOP by an internal state.

We can pass a prop to PopoverWithMeasuredContent to control this alignment "switching" behavior.

With this approach, there'll be no hardcoded value required, some minor style changes will be needed to accommodate the additional adjustments by the design team, but outlined above should be the core solution.

What alternative solutions did you explore? (Optional)

NA

[nexarvo]

yes, @dubielzyk-expensify we can make the menu to left like this:

We can take input from the design team to make sure the spacing is looking good.

Also, one thing that I want to mention here about the existing logic on which we determine the anchorPosition of menu on the screen. In the code we have PageX and PageY, these are the location where the user clicks / touches of the screen in the three dots to open the over flow menu. Now keeping in mind the size of the three dot button the PageX and PageY might vary on every click. Hence the anchorPosition of the menu will also change. In order to change existing behaviour we need to completely change how PopoverWithMeasuredContent component work. But the solution to that is we can add a base yOffset so that both menu never overlap. Here are the examples:

When we touch in the very bottom edge of three dot icon:

When we touch in the very top edge of three dot icon:

[shawnborton]

Interesting, I would think this one should always launch respective of the overflow icon and not the mouse position. And it should be aligned on the right edge of the mini menu.

[dannymcclain]

Interesting, I would think this one should always launch respective of the overflow icon and not the mouse position. And it should be aligned on the right edge of the mini menu.

+1.

Is there a way for us to make all our popovers (that are triggered from a button, not a right-click) have consistent positions relative to the button that spawned them? Looking at the screenshots on this issue, the position/spacing looks different from the Emoji menu which also looks different from the three-dot overflow at the top. What's the rule? Always 8px away from the trigger?

[dubielzyk-expensify]

Yeah, +1 to what @dannymcclain and @shawnborton said. The screenshot above looks too far off now. Basically where the red outline is below is where we want it to line up:

But like Danny said, this should be consistent with other parts of the app. It should be tied to the 3 dots and where the icon is, not the click itself.

Does that make sense?

[DylanDylann]

@dubielzyk-expensify I think the horizontal position of the context menu is aligned with the position of a three-dot menu in the header

Screen.Recording.2024-03-01.at.15.27.28.mov

[dukenv0307]

I'm working on the PR.

[melvin-bot]

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

[jliexpensify]

Hello @sonialiap - I am headed OOO from the 21st to 31st March so have used the auto-assigner for a buddy.

Assuming all goes well:

PAYMENT SUMMARY:

• C: @dukenv0307: $500
• C+: @DylanDylann $500

Upwork job

• PR is being worked on atm
• Payment may need to be made before I get back

Thanks!

[dukenv0307]

@DylanDylann The PR is ready for review.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

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

• Fix three-dot menu overlaps with emoji menu #38553

If no regressions arise, payment will be issued on 2024-04-05. 🎊

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

• @dukenv0307 requires payment automatic offer (Contributor)
• @DylanDylann 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:

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

[jliexpensify]

Back, unassigning Sonia!

[jliexpensify]

PAYMENT SUMMARY:

• C: @dukenv0307: $500
• C+: @DylanDylann $500 - bumping for checklist!

Upwork job

[DylanDylann]

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:

[@DylanDylann] The PR that introduced the bug has been identified. Link to the PR: NA
[@DylanDylann] 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: NA
[@DylanDylann] 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: NA
[@DylanDylann] Determine if we should create a regression test for this bug. Yes
[@DylanDylann] 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 a report
2. Send a message
3. Hover over the message and tap on three-dot menu
4. Verify that the menu is displayed above or below the three-menu icon and it doesn't overlap the mini context menu

Do we agree 👍 or 👎

[jliexpensify]

Paid and job closed!