App
App copied to clipboard
Expensify
[$1000] Bug: Tooltip gets stuck on the page reported by @Puneet-here
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:
- Decrease the screen size
- Hover over any contact at chat navigator
- When the tooltip appears open the chat
- Hover at back button until the tooltip shows up then press back
Expected Result:
The tooltip should go away
Actual Result:
The tooltip should doesn't go away
Workaround:
unknown
Platform:
Where is this issue occurring?
- Web
- Desktop App
Version Number: 1.2.18-2 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/196830711-438ca9f7-1bbd-44df-80a2-3bf7e0b9ff01.mp4 https://user-images.githubusercontent.com/43996225/196830700-9a924829-e70f-4121-bd50-80e148d1fc91.mov
Expensify/Expensify Issue URL: Issue reported by: @Puneet-here Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1666210802145239
Triggered auto assignment to @laurenreidexpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
Triggered auto assignment to @chiragsalian (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.
looks like a solid external @chiragsalian ? can you do the honours pls :)
Current assignee @laurenreidexpensify is eligible for the External assigner, not assigning anyone new.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (External)
Current assignee @chiragsalian is eligible for the External assigner, not assigning anyone new.
Proposal This seems to be happen because sometimes the setState callback which triggers the tooltip to hide is never called despite the state being updated. It seems to happen when combined with the navigation drawer opening/closing.
https://github.com/Expensify/App/blob/b2acb8f48e6ca05cbce64771e9499f85c4872018/src/components/Hoverable/index.js#L52-L54
A solution to this is to remove the setState callback and call setState followed by the onHoverIn/Out callbacks.
if (isHovered !== this.state.isHovered && !(isHovered && this.hoverDisabled)) {
- this.setState({isHovered}, isHovered ? this.props.onHoverIn : this.props.onHoverOut);
+ this.setState({isHovered});
+ if (isHovered) {
+ this.props.onHoverIn();
+ } else {
+ this.props.onHoverOut();
+ }
}
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.
@Ollyws thanks for your proposal!
sometimes the setState callback which triggers the tooltip to hide is never called despite the state being updated
I can't find any evidence that points to callback not being called. You could try console logging.
You're close but we need to fix the root cause because not using callbacks will lead to race conditions.
@rushatgabhane Let me first explain that I'm new here and I'm learning so I apologize ahead of time if I'm wrong. I looked at @Ollyws code and tested it locally. It does work for me. I'm not sure exactly how it could lead to a race condition like you suggested. Since setState is always called now and not wrapped in a conditional, it seems like a solid solution so I'm wondering what I am missing here. Thanks!
@mmarceau I think he meant because setState is asynchronous hence why you need the callbacks. My solution didn't address the root cause either. I may try and diagnose the root cause when I have time but feel free to have a crack at it.
Bumped in Slack https://expensify.slack.com/archives/C01GTK53T8Q/p1667826775048499?thread_ts=1666210802.145239&cid=C01GTK53T8Q and set price to $1000
Proposal
It is quite interesting to see this detailed comment https://github.com/Expensify/App/blob/c02031489b46200f95d19bfda1e0c048cb99bb45/src/components/Hoverable/index.js#L62-L67 What do we miss? https://github.com/Expensify/App/blob/c02031489b46200f95d19bfda1e0c048cb99bb45/src/components/Tooltip/index.js#L187-L192
+ resetsOnClickOutside={true}
https://github.com/Expensify/App/blob/c02031489b46200f95d19bfda1e0c048cb99bb45/src/components/Hoverable/index.js#L71-L78
- if (this.wrapperView && !this.wrapperView.contains(event.target) && this.props.resetsOnClickOutside) {
- this.setIsHovered(false);
- }
+ if ((this.wrapperView && !this.wrapperView.contains(event.target)) || this.props.resetsOnClickOutside) {
+ this.setIsHovered(false);
+ }
Result
https://user-images.githubusercontent.com/75031127/200337163-1ef1fbc0-f5ff-4986-807a-d22cadc27c75.mp4
@chiragsalian I like @getusha's proposal 👍
🎀 👀 🎀 C+ reviewed
@getusha I'd split those conditions into two if statements for better readability.
@chiragsalian I like @getusha's proposal +1
ribbon eyes ribbon C+ reviewed
@getusha I'd split those conditions into two if statements for better readability.
That sounds great. We can split it.
📣 @getusha You have been assigned to this job by @chiragsalian! 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 📖
@getusha if you didn't already know, there's a 50% bonus if we can get the PR merged within 3 business days.
So let's ship this 🚢
Thank you @chiragsalian @rushatgabhane for assigning me. I will apply for the job on upwork now. the price is not up to date on UW hope it will get updated soon.
Current assignee @laurenreidexpensify is eligible for the Bug assigner, not assigning anyone new.
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:
- [ ] @laurenreidexpensify A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:
- [ ] @rushatgabhane @chiragsalian The PR that introduced the bug has been identified. Link to the PR:
- [ ] @laurenreidexpensify 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:
- [ ] @laurenreidexpensify A discussion in #contributor-plus 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:
- [ ] @laurenreidexpensify Payment has been made to the issue reporter (if applicable)
- [ ] @laurenreidexpensify Payment has been made to the contributor that fixed the issue (if applicable)
- [ ] @laurenreidexpensify Payment has been made to the contributor+ that helped on the issue (if applicable)