[$8000] Context menu does not close when secondary menu opens or secondary menu should not open

[kavimuru]

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

---

Action Performed:

1. Go to any report
2. Right click to open the secondary menu
3. Right click again elsewhere on the screen

Expected Result

The previous secondary menu should close and replaced with a new one.

Actual Result

Previous secondary menu remains there. A new native secondary menu opens.

Workaround:

unknown

Platforms:

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

• MacOS / Chrome / Safari

Version Number: 1.2.74-0
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:

Screen.Recording.2023-02-19.at.3.07.34.AM.mov

Recording.104.mp4

Expensify/Expensify Issue URL:
Issue reported by: @allroundexperts
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1676758078417199

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017f298b5f370cb2ba
• Upwork Job ID: 1628420077151154176
• Last Price Increase: 2023-05-09

[MelvinBot]

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

[MelvinBot]

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

[zanyrenney]

Yup, I have had this happen before, so can confirm. adding labels.

[MelvinBot]

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

[MelvinBot]

Triggered auto assignment to @greg-schroeder (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[MelvinBot]

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

[MelvinBot]

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

[zanyrenney]

@greg-schroeder I'm going OOO which Is why I think this has assigned you as well of me as Bug0. Are you okay to buddy up on this one whilst I am out?

[priyeshshah11]

I don't think this is a bug nor it is worth fixing as you're interacting with the native browser right click action. Why would you expect there to be any change in the app in this case? It would have been an issue if the secondary menu wasn't focused & you're not able to click on it but that's not the case here.

Any solutions would be adding extra/unnecessary code to handle this, we should not put in any custom code/hacks to solve such things and let native things work as per it's default behaviour unless it's causing major concerns within the app.

[s77rt]

Proposal

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

Context menu does not close on second right click.

What is the root cause of that problem?

contextmenu event is not being intercepted/handled on PopoverReportActionContextMenu.

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

Listen to contextmenu event on PopoverReportActionContextMenu and in the callback preventDefault and hideContextMenu.

What alternative solutions did you explore? (Optional)

None

[aimane-chnaif]

@dangrous @greg-schroeder do you think this is worth fixing?
I don't find any negative impacts with current behavior.

[dangrous]

Yeah I'm not confident I know the answer to this question haha.

Personally, I would say this is a feature. I like always being able to access the right click menu and am annoyed when apps take it over. This seems like the best of both worlds. But, that's just my opinion, and not an official design statement or anything.

@greg-schroeder, how about you?

Maybe we ask the question in Slack and see what others think?

[dangrous]

As an example I just tested Gmail. Gmail has its own right click (if you click on an email) and if you right click elsewhere that moves around with you, and will never open the default right click menu unless you click on something that doesn't have that feature. And either way it still closes the original menu. Having two open at once does feel a bit weird, as I think about it more.

I feel like I've interacted with other apps that are the same, so if we want to fit in we might want to do that too, but that's not necessarily a reason to do it either.

[aimane-chnaif]

@dangrous good example! I was actually finding any example popular app which has its own custom context menu.
So it makes sense to prevent system context menu when click any area where our own context menu is applied.

[allroundexperts]

@aimane-chnaif @dangrous As another example, please check Google Drive. It also does not show the native context menu once the custom context menu is opened.

[allroundexperts]

Personally, when I'm using Google drive, I often use context menu switching when selecting different files. This is how I found this bug on our app in the first place. I copied one message using our context menu and then when I right clicked on another message (with the intention of copying that), the native context menu opened. Since I was used to doing this on Drive, this bug could be based on my bias only. Just letting you guys know.

[dangrous]

Okay, cool. So I'll make an issue for 1., which will also likely solve 2. I'll hold on creating one for 2., in case of that.

I'll also make one for 3.

@aimane-chnaif do you want to report 4. in channel so you get the bonus for finding it?

And then 5. will be done by the usual process.

[aimane-chnaif]

6. Able to type composer while popover is open (pre-existed bug)

Screen.Recording.2023-08-08.at.9.19.13.PM.mov

[dangrous]

Okay tooltip issue is linked above, and here's the Safari one.

#24297

Again, really happy that we only had a couple updates needed from this

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

• Add popovers without overlay #19011
• fix: broken attachments and avatar picker #24184

If no regressions arise, payment will be issued on 2023-08-16. 🎊

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:

• @allroundexperts does not require payment (Eligable for Manual Requests)
• @aimane-chnaif requires payment

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:

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

[greg-schroeder]

Processing payments today

[greg-schroeder]

This is a very complicated and long running issue. A few things:

• We agreed not to worry about the speed incentive on this one: context here

• There were some bugs that came up which is to be expected with something as large as this, but I don't think we should do any penalties related to those either

• There are a lot of people involved in this issue, so we need to make sure everyone who participated is compensated in a fair manner

[greg-schroeder]

Issue Reporter: @allroundexperts ($250)
Contributor: @allroundexperts ($8000)
C+: @aimane-chnaif ($8000)

Looks like @narefyev91 also helped out here as well, but that's outside the scope of our payment structure (Callstack)

[greg-schroeder]

Does everyone agree that we go with the flat amount here and ignore penalties/incentive bonus structure given the scope?

cc @aimane-chnaif @allroundexperts @dangrous

[allroundexperts]

I agree.

[greg-schroeder]

Hey @allroundexperts - just to make sure I understand - are all of the reported bugs/reports that came up as a result of this being handled in separate issues at this stage?

[allroundexperts]

Yes as far as I know. But the bugs won't have any payments. The follow-ups would have payments as anyone can work on those!

[greg-schroeder]

Okay you should be good to request ND payment then!

[greg-schroeder]

Offer sent to @aimane-chnaif

[greg-schroeder]

Okay @aimane-chnaif / @allroundexperts not sure if this one needs a regression test but just need to complete the checklist either way!

[aimane-chnaif]

As new feature, I don't think this requires regression test.

[greg-schroeder]

Agreed. I'll close this out then! NewDot payments handled separately - breakdown here: #15289 (comment)

[JmillsExpensify]

Reviewed the details for @allroundexperts. $8,250 approved for payment in NewDot based on the BZ summary above.