[HOLD for payment 2023-06-21] Tooltip getting stuck with combination of esc key and hover effect

[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. Resize window to get view of small screen where LHN not show on left side.
2. Open any chat room.
3. Hover back button and wait for tooltip to visible.
4. Press ESC key.

Expected Result:

Tooltip shouldn’t be stuck

Actual Result:

Tooltip stuck on screen until we open same screen again.

Workaround:

unknown

Platforms:

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

• [ ] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

Version Number: 1.2.47-0 Reproducible in staging?: y Reproducible in production?: y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/210281400-e222d110-81ac-4c0a-b8ce-b67e56e02ed0.mp4

https://user-images.githubusercontent.com/43996225/210281410-dfbf2920-e86d-4529-8ca1-8ef33885c818.mov

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017baca264df205faf
• Upwork Job ID: 1610608308003094528
• Last Price Increase: 2023-01-04

[melvin-bot[bot]]

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

[zanyrenney]

Is this still happening @kavimuru ? I can't reproduce this on production or staging.

[kavimuru]

@zanyrenney Able to reproduce

https://user-images.githubusercontent.com/43996225/210415456-a18ba38d-37cb-4d4e-9584-df6578f976c8.mp4

[sketchydroide]

I'll take a look at this

[zanyrenney]

Thanks @sketchydroide !

[s77rt]

Is this supposed to be open for external contributors?

[jatinsonijs]

I think it is going to fix in Tooltip refactor in this PR

https://github.com/Expensify/App/pull/14048

What you think @s77rt

[s77rt]

@jatinsonijs Not exactly.

That PR will fix this behaviour: Hover + Pressing ENTER but not Hover + Pressing ESC

Because pressing the enter key will focus the tooltip and we blur it later (hide it). While pressing the esc key won't do anything related (won't emit an event that cancels the effects of mouseenter)

---

I do have a solution for this though, it's quiet simple but aren't we ignoring such type of issues (keyboard related issues). Source

[jatinsonijs]

Yes @s77rt I don't think it's related to Keyboard actually. Keyboard is the just way to reproduce it. It is also getting stuck with just normal back swipe which we do normally in safari to go back. I think its happening due to Freeze component.

[sketchydroide]

I was going to do it myself, and I set as internal

[s77rt]

It is related to the <Freeze /> component but you will have to use keyboard to reproduce it, that makes it out of scope.

we are going to :snowflake: freeze :ice_cube: any GH reports that relate to keyboard navigation or accessibility

---

I'm not opposed to getting this fixed if it's not going to be much of trouble.

[melvin-bot[bot]]

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.

[jatinsonijs]

It is related to the <Freeze /> component but you will have to use keyboard to reproduce it, that makes it out of scope.

we are going to ❄️ freeze 🧊 any GH reports that relate to keyboard navigation or accessibility

As I said I can reproduce it even without esc key. without using keyboard also.

[sketchydroide]

Maybe I'll work on it this week

[zanyrenney]

If that's the case @sketchydroide should we be updating the priority of this to weekly rather than have it as daily?

[zanyrenney]

hey @sketchydroide what's the latest here?

[sketchydroide]

sorry, came back from OOO this week and priorities have been a bit switched I don't know if I have time to look at it at the moment, let's get this as an external. There are already some ideas floating.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[s77rt]

Proposal

https://github.com/Expensify/App/blob/91c7467c865d949c8d96e8a6cd999b11a896d85c/src/components/Hoverable/index.js#L48

-            this.setState({isHovered}, isHovered ? this.props.onHoverIn : this.props.onHoverOut);
+            this.setState({isHovered});
+            if (isHovered) {
+                this.props.onHoverIn();
+            } else {
+                this.props.onHoverOut();
+            }

RCA

The issue is happening due to <Freeze /> taking effect and suspending the <ReportScreen /> preventing the callback of setState from getting executed.

The easiest way to solve this is to call the callback immediately so we avoid <Freeze />'s effect.

[parasharrajat]

I see. Did you check for side-effects?

[s77rt]

@parasharrajat The side effect is that we may call the callbacks before the value is updated in the state. As far as I tested this didn't break anything.

[situchan]

There was already same proposal here but it seems @rushatgabhane not accepted that solution.

[s77rt]

I think that was because of the lack of the root cause, I have provided the root cause above.

[situchan]

I think that was because of the lack of the root cause, I have provided the root cause above.

But #12025 is not related to Freeze. Freeze is used only for mobile but mentioned issue happened on desktop/web.

[s77rt]

Freeze is used for all platforms

[situchan]

Freeze is used for all platforms

No! As far as I know, freeze is used only for small screens - mWeb, native. The purpose of it is to improve performance when hidden on the screen but still rendered. On web/desktop, no need because always rendered and shown by user.

Here are all usages: https://github.com/Expensify/App/blob/91c7467c865d949c8d96e8a6cd999b11a896d85c/src/pages/home/ReportScreen.js#L224-L235 https://github.com/Expensify/App/blob/91c7467c865d949c8d96e8a6cd999b11a896d85c/src/pages/home/sidebar/SidebarLinks.js#L178

[situchan]

And I am heard that freeze will be deprecated in the future

[s77rt]

Desktop/Web can have small screens.

[s77rt]

I have already proved that <Freeze /> is the cause on another issue, if in doubt, pass freeze={false} on all <Freeze /> and the issue will be gone.

[situchan]

https://github.com/Expensify/App/issues/12025#issuecomment-1398658220 Sounds like this is a regression from https://github.com/Expensify/App/issues/13146

[s77rt]

No, it's not. You can verify by getting back to any point in the commit history, you will get the same issue, this issue existed for a long time.

[getusha]

It's not a regression @situchan

[situchan]

Here's original commit of using setState callback by @roryabraham

[s77rt]

How is that supposed to help fix the issue @situchan ?

[situchan]

I see. Did you check for side-effects?

I was just trying to answer this

[s77rt]

Ah okay, so the proposal will recreate this issue https://github.com/Expensify/Expensify/issues/147472

[roryabraham]

The issue is happening due to <Freeze /> taking effect

I don't have all the context on this issue, but I've heard that <Freeze> is slated to be removed as part of the react-navigation reboot project. (cc @mountiny can you confirm?) We're getting rid of the Drawer entirely and using a Stack instead, so Freeze no longer makes sense. <Freeze> has caused a ton of issues because it doens't really respect the React lifecycle, so maybe we should put this on HOLD for the react-navigation reboot rather than implementing a workaround.

[sketchydroide]

that sounds good to me @roryabraham.

[mountiny]

Yep, I think we should be removing the react-freeze with the navigation-reboot

[zanyrenney]

@mountiny just to confirm, so this should remain on hold for now or what are the next steps here? Thanks!

[mountiny]

Yep, removed the Help Wanted label, let's wait for the refactor to be done (hopefully around one month from now) and then we can come back to this issue and tackle it if it still exists.

[sketchydroide]

I think this is still on hold

[zanyrenney]

If this is still on hold, should we update the prioritty?

[sketchydroide]

yeah lets

[sketchydroide]

I think it's still on hold

[zanyrenney]

reassigning Eng as Andre isn't with us anymore.

[MelvinBot]

Triggered auto assignment to @cristipaval (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[zanyrenney]

oops, in removing andre, I removed the new assignment too - manually reassigning - thanks @cristipaval