search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.55k stars 2.89k forks source link

[HOLD for payment 2023-06-01] [HOLD PR#16052][$4000] Tooltip doesn't disappear while page is scrolling #15757

Closed kavimuru closed 1 year ago

kavimuru commented 1 year ago

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. Login with any account.
  2. Go to any report that has a lots of request, click on one of those request to open Detail Page.
  3. Try to hover the user name to see the tooltip, after that scroll the page.
  4. Notice that tooltip doesn't disappear while page is scrolling.

Expected Result:

Tooltip should disappear while page is scrolling.

Actual Result:

Tooltip doesn't disappear while page is scrolling.

Workaround:

unknown

Platforms:

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

Version Number: 1.2.80-1 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:

https://user-images.githubusercontent.com/43996225/223875450-fc035282-c95f-43fd-93fa-48033fc6c352.mp4

https://user-images.githubusercontent.com/43996225/223875464-e402dcf5-59f0-4d30-8150-066cc9c97df8.mov

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~019546d7509b0c8a21
  • Upwork Job ID: 1636029730419101696
  • Last Price Increase: 2023-03-30
MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

isabelastisser commented 1 year ago

Hey @kavimuru, @hungvu193 did you copy the email? From your recording, I'm unsure if you did it, but it looks like it was the case. Please provide more context and I will continue to review this. Thanks!

isabelastisser commented 1 year ago

@kavimuru @hungvu193 please follow up on my question above, thanks! I'm unable to reproduce it.

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~019546d7509b0c8a21

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

kavimuru commented 1 year ago

@isabelastisser Account should have several IOUs (better with optional messages in the IOU )and hovering on of the emails in the details page and scrolling

goddev99 commented 1 year ago

Hi, I just checked the description that posted at Upwork. and checked the video. It is related with package's delay time. To fix it, there are two methods.

  1. set the delay time for the tooltip
  2. detect scroll action and hide the tooltip. I am ready to work the the task. Hoping to reply from you.
MelvinBot commented 1 year ago

📣 @goddev99! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
goddev99 commented 1 year ago

Contributor details Your Expensify account email: rodev097@gmail.com Upwork Profile Link: (https://www.upwork.com/freelancers/~019db16fae3db25fa9)

MelvinBot commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

isabelastisser commented 1 year ago

Not overdue, waiting for proposals!

soumyajit4419 commented 1 year ago

Proposal

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

Tooltip doesn't disappear while page is scrolling

What is the root cause of that problem?

I think the root cause for this is even on scroll the hovered component view is unable to get the mouse leave event on it. So the tooltip stills appers.

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

We can attach a wheel event listener to the tooltip component and call thehideTooltip when there is onWheel event. This will make the tooltip hidden. 

    componentDidMount() {
        window.addEventListener('wheel', () => {
            this.hideTooltip();
        });
    }

UI after applying the above code

https://user-images.githubusercontent.com/46092576/225435019-9d82e7bf-6a61-4104-adfe-0d9fb5d89e3c.mov

What alternative solutions did you explore? (Optional)

None

goddev99 commented 1 year ago

@soumyajit4419 if you can't find others from the parameter list of package, I think the method is good to solve the issue.

usenbekov commented 1 year ago

Contributor details Your Expensify account email: altosh.mail@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01d508c9ea2305f211

MelvinBot commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

usenbekov commented 1 year ago

Proposal

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

Tooltip doesn't disappear while page is scrolling

What is the root cause of that problem?

The Hoverable component's view is not receiving the mouse leave event on the scroll.

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

When the Hoverable component becomes "isHovered:true" we can start listening document wheel event. When we receive wheel event we can change the state to "isHovered:false" and call onHoverOut (and remove the wheel listener).

We can quickly test if the idea works (Hoverable):

componentDidMount

this.onWheel = () => {
    if (!this.state.isHovered) {
        return;
    }
    this.setState({isHovered: false}, this.props.onHoverOut);
};
document.addEventListener('wheel', this.onWheel);

componentWillUnmount document.removeEventListener('wheel', this.onWheel);

It might lead to an issue if the pointer is still in the hoverable area after the scroll: onMouseEnter won't be triggered until onMouseLeave. I guess we can solve it by listening to the "mouse move" and changing the state according to the local mouse position and bounds (See the alternative solutions section).

What alternative solutions did you explore? (Optional)

According to the bounds

This is a quick code to see if the idea works: Hoverable component

<View onWheel=...

onWheel(event) {
    const bounds = event.currentTarget.getBoundingClientRect();
    const localY = event.clientY - bounds.top;
    let hovered = this.isHovered;
    if (localY < 0 || localY > bounds.height) {
        hovered = false;
    } else {
        hovered = true;
    }

    if (this.isHovered !== hovered) {
        this.setIsHovered(hovered);
    }
}

And the result is:

https://user-images.githubusercontent.com/56184539/226363019-7e67b28c-d336-4266-aa53-08535c320f64.mp4

Inverted FlatList

Update: As I see this solution does not work for all situations, so I think for now the best solution would be "According to the bounds".

According to the further investigation: While the above solutions solve the issue but it is still not the best solution, as I believe they don't solve the main issue. The issue exists everywhere where used Hoverable in a scrollable list. For instance:

https://user-images.githubusercontent.com/56184539/226363050-827ed825-5458-4082-8df2-429a40260c9a.mp4

. As I mentioned as a root cause: The Hoverable component's view is not receiving onMouseLeave and onMouseEnter events on scrolling. But I realized that if the FlatList is inverted (<FlatList inverted...) then it starts to receive these events on the scroll. I searched for the cause in the React Native source code but could not find it. But it seems we can use this workaround because, in my opinion, it gives the best result. The basic idea is to use the inverted FlatList with the reversed data list.

And the result is:

https://user-images.githubusercontent.com/56184539/226364814-b113ca86-0c03-44eb-950a-b44667ff7790.mp4

According to bounds (2)

I extended the previous solution: A new mechanism that accepts Hoverable elements and provides 'mouse enter' and 'mouse leave' events for that elements.

On the document 'wheel' event, it gets the position and size of every element (Hoverable elements) and detects if the mouse pointer is inside or outside of the element, and emits the 'enter' or 'leave' event.

As in the video capture below, it solves the issue at all places (tooltip, highlighting...)

https://user-images.githubusercontent.com/56184539/226463504-c5d5aef4-a132-43be-89e0-6345dce9e564.mp4

If needed I can share the code (I wrote it just for validation so it is low quality and about 50 lines) I used getBoundingClientRect just for validation, I guess its better to use IntersectionObserver to prevent the reflow

Santhosh-Sellavel commented 1 year ago

Unassigning As I will not be get to this sooner, please reassign other C+ by applying external label again @yuwenmemon or @isabelastisser

0xmiros commented 1 year ago

I am happy to take this since I am already run out of issues for a few weeks.

Santhosh-Sellavel commented 1 year ago

@isabelastisser or @yuwenmemon Please assign @0xmiroslav so they can take over as C+

MelvinBot commented 1 year ago

📣 @0xmiroslav You have been assigned to this job by @yuwenmemon! 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 📖

0xmiros commented 1 year ago

Thanks for assigning me. Help Wanted label can be added back.

Santhosh-Sellavel commented 1 year ago

@0xmiroslav There are proposals above please review them

0xmiros commented 1 year ago

@goddev99 @usenbekov Please read through contributing guideline to be familiar with our process.

0xmiros commented 1 year ago

@soumyajit4419 thanks for your proposal but your root cause analysis isn't correct.

soumyajit4419 commented 1 year ago

@soumyajit4419 thanks for your proposal but your root cause analysis isn't correct.

@0xmiroslav I think the root cause for this is even on scroll the tooltip listeners or tooltip component is unable to know that the pointer is still on the element or not So it keeps on showing

usenbekov commented 1 year ago

Proposal

Updated

0xmiros commented 1 year ago

@soumyajit4419 your root cause is still not accurate. Also please follow updated proposal template like this, instead of adding segmented comments.

0xmiros commented 1 year ago

I guess we can solve it by listening to the "mouse move" and changing the state according to the local mouse position and bounds (this will solve the issue but needs some investigation to find a better implementation).

@usenbekov do you have solution in detail?

soumyajit4419 commented 1 year ago

Proposal

Updated

0xmiros commented 1 year ago

@soumyajit4419 I don't quite understand your solution.

Then In tooltip we can pass empty text based on isScrollling prop

<Tooltip text={props.isScrolling ? '' :  actorEmail}>

isScrolling should come from parent component. Which component are you referring to?

usenbekov commented 1 year ago

Proposal

Updated

usenbekov commented 1 year ago

I guess we can solve it by listening to the "mouse move" and changing the state according to the local mouse position and bounds (this will solve the issue but needs some investigation to find a better implementation).

@usenbekov do you have solution in detail?

Updated

soumyajit4419 commented 1 year ago

@soumyajit4419 I don't quite understand your solution.

Then In tooltip we can pass empty text based on isScrollling prop

<Tooltip text={props.isScrolling ? '' :  actorEmail}>

isScrolling should come from parent component. Which component are you referring to?

Here the parent component means IOUTransactionsModal But I think this solution will be somewhat specific to this modal only

To make it work for all the cases or generic we can add listeners to the tooltip component only which I had proposed in my alternate solution.

    componentDidMount() {
        window.addEventListener('wheel', () => {
            this.hideTooltip();
        });
    }
0xmiros commented 1 year ago

@soumyajit4419 We're looking for generic solution which applies to all scroll views across the app

https://user-images.githubusercontent.com/97473779/226390872-e9e36d43-c4dc-49dc-9eb6-67825607324e.mov

soumyajit4419 commented 1 year ago

Proposal

@0xmiroslav I have updated my proposal to an generic solution

Updated

usenbekov commented 1 year ago

Proposal

Updated

Added a new solution.

0xmiros commented 1 year ago

@soumyajit4419 it's not a good idea to hide tooltip on any wheel event. This is current version:

https://user-images.githubusercontent.com/97473779/226903688-73f32683-6c4e-4855-a4b2-5cb08b1a9e8d.mov

So if mouse pointer is still within area which should show tooltip, it doesn't disappear on wheel. However, another issue in your solution is that after scroll and back to original position, hidden tooltip doesn't show back again.

0xmiros commented 1 year ago

@usenbekov I reviewed your alternative solutions to fix this concern but your bounding approach is still not ideal. This can be considered as last option if no better solutions exist. Btw, can I check your branch for testing?

But I realized that if the FlatList is inverted (<FlatList inverted...) then it starts to receive these events on the scroll. I searched for the cause in the React Native source code but could not find it. But it seems we can use this workaround because, in my opinion, it gives the best result. The basic idea is to use the inverted FlatList with the reversed data list.

This is not recommended at all. This still doesn't work on Safari.

0xmiros commented 1 year ago

@isabelastisser can we add Help Wanted label to get more proposals?

MelvinBot commented 1 year ago

@yuwenmemon @isabelastisser @0xmiroslav this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

getusha commented 1 year ago

Proposal

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

Tooltip is not dismissed on scroll

What is the root cause of that problem?

When we scroll up and down fast we're not giving the time to hover the next element, there is currently no function to dismiss a visible tooltip on scroll rather than dismissing on mouse leave.

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

To hide the tooltip when the user scrolls, we can attach an event listener to the document object using the addEventListener method. This listener will call the hideTooltip function, which will take care of hiding the tooltip. By passing true as the third argument to addEventListener, we ensure that the event bubbles up to all scrollable containers on the page, this will insure the solution works on all scrollable containers.

document.addEventListener("scroll", this.hideOnScroll, true);
Screenshot 2023-03-23 at 11 06 18 AM

https://user-images.githubusercontent.com/75031127/227308538-0a461f28-92a2-49b8-8626-494aa5bcd99b.mov

I handled both performance and edge cases to make sure everything works smoothly and as expected resulting the hideTooltip being called only if the tooltip is shown and scrolled making sure the performance is not impacted. Here is the branch for testing purpose - https://github.com/Expensify/App/compare/main...getusha:App:hide-tooltip-onscroll

What alternative solutions did you explore? (Optional)

Like Slack and Discord set hover state of Hoverable false when the user scrolls

https://user-images.githubusercontent.com/75031127/227307613-ba0857fe-de89-41b0-8316-fcd6e9fe98f8.mov

isabelastisser commented 1 year ago

not overdue.

s77rt commented 1 year ago

This is a ~dupe~ similiar to https://github.com/Expensify/App/issues/14127

getusha commented 1 year ago

It was decided to do nothing because the issue linked was specific only to safari, and this is a different issue which is happening on all Browsers and Desktop.

So the only reason https://github.com/Expensify/App/issues/14127 closed was the issue happened only on Safari

s77rt commented 1 year ago

Thank you @getusha.

I think for performance reasons browsers do not fire the mouseleave event on scroll. This is behaviour can be seen across the app except on the report actions list. The mouseleave will still fire. I think this has to do with some css styles (perhaps transform styles) that forces the browser to fire the event. This looks related to the styles that we apply after we make the flatlist an inverted flatlist.

InvertedFlatList FlatList

What I'm trying to say here is maybe we should investigate the styles that forces the browser to fire the mouseleave event on scroll and apply them on places that are critical to have such behaviour.

usenbekov commented 1 year ago

@usenbekov I reviewed your alternative solutions to fix this concern but your bounding approach is still not ideal. This can be considered as last option if no better solutions exist. Btw, can I check your branch for testing?

Sure, sharing the Hoverable class (I wrote it just to validate the idea): https://gist.github.com/usenbekov/b6f9e2dbd349715df20f27a661af4a7d

The result can be seen here (last video): https://github.com/Expensify/App/issues/15757#issuecomment-1470922193

getusha commented 1 year ago

@s77rt we still want to hide the tooltip even on InvertedFlatList on scroll

https://user-images.githubusercontent.com/75031127/227756496-79600ce7-31ae-48ae-92d3-4726ef90a0ca.mov