[HOLD for payment 2023-09-20] [$1000] Showing the report action menu for two messages at the same time

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Navigate to chat
2. Send any message(e.g. test 1), send another message(e.g. test 2)
3. Hover over the first sent message (in this example, test 1)
4. Now press on the composer and then press the up key so that the last message will show edit composer(in this case, test 2), and then hover over test 1 message with your cursor
5. Now edit the message and press enter and observe that both messages are showing the action menu

Expected Result:

The action menu should be displayed on the last focused message, not on the two messages at the same time

Actual Result:

The action menu is showing for two messages at the same time

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• [ ] Android / native
• [ ] Android / Chrome
• [ ] iOS / native
• [ ] iOS / Safari
• [x] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

Version Number: 1.2.94-0 Reproducible in staging?: Need reproduction Reproducible in production?: Need reproduction 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: Any additional supporting documentation

https://user-images.githubusercontent.com/43996225/229856932-dfebf217-6e63-4451-8513-66f4158e672a.mov

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017c1c2a84205bafde
• Upwork Job ID: 1661084222111625216
• Last Price Increase: 2023-08-31
• Automatic offers:
• wildan-m | Contributor | 26513246
• jayeshmangwani | Reporter | 26513247

[MelvinBot]

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

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] 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
• [x] 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.
• [x] 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.
• [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[dylanexpensify]

Reproduced, but updated OP to be a bit clearer of repro steps

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[jjcoffee]

Proposal

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

The mini report action context menu is sometimes displaying multiple times when we only want to display it once.

What is the root cause of that problem?

When we initiate the edit, if we move our mouse inside the ReportActionItem, the onMouseEnter event is fired, which sets isHovered to true. However, when we press enter to save the edit, isHovered is not updated via any event (e.g. onMouseLeave or onBlur), even though the mouse "jumps" to be hovering over the message above. The message the mouse jumps to does trigger onMouseEnter, which results in both being highlighted (as isHovered is true for both).

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

The most straight forward way to solve this is to trigger a blur event on the TextInput (via this.textInput.blur()), to update isHovered when we're handling pressing Enter to save the message here:

https://github.com/Expensify/App/blob/2831da1ddaf03ea077de3a24e16745f24a142d05/src/pages/home/report/ReportActionItemMessageEdit.js#L232-L244

The same happens if you press Esc so we'd want to handle that there too.

https://user-images.githubusercontent.com/27287420/230180840-95d11107-6a1f-48da-8925-6ba2cad94392.mp4

What alternative solutions did you explore? (Optional)

Another way to implement the same solution above is to call setIsHovered directly, by passing it along with the hovered state here:

https://github.com/Expensify/App/blob/6f6cf651d626932ac5b344b62805fc084f1356f8/src/components/Hoverable/index.js#L107-L111

Then we can call it in the same place(s) as the main solution above.

[Pujan92]

Seems this was solved earlier by @s77rt here and issue might be occurring again after this PR

[s77rt]

@Pujan92 The linked issue is slightly different than this one. The linked PR seems unrelated too.

[Santhosh-Sellavel]

Seems this was solved earlier by @s77rt here and issue might be occurring again after this PR

@Pujan92 Why do you think that breaks can you explain?

[Pujan92]

Sorry, my bad. Based on the title I just mapped it to an earlier issue. but yes this issue is slightly different.

[Santhosh-Sellavel]

Cool, there is no rush, In the future please check twice before you mention PR that caused a regression or include any specifics about the changes that were made which caused a regression, thanks!

[parasharrajat]

Heads Up! We are looking to remove onBlur logic from Hoverable #16052. I am yet to confirm this but chances are evolving.

[Santhosh-Sellavel]

@parasharrajat Should we keep this on hold?

[Santhosh-Sellavel]

bump @parasharrajat

[parasharrajat]

I will update you soon on this. Thanks.

[MelvinBot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[Santhosh-Sellavel]

bump @parasharrajat

[parasharrajat]

Checking this today in few minutes.

[Santhosh-Sellavel]

@parasharrajat Any update now?

[parasharrajat]

I gonna post one in 2 hours. Doing the research now about onBlur.

[Santhosh-Sellavel]

Any update on this buddy @parasharrajat

[parasharrajat]

So we are not removing onBlur on the Hoverable component for now. thoughts were that https://github.com/Expensify/App/issues/16923#issuecomment-1497986875 proposal is indirectly trying to trigger the onBlur on Hoverable via textInput blur which led to https://github.com/Expensify/App/issues/16923#issuecomment-1500324521.

@Santhosh-Sellavel Please continue. Thanks for waiting.

[parasharrajat]

@Santhosh-Sellavel Can you confirm that reverting https://github.com/Expensify/App/pull/14377 fixes this issue?

It fixed it for me but I wanna be sure. I am trying cleanup Tooltip and Hoverable components.

Note: I know that it will reproduce https://github.com/Expensify/App/issues/14080.

[Santhosh-Sellavel]

@parasharrajat Able to reproduce even after reverting

https://user-images.githubusercontent.com/85645967/232175669-2006c7fc-b963-4de2-8e59-6d2473b2b27c.mov

[MelvinBot]

@thienlnam, @dylanexpensify, @Santhosh-Sellavel Huh... This is 4 days overdue. Who can take care of this?

[MelvinBot]

@thienlnam, @dylanexpensify, @Santhosh-Sellavel Still overdue 6 days?! Let's take care of this!

[dylanexpensify]

@parasharrajat mind updating this please?

[parasharrajat]

@Santhosh-Sellavel Got, it. I just tested I can also reproduce it on https://github.com/Expensify/App/pull/17422

[Santhosh-Sellavel]

@parasharrajat So what's the plan here for how to move this forward, can this be treated separately?

[Santhosh-Sellavel]

Low bandwidth, I'm gonna unassign myself here. @dylanexpensify Please add a new C+ here by re-applying the external label.

@parasharrajat Since you already have the context of this and related issues as well it's better you take this one for ease of communication/handling, if you have bandwidth please a leave comment to pick this one up, thanks!

cc: @thienlnam

[parasharrajat]

Sure gonna check this today.

[MelvinBot]

📣 @parasharrajat You have been assigned to this job by @thienlnam! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[thienlnam]

Can you please summarize where we are at with this issue when you get the chance?

[parasharrajat]

@jjcoffee How does your proposal solve the issue?

[parasharrajat]

@thienlnam Few days back I created a PR that I thought to solve this issue but it's not. So we are back to waiting for the proposal status on this issue.

[jjcoffee]

@parasharrajat It's a bit hacky, but basically by calling this.textInput.blur() we're forcing the Hoverable isHovered state to false (when the Enter key is pressed in this case).

I guess we could also pass the setIsHovered function along with the hovered state here:

https://github.com/Expensify/App/blob/6f6cf651d626932ac5b344b62805fc084f1356f8/src/components/Hoverable/index.js#L107-L111

Then we could just call it directly in ReportActionItemMessageEdit instead of via this.textInput.blur().

The only downside of both of these is that they seem to indirectly stop onMouseEnter from firing again on the same message, e.g. if your cursor position is around where the buttons are (but not hovering over a button - not sure why this is!), so the original message doesn't get "re-hovered". Moving the mouse away and back over brings it back. See demo:

https://user-images.githubusercontent.com/27287420/235200088-d494604d-2151-4de5-8438-0368ea247e07.mp4

I don't know if that's a deal-breaker?

[parasharrajat]

but basically by calling this.textInput.blur() we're forcing the Hoverable isHovered state to false (when the Enter key is pressed in this case).

How does this work? That is what I am asking.

[parasharrajat]

@jjcoffee Bump.

If the solution depends on the onBlur logic of Hoveravble then I will suggest finding alternatives. I am inclined to remove the onBlur from Hovrable as it is solving a bug currently and is not very clear how it works. If the target is not focusable, it won't work.

[parasharrajat]

Meanwhile, we are waiting for proposals.

[jjcoffee]

@parasharrajat It does depend on the onBlur logic yes, as it triggers the onBlur event (which calls setIsHovered) in Hoverable here:

https://github.com/Expensify/App/blob/6f6cf651d626932ac5b344b62805fc084f1356f8/src/components/Hoverable/index.js#L83-L91

As I mentioned, we can also just pass setIsHovered along to the children and then we'd be able to call it directly - essentially the same solution via a different route.

[jjcoffee]

Updated my proposal with that alternative to keep things tidy!

[parasharrajat]

@dylanexpensify Can you please increase the price for the job?

[jjcoffee]

@parasharrajat Sorry if it's annoying to ask, but do you have any feedback on my alternative proposal? It'd be good to know if you think something is wrong with it.

[parasharrajat]

Yeah, Sure. I think exposing the internal method to the outside scope is not a good idea for a component that is supposed to be inclusive of its behavior. Hoverable is set to define the hover state for all children and it should single-handedly manage that. Allowing children to manipulate sounds like a hack. Thus I would like hear more ideas on it.

[MelvinBot]

@parasharrajat @thienlnam @dylanexpensify this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[MelvinBot]

Current assignee @parasharrajat is eligible for the Internal assigner, not assigning anyone new.

[MelvinBot]

@parasharrajat @thienlnam @dylanexpensify this issue is now 4 weeks old and preventing us from maintaining WAQ. This should now be your highest priority. Please post below what your plan is to get a PR in review ASAP. Thanks!

[parasharrajat]

@dylanexpensify We can move this to external and increase the price. Technically the price should have been incremented now.