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
2.99k stars 2.5k forks source link

[HOLD for payment 2023-06-01] [$4000] After returning to a setting from a profile, the tooltip loses position on the profile image when you hover the mouse for the first time. #15229

Closed kavimuru closed 11 months 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. Go to the setting -> Profile
  2. Go back to Setting and hover the mouse over the profile image. (hover the image quickly)

Expected Result:

The tooltip should not lose the position even if you hover the image quickly.

Actual Result:

If you quickly hover over the image after returning from the profile, the tooltip loses position.

Workaround:

unknown

Platforms:

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

Version Number: 1.2.73-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/219512628-3d4e28ad-dabe-46d0-9be3-64bd37826d8a.mp4

https://user-images.githubusercontent.com/43996225/219512642-13a4168c-f829-4e2e-935e-cf2fce52f0ef.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0175f90dca3be1912f
  • Upwork Job ID: 1628033343922810880
  • Last Price Increase: 2023-04-17
MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

MelvinBot commented 1 year ago

@CortneyOfstad Whoops! This issue is 2 days overdue. Let's get this updated quick!

CortneyOfstad commented 1 year ago

Was OoO — tackling this now!

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~0175f90dca3be1912f

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Triggered auto assignment to Contributor Plus for review of internal employee PR - @parasharrajat (Internal)

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

Ramkrishna1998 commented 1 year ago

The tooltip position is initialized only once during the first hover and the issue occurs because of the single initialization of the position of the tooltip so even if the target element position gets changed but tooltip position will not changed.

I have tried to implement the same scenario using the same component and I am attaching the before and after the fix

before: https://www.loom.com/share/f40dfbbdf9f94c07bee3e3c73285de34

after: https://www.loom.com/share/fa19295843f74158a8c4164546455202

Litande commented 1 year ago

Proposal

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

After returning to a setting from a profile, the tooltip loses position on the profile image when you hover the mouse for the first time.

What is the root cause of that problem?

There 2 events that are causing this issue.

  1. The back button on settings have also a tooltip which triggers active state on TooltipSense component.
  2. While the initial display in Tooltip is delayed (500 ms - atm) when TooltipSense is active it is happening imidiate, those the positions of tooltip is passed imidiatly and the tooltip is shown in the wrong place

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

I propose 2 things:

  1. Extend TooltipSense component to have method called deactivateImidiate which will reset active state on back button click.
  2. In Tooltip to move the delay parameter as depency on active state before the execution of getWrapperPosition which desides on the tooltip position this will effectly will take the correct position and conserve the TooltipSense behavior.

What alternative solutions did you explore? (Optional)

Moving the delay to Hoverable component but at this point i belive it is over complecated without breaking the TooltipSense behavior.

MelvinBot commented 1 year ago

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.

MelvinBot commented 1 year ago

📣 @Litande! 📣

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>
MelvinBot commented 1 year ago

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

Litande commented 1 year ago

Contributor details Your Expensify account email: mihaelchas@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/michaelchasovin

MelvinBot commented 1 year ago

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

Litande commented 1 year ago

Can someone help me out with Melvin here?) both are valid image image

Prince-Mendiratta commented 1 year ago

@Litande you need not specify the scheme in the upwork URL, just upwork.com/freelancers/michaelchasovin will do, I believe! 😄

MelvinBot commented 1 year ago

📣 @Prince-Mendiratta! 📣

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>
Litande commented 1 year ago

@Prince-Mendiratta thanks, didn't notice )

Litande commented 1 year ago

Contributor details Your Expensify account email: mihaelchas@gmail.com Upwork Profile Link: upwork.com/freelancers/michaelchasovin

MelvinBot commented 1 year ago

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

ayazhussain79 commented 1 year ago

I have reported this issue, should i share my email and upwork as well.

MelvinBot commented 1 year ago

📣 @ayazhussain79! 📣

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>
ayazhussain79 commented 1 year ago

Contributor details Expensify account email: hussainayaz79@gmail.com

Upwork Profile Link: https://www.upwork.com/fl/~018a62c1e70a3fab40

Ramkrishna1998 commented 1 year ago

Proposal

Problem. The position of the tooltip appears in the wrong location.

What is the root cause of that problem? The tooltip position is initialized only once during the first hover and the issue occurs because of the single initialization of the position of the tooltip so even if the target element position gets changed but tooltip position will not changed.

What changes do you think we should make in order to solve the problem? We have to re-render that part after UI will get into its original position

Contributor details Your Expensify account email: bhattramkrishna98@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0121390686e5dc8b39

MelvinBot commented 1 year ago

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

parasharrajat commented 1 year ago

All the proposals at their current stage do not have sufficient information to proceed. Please update your proposals to propose how will you solve this issue with technical details. The root cause should also be explained with technical details.

cc: @Ramkrishna1998 @Litande

MelvinBot commented 1 year ago

📣 @parasharrajat! 📣

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>
Ramkrishna1998 commented 1 year ago

@parasharrajat I have implemented the same scenario using the same component in my local machine using your code and component and I am attaching the before and after the fix videos. can we connect for further explanation?

before: https://www.loom.com/share/f40dfbbdf9f94c07bee3e3c73285de34

after: https://www.loom.com/share/fa19295843f74158a8c4164546455202

MelvinBot commented 1 year ago

📣 @Ramkrishna1998! 📣

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>
cubuspl42 commented 1 year ago

Proposal

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

Tooltips are just not synchronized with the "tooltiped" element after they are shown.

In this case, tooltip is first shown when the avatar is animating (although the animation is short and it's a temporary position), but stays in the incorrect position.

Another example where the problem is obvious is when the user scrolls the chat, an by chance has a cursor in the line of the avatar:

https://user-images.githubusercontent.com/2590174/225157987-55a12ab3-c7c0-41e8-8db5-01920baa213d.mov

I'm arguing that these are two aspects of the same problem.

What is the root cause of that problem?

The tooltip position is sampled and stored into a fixed CSS position, relative to the viewports's top left corner.

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

Conceptually, I'd like to propose just keeping the tooltip over the "tooltiped" element.

Technically, the solution can be expressed using a few helper callback-oriented operators:

    /**
     * Start syncing the tooltip's position with wrapper's position
     *
     * @param {function(): void} onStarted a callback called once bounds are established for the first ime
     * @returns {Handle} a handle to the syncing process
     */
    startSyncingPosition({
        onStarted,
    }) {
        /**
         * @param {{x: number, y: number, width: number, height: number}} position
         * @returns {void}
         */
        const updatePosition = ({
            x, y, width, height,
        }) => {
            this.setState({
                wrapperWidth: width,
                wrapperHeight: height,
                xOffset: x,
                yOffset: y,
            });
        };

        /**
         * Function that processes each new bounds, updating the tooltip position and ensuring that onStarted is called
         * when we got first bounds
         *
         * @param {{x: number, y: number, width: number, height: number}} position
         * @returns {void}
         */
        const processWrapperBounds = CallbackUtils.merge(
            updatePosition,
            CallbackUtils.calledOnce((_) => {
                onStarted();
            }),
        );

        /**
         * Handle called at each animation frame, starting the position getting async task
         *
         * @type {{
         *   call: function(): void,
         *   cancel: function(): void,
         * }}
         */
        const processAnimationFrameHandle = PromiseUtils.transformAsync(processWrapperBounds, this.getWrapperBounds);

        /**
         * Handle to the animation loop that drives the whole syncing process
         *
         * @type {Handle}
         */
        const animationHandle = AnimationUtils.requestAnimationFrames(processAnimationFrameHandle.call);

        return HandleUtils.mergeHandles(
            processAnimationFrameHandle,
            animationHandle,
        );
    }

I tested this code and didn't observe any performance issues, neither visually nor in the Performance Timeline. Aside that, it's worth noting that:

This solves the tooltip positioning in the general case.

The tooltip slightly readjusts while the animation is finishing:

https://user-images.githubusercontent.com/2590174/227300180-dee76064-b233-4399-b784-98b22a515733.mp4

It's so subtle that best observed in slow motion:

https://user-images.githubusercontent.com/2590174/227300434-038c741e-acee-40ff-af12-cdda77cb1f23.mp4

When scrolling the chat, it looks like this:

https://user-images.githubusercontent.com/2590174/227303773-ff751fd1-ebe8-43d6-8797-cb494b1bae12.mp4

To sum up, we have a solution that:

MelvinBot commented 1 year ago

📣 @cubuspl42! 📣

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>
cubuspl42 commented 1 year ago

Contributor details Your Expensify account email: cubuspl42.1@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~012b13cc109598f579

MelvinBot commented 1 year ago

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

parasharrajat commented 1 year ago

@Ramkrishna1998 Please read the contribution guidelines if you are new to the project. You will find them on the ReadMe page. You will also find a ton of useful info to propose a better solution.

Litande commented 1 year ago

@parasharrajat should i udate the proposal inline? or add new one ?

rushatgabhane commented 1 year ago

two c+ assigned to this issue? @parasharrajat

unassigned myself

Litande commented 1 year ago

Updated Proposal https://github.com/Expensify/App/issues/15229#issuecomment-1439511687

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

The position of the tooltip apear in the wrong location.

What is the root cause of that problem?

ModalStackNavigator is used in this instance and while in transition interactions on web aren't disabled, those hovering on it, while in transition causes a trigger to show Tooltip which uses Hoverable onMouseEnter events with X and Y params that aren't in their final position.

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

We can change the logic to such that the initialization of the tooltip is throttled while the cordinates are changing, with a small delay to wait for fixed cordinates.

I am proposing to update Hoverable, in there onMouseMove to add an event which will also apply IsHoverable(true) here https://github.com/Expensify/App/blob/9d04b46456253955c96981db9ef38245953baf18/src/components/Hoverable/index.js#L46-L55 we should update the logic contain a throttle mechanic for acutally settings is hoverable to true so it will happen upon stationary set of mouse after, for example for 50 miliseconds to show the tooltip, this

here is a video of the result https://www.loom.com/share/b4f1fb67e7204659a9f280b044100611 in here the throttle is 300 but i tested with 50 and looks also good.

What alternative solutions did you explore? (Optional)

Where trying check why overlay isn't used in web display for stack navigation component in android and ios there is such overlay (transparent) which blocks interactions, but i don't think filling this for web would be an easy task.

Litande commented 1 year ago

@parasharrajat updated https://github.com/Expensify/App/issues/15229#issuecomment-1440067841

Also do by some change know if there is anyone to help with joining slack send the email 3 already )

Thanks.

parasharrajat commented 1 year ago

@Litande

Also do by some chance know if there is anyone to help with joining slack send the email 3 already )

Sometimes, people are busy so they miss these. cc: @CortneyOfstad

@Litande Do you mind sharing more on how will you throttle & where? I don't think Hoverable is a good target for that. Hoverable is independent of navigation so it should remain as it is.

We should either create a wrapper component, HOC, or make use of props that makes sense to Hoverable and does not depend on any external factors.


Oh, so you are just going to throttle for a specific time irrespective of the navigation and all. I don't think that is a good approach. It will affect all the tooltips.

cubuspl42 commented 1 year ago

@parasharrajat Would you mind commenting on my proposal? Don't you think that changing the way tooltips are layed out could solve the problem at its core? We wouldn't have to apply the new strategy to all tooltips at once, to minimize a risk of breakage.

Litande commented 1 year ago

@parasharrajat ofcourse,

inside setIsHoverable we can add a mechanic similar to the folowing

if (this.timeout) {
    clearTimeout(this.timeout);
    this.timeout = null;
}
this.timeout = setTimeout(() => {
    //// SET TIMEOUT
}, 50)

ofcourse adding mousemove will trigger that event while you are moving you mouse above it also.

this in fact an effect delay of hoverable exection while the mouse is moving, this is ofcourse the general idean and not how a potential fix will look like.

Also regarding the location, I am not proposing fixing the navigation this what i wrote in the optional, where we are looking into implementing the logic of cover the element with other transparent element (which blocks integrations), and also this will be only partial fix to this location if any.

Hoverable is the target of the fix/imporvement, in such a way that while the user is moving his mouse the execution of hoverable will be delayed/throttled, and will actually take effect if the user stopped on the target element and those hoverable should apply.

As for implementation in web delay effect over tooltip is a common usage take for example the "member" label on your comment hear in github, it has also a minor delay, scroll over it nothing apear scroll into it and stop and a tooltip apears.

parasharrajat commented 1 year ago

@cubuspl42 Sorry missed it.

Embed the tooltip in the DOM tree close to the avatar, but with the size of zero, with appropriate Z-index Reactively update the fixed tooltip position during the animation

Out of these, 2 are very expensive. It might also show a moving tooltip which is not good.

For 1, I didn't understand. What do you mean by size of zero. I think initially tooltip was developed like this only but there were some overlapping issues. You have tested it before proposing.

parasharrajat commented 1 year ago

@Litande Thanks for the explanation. I think static delay for this problem is not a good idea. We don't need this delay everywhere.

As for implementation in web delay effect over tooltip is a common usage take for example the "member" label on your comment hear in github, it has also a minor delay, scroll over it nothing apear scroll into it and stop and a tooltip apears.

This is already present on our Tooltip. They are not immediately shown to the users.

Litande commented 1 year ago

@parasharrajat Hi i went over the layers of Hoverable and Tooltip and didn't find delay mechanics, also check the behavior and seems to only have small animation sequence, can you please check it out?

Regrading evertwgere, i am proposing for it be parametrized so it can be adjusted depending on the implementation, we can set paramater "delay" with default as undefined in which case there won't be any.

i have also went over where it is implemented, and it exists in 5 components mainly tooltips, and report user namings.

cubuspl42 commented 1 year ago

@parasharrajat After some experimenting, I realized that approach no. 1 doesn't integrate well with scrollables. So it's not an acceptable solution. Sorry if I made it sound confusing. I'm talking about an approach where we lay out an "anchor", having no size (size of zero; being empty) in the place we want the tooltip to point to. Then we position the tooltip absolutely to that anchor (with an extra transform, like [{translateX: '-50%'}, {translateY: '-50%'}] and a high z-order. It's a simple and neat solution, but such a tooltip cannot "escape" scrollables. Which makes this approach not applicable here.

I don't know why the moving tooltip is a bad thing... If what is under the tooltip moves, I'd assume that the tooltip should move too. For example, currently in Expensify when you scroll the chat while you hover an avatar, the glitch we're talking about (or a similar one) also appears. Of course, it's another topic wether the tooltip is not displayed too soon.

When you said "expensive", you mean expensive performance-wise, right?

bernhardoj commented 1 year ago

Proposal

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

The tooltip position is misplaced when hover the profile image while navigation is transitioning.

What is the root cause of that problem?

The current implementation of the tooltip uses a portal to put it at the body level. When we hover a component, it will calculate the current position of the component and the position of the tooltip based on it. However, because the transition is happening, the component position is not in a final position, yet we are calculating the tooltip position.

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

At first, I thought we can use InteractionManager.runAfterInteractions, but web does not have a full support for it yet. The alternative I could think of is

  1. to create a new function inside Navigation called runAfterNavigation which will execute the passed callback after the transitioning is done. We will wrap the showTooltip code with runAfterNavigation.

Here is the pseudocode of runAfterNavigation

runAfterNavigation (action):
    if isNavigating then:
        pendingAction push action
        return
    invoke action

If the transition is happening, we push the action to the pendingAction array, else we execute it immediately. We will execute all pendingAction later inside setIsNavigating when the value passed is false. The isNavigatingValue will be true when the transition starts and false when it ends.

https://github.com/Expensify/App/blob/b6636348ff9ed53a4fd65ba0dba1b2f73f3809a9/src/libs/Navigation/Navigation.js#L75-L77

  1. However, transition start event does not fire when we navigate forward, so we need to also call setIsNavigating inside navigate except when it's a drawer route, because drawer route never calls transition end event (only stack navigator calls the transition event ref).

https://github.com/Expensify/App/blob/b6636348ff9ed53a4fd65ba0dba1b2f73f3809a9/src/libs/Navigation/Navigation.js#L153-L186

But with this 2nd change, this will prevent multiple navigation when we press quickly over several components that call navigate. Currently, we only prevent it when navigating back only.

CortneyOfstad commented 1 year ago

@MariaHCD Any updates with this? Just checking to see if we're close to selecting a proposal. Thanks!

MariaHCD commented 1 year ago

Catching up here. @parasharrajat let me know what your thoughts are so far and how I can help!