[HOLD for payment 2023-03-14] [$1000] Report item remains hovered even when mouse is moved away from it.

[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. Hover over a message other than the first one in a group of message in the chat history (i.e any message that does not have the author avatar alongside it).
2. Move the cursor to the mini context menu (top right corner).
3. Right click on the mini context menu.
4. Close the big context menu by clicking anywhere outside the highlighted row.

Expected result:

When clicked outside the item to close the main context menu, the mini context menu should disappear along with the hovered row.

Actual result:

Both the hovered row and mini context menu remain visible.

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:

mac.chrome.repro.mov

Screen.Recording.2023-02-19.at.2.59.26.AM.1.mov

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01bff08adf85d6111c
• Upwork Job ID: 1628009672196374528
• Last Price Increase: 2023-03-16

[MelvinBot]

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

[trjExpensify]

Okay, I can repro this reliably:

Took me a bit to figure out the steps, so I'm updating the OP to stress it's any message that isn't the first in a group of messages.

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[tienifr]

Proposal

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

Both the hovered row and mini context menu remain visible when we hover over the mini context menu and right click to open the main context menu, then click outside.

What is the root cause of that problem?

The root cause is when we right click, the isContextMenuActive will become true and then the MiniReportActionContextMenu will be hidden immediately, before the onMouseLeave of the Hoverable is triggered.

Due to this the Hoverable is stuck in the hovered: true state, even though the cursor already moves away from it.

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

To fix this we can simply remove this line

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

Line 168 in df3c6e9

&& !this.state.isContextMenuActive

When the main context menu is opened, the onMouseLeave of Hoverable will be triggered and hovered will become false, then MiniReportActionContextMenu will hide, and the row will lose it's hovered state as expected.

We don't need that line because hovered will always become false once the ContextMenu becomes active, and will hide the MiniReportActionContextMenu.

What alternative solutions did you explore? (Optional)

NA

Result

Working well after the fix:
https://user-images.githubusercontent.com/113963320/220355328-5eca2915-3bee-4481-ba3e-4e96e99b5766.mov

[getusha]

Proposal

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

There is a problem where the ReportItem remains in a "hovered" state even after we have closed the context menu. This occurs when we trigger the context menu by clicking on a context menu item and then click outside the menu to close it.

What is the root cause of that problem?

Previously, we were resetting the "hover" state of the ReportItem component by detecting a mousedown event and checking if the clicked element was outside of the "Hoverable" component. However, recently theresetHoverStateOnOutsideClick function was removed and replaced with the onBlur event to reset the hover state. The problem is that the onBlur event will not be triggered in the scenario where we click outside the context menu to close it after triggering it by right clicking on a context menu item. As a result, the ReportItem component will remain in the "hovered" state even after the context menu has been closed, causing this current issue.

There was a comment in the previously removed code that described the purpose of the resetHoverStateOnOutsideClick function. This comment provided clarity on the intended use-case of the function. which exactly relates to the issue we currently have here.

   /**
     * If the user clicks outside this component, we want to make sure that the hover state is set to false.
     * There are some edge cases where the mouse can leave the component without the `onMouseLeave` event firing,
     * leaving this Hoverable in the incorrect state.
     * One such example is when a modal is opened while this component is hovered, and you click outside the component
     * to close the modal.
     *
     * @param {Object} event - A click event
     */
    resetHoverStateOnOutsideClick(event) {

More details: here

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

We can bring back the resetHoverStateOnOutsideClick function to fix the issue of the ReportItem component remaining in a "hovered" state even after the context menu has been closed. This function can detect if a user clicks outside the "Hoverable" component and reset the hover state accordingly.

What alternative solutions did you explore? (Optional)

[tienifr]

@getusha I think resetHoverStateOnOutsideClick was intentionally removed because it's a hack (see this comment).

Just curious if you find any other cases besides this one that requires resetHoverStateOnOutsideClick? Because it seems for this case the root cause is known and can be fixed without reviving the resetHoverStateOnOutsideClick workaround.

[getusha]

@tienifr Thanks for sharing the link. I have read it, but in my opinion, it is the way in which the functions are being used that makes it feel like a hack. Additionally, we should keep in mind that removing code without proper testing or researching its use case and intention can result in future issues, such as the one we are currently facing.

[s77rt]

I have taken the time to investigate this further and I can confirm that @tienifr RCA is correct and the suggested solution is good as well. The mouseLeave event is not firing because the target of that even which is the mini context menu no longer exists.

@tienifr I think you have a tiny typo here "when we right click, the isContextMenuActive will become false" I believe you meant to write true not false. Other than that, good work! 👍

[tienifr]

I think you have a tiny typo here "when we right click, the isContextMenuActive will become false" I believe you meant to write true not false

@s77rt yes, thanks for noticing that!

[bernhardoj]

@tienifr proposal will make a regression where the mini context menu does not hide when we right click the action item until we move the cursor (Safari only). I'm pretty sure it's because of the onMouseLeave Safari issue that both of you should remember from the tooltip issue here @aimane-chnaif @s77rt.

Screen.Recording.2023-02-25.at.06.26.03.mov

[s77rt]

@bernhardoj Nice catch! The rule is usually if you fix one thing and broke another then you fixed nothing but not sure if this applies here since it only effect Safari (Safari only emits mouseLeave on actual mouse movements).

[aimane-chnaif]

@bernhardoj Good catch! However, I don't see any bad effect since mini context menu disappears immediately when click/move the mouse or press Cmd key for shortcut. And the strange thing is this doesn't happen when right click inside mini context menu.

It seems relying on only event listeners (onMouseEnter, onMouseLeave, onBlur) without resetHoverStateOnOutsideClick causes many problems on safari. (related PRs: #14377, #14507, all of which are authored by @s77rt)

@tienifr do you have an updated proposal considering this safari issue?

My suggestions here:

Option 1:
If no perfect solution without hack, I'd suggest to go with accepted solution as long as no other regressions found than do nothing.
Option 2:
I think this is a regression from #14377 (deprecated resetHoverStateOnOutsideClick) which fixes #14080 and simply reverting #14377 will fix this issue. I also tested #14080 after reverting #14377 but not able to reproduce any more (@s77rt can you confirm?)

@roryabraham what do you think? (since you suggested to remove resetHoverStateOnOutsideClick here)

[MelvinBot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[MelvinBot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.79-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix/15300: Report item remains hovered even when mouse is moved away from it #15476

If no regressions arise, payment will be issued on 2023-03-14. 🎊

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

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

[MelvinBot]

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:

• [@aimane-chnaif / @yuwenmemon] The PR that introduced the bug has been identified. Link to the PR:
• [@aimane-chnaif / @yuwenmemon] 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:
• [@aimane-chnaif / @yuwenmemon] 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:
• [@trjExpensify] Determine if we should create a regression test for the bug.
• [@aimane-chnaif] If we decide to create a regression test for the bug, please propose the regression test steps to the appropriate location to ensure the same bug will not reach production again.
• [@trjExpensify] Review the proposed regression test steps and location.
• [@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[MelvinBot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[trjExpensify]

Okay, how are we looking with these items?

[@aimane-chnaif / @yuwenmemon] The PR that introduced the bug has been identified. Link to the PR:
[@aimane-chnaif / @yuwenmemon] 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:
[@aimane-chnaif / @yuwenmemon] 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:

To prepare for payments. It looks like melvin increased the price while the proposal was under review, so I'm going to change the title back to $1k. To confirm the amounts due:

• @tienifr: $1,000 + $500 #urgency bonus
• @aimane-chnaif: $1,000 + $500 #urgency bonus
• @allroundexperts: $250 for the bug report

Is that correct?

[MelvinBot]

Upwork job price has been updated to $1000

[aimane-chnaif]

• [@aimane-chnaif / @yuwenmemon] The PR that introduced the bug has been identified. Link to the PR:

#14377

• [@aimane-chnaif / @yuwenmemon] 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:

#14080 (comment)

• [@aimane-chnaif / @yuwenmemon] 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:

I think this is kind of edge case and I am not sure if this should be added to checklist.
Maybe #14080 (comment) is enough to avoid this type of bugs in the future.

[trjExpensify]

Yeah, I tend to agree that this is sort of edge case to include in the PR checklist.

[trjExpensify]

On a similar note, I don't think we'd add a regression test for this. It's the sort of visual bug that would be caught by executing other existing test cases. Do you guys agree with that?

[trjExpensify]

Okay, @aimane-chnaif, confirming these are correct right?

#15300 (comment)

[aimane-chnaif]

Okay, @aimane-chnaif, confirming these are correct right?

#15300 (comment)

You are asking about price revert?
Here's summary:
Acceptable proposal was made before price bump but we found out that solution causes regression on Safari.
I asked contributor to update proposal to fix regression so they did it. But it's still a workaround.
Known that it's minor visual bug which doesn't affect negative impact to users, we agreed to keep as is.
While this discussion, price was bumped.
So it makes sense to revert back to 1k on which original proposal was accepted.

[trjExpensify]

Yep, to confirm you agree these are the payments due. I also arrived at that conclusion.

Sent all the offers 👍

[trjExpensify]

Settled up with @aimane-chnaif!

[trjExpensify]

Settled up with @allroundexperts. Over to you, @tienifr and then we can close this out. 👍

[trjExpensify]

@tienifr settled up! closing this out, thanks everyone!