Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$250] Submit expenses and chat with approver tooltip appears at the top of the page sometimes #54924

Open
1 of 8 tasks
m-natarajan opened this issue Jan 8, 2025 · 91 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Jan 8, 2025

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: v9.0.81-6
Reproducible in staging?: yes
Reproducible in production?: yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
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
Expensify/Expensify Issue URL:
Issue reported by: @flaviadefaria
Slack conversation (hyperlinked to channel name): migrate

Action Performed:

  1. Go to staging.new.expensify.com and create an account
  2. Open the workspace chat and submit an expense
  3. Observe the tooltip for "submit expense and chat with approvers here"

Expected Result:

Should appear with in the report active window

Actual Result:

sometimes displays at the top of the page. Also when I scroll down the chats on the LHP it stays static rather than following the workspace that it is pointing to.

Workaround:

unknown

Platforms:

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

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Snip - New Expensify - Google Chrome (3)

2025-01-06_21-26-00.1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021878933333602082321
  • Upwork Job ID: 1878933333602082321
  • Last Price Increase: 2025-01-20
Issue OwnerCurrent Issue Owner: @
Issue OwnerCurrent Issue Owner: @hoangzinh
@m-natarajan m-natarajan added Daily KSv2 Needs Reproduction Reproducible steps needed Bug Something is broken. Auto assigns a BugZero manager. labels Jan 8, 2025
Copy link

melvin-bot bot commented Jan 8, 2025

Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@MelvinBot
Copy link

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

@m-natarajan m-natarajan removed the Needs Reproduction Reproducible steps needed label Jan 8, 2025
@hoangzinh
Copy link
Contributor

I'm able to reproduce this issue with either new accounts

  1. Go to staging.new.expensify.com
  2. Sign in with newly account
  3. Create a workspace
  4. Submit an expense on the workspace chat
  5. On the LHN, observe the product training tooltip for "submit expense and chat with approvers here"

or set Onyx.set('nvp_dismissedProductTraining', {})

  1. Go to staging.new.expensify.com
  2. Sign in with any account
  3. Run Onyx.set('nvp_dismissedProductTraining', {}) to reset product training steps
  4. Create a workspace
  5. Submit an expense on the workspace chat
  6. On the LHN, observe the product training tooltip for "submit expense and chat with approvers here"
Screen.Recording.2025-01-08.at.17.42.22.mov

@hoangzinh
Copy link
Contributor

hoangzinh commented Jan 8, 2025

Please assign me as a C+ for this issue. Thank you.

@M00rish
Copy link
Contributor

M00rish commented Jan 8, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-01-17 14:57:08 UTC.

Proposal

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

Submit expenses and chat with approver tooltip appears at the top of the page sometimes

What is the root cause of that problem?

there's no mechanism set to handle scrolling events

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

in educationalToolTip

a more straight forward appraoch, is chekking if element is within view port, this is flexible way to deal with all pages

    const targetRef = useRef<any | null>(null);
    const wasTooltipActive = useRef(false);
    const checkVisibilityIntervalRef = useRef<number | null>(null);

    const isElementInViewport = useCallback((element: HTMLElement) => {
        const rect = element.getBoundingClientRect();
        return (
            rect.top >= 0 &&
            rect.left >= 0 &&
            rect.bottom <= (window.innerHeight || document.documentElement.clientHeight) &&
            rect.right <= (window.innerWidth || document.documentElement.clientWidth)
        );
    }, []);

    const checkVisibility = useCallback(() => {
        if (!targetRef.current || !(targetRef.current instanceof HTMLElement)) {
            return;
        }

        const isVisible = isElementInViewport(targetRef.current);
        
        if (!isVisible) {
            wasTooltipActive.current = true;
            hideTooltipRef.current?.();
        } else if (wasTooltipActive.current) {
            showTooltipRef.current?.();
            wasTooltipActive.current = false;
        }
    }, [isElementInViewport]);

    useEffect(() => {
        if (!shouldRender) {
            return;
        }

        checkVisibility();

        checkVisibilityIntervalRef.current = window.setInterval(checkVisibility, 100);

        const handleScrollOrResize = () => {
            checkVisibility();
        };

        window.addEventListener('scroll', handleScrollOrResize, true);
        window.addEventListener('resize', handleScrollOrResize);

        return () => {
            if (checkVisibilityIntervalRef.current) {
                clearInterval(checkVisibilityIntervalRef.current);
            }
            window.removeEventListener('scroll', handleScrollOrResize, true);
            window.removeEventListener('resize', handleScrollOrResize);
        };
    }, [shouldRender, checkVisibility]);

    const navigator = useContext(NavigationContext);

    useEffect(() => {
        return () => {
            hideTooltipRef.current?.();
            if (checkVisibilityIntervalRef.current) {
                clearInterval(checkVisibilityIntervalRef.current);
            }
        };
    }, []);

the above is for web only to include native, it could be something similar to this :

const targetRef = useRef<any>(null);
  const wasTooltipActive = useRef(false);
  const checkVisibilityIntervalRef = useRef<number | null>(null);

  const isElementInViewport = useCallback((measurements: { 
    x: number; 
    y: number; 
    width: number; 
    height: number; 
  }) => {
    if (Platform.OS === 'web') {
      // Web implementation
      return (
        measurements.y >= 0 &&
        measurements.x >= 0 &&
        measurements.y + measurements.height <= (window.innerHeight || document.documentElement.clientHeight) &&
        measurements.x + measurements.width <= (window.innerWidth || document.documentElement.clientWidth)
      );
    } else {
      // React Native implementation
      const windowDimensions = Dimensions.get('window');
      return (
        measurements.y >= 0 &&
        measurements.x >= 0 &&
        measurements.y + measurements.height <= windowDimensions.height &&
        measurements.x + measurements.width <= windowDimensions.width
      );
    }
  }, []);

  const getMeasurements = useCallback(() => {
    return new Promise<{ x: number; y: number; width: number; height: number } | null>((resolve) => {
      if (Platform.OS === 'web') {
        if (!targetRef.current || !(targetRef.current instanceof HTMLElement)) {
          resolve(null);
          return;
        }
        const rect = targetRef.current.getBoundingClientRect();
        resolve({
          x: rect.left,
          y: rect.top,
          width: rect.width,
          height: rect.height,
        });
      } else {
        if (!targetRef.current) {
          resolve(null);
          return;
        }
        targetRef.current.measure((_x: number, _y: number, width: number, height: number, pageX: number, pageY: number) => {
          resolve({
            x: pageX,
            y: pageY,
            width,
            height,
          });
        });
      }
    });
  }, []);

  const checkVisibility = useCallback(async () => {
    const measurements = await getMeasurements();
    if (!measurements) return;

    const isVisible = isElementInViewport(measurements);
    
    if (!isVisible) {
      wasTooltipActive.current = true;
      hideTooltipRef.current?.();
    } else if (wasTooltipActive.current) {
      showTooltipRef.current?.();
      wasTooltipActive.current = false;
    }
  }, [isElementInViewport, getMeasurements]);

  useEffect(() => {
    if (!shouldRender) {
      return;
    }

    checkVisibility();

    if (Platform.OS === 'web') {
      // Web-specific event listeners
      checkVisibilityIntervalRef.current = window.setInterval(checkVisibility, 100);
      const handleScrollOrResize = () => {
        checkVisibility();
      };
      window.addEventListener('scroll', handleScrollOrResize, true);
      window.addEventListener('resize', handleScrollOrResize);

      return () => {
        if (checkVisibilityIntervalRef.current) {
          clearInterval(checkVisibilityIntervalRef.current);
        }
        window.removeEventListener('scroll', handleScrollOrResize, true);
        window.removeEventListener('resize', handleScrollOrResize);
      };
    } else {
      // React Native-specific listeners
      checkVisibilityIntervalRef.current = setInterval(checkVisibility, 100);
      const dimensionsSubscription = Dimensions.addEventListener('change', checkVisibility);

      return () => {
        if (checkVisibilityIntervalRef.current) {
          clearInterval(checkVisibilityIntervalRef.current);
        }
        dimensionsSubscription.remove();
      };
    }
  }, [shouldRender, checkVisibility]);

  const navigator = useContext(NavigationContext);

  useEffect(() => {
    return () => {
      hideTooltipRef.current?.();
      if (checkVisibilityIntervalRef.current) {
        clearInterval(checkVisibilityIntervalRef.current);
      }
    };
  }, []);

....
return (
        <GenericTooltip
            shouldForceAnimate
            shouldRender={shouldRender}
            {...props}
        >
            {({showTooltip, hideTooltip, updateTargetBounds}) => {
                hideTooltipRef.current = hideTooltip;
                showTooltipRef.current = showTooltip;
                return (
                    <BoundsObserver
                        enabled={shouldRender}
                        onBoundsChange={(bounds) => {
                            updateTargetBounds(bounds);
                        }}
                    >
                        {React.cloneElement(children as React.ReactElement, {   
                            onLayout: (e: LayoutChangeEventWithTarget) => {
                                if (!shouldMeasure) {
                                    setShouldMeasure(true);
                                }
                                const target = e.target || e.nativeEvent.target;
                                targetRef.current = target;

                                show.current = () => {
                                    wasTooltipActive.current = true;
                                    measureTooltipCoordinate(target, updateTargetBounds, showTooltip);
                                };

                                if (target instanceof HTMLElement) {
                                    checkVisibility();
                                }
                            },
                        })}
                    </BoundsObserver>
                );
            }}
        </GenericTooltip>
    );

}
                                }

could be polished in PR phase

bandicam.2025-01-17.14-48-36-976.mp4

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

UI issue

What alternative solutions did you explore? (Optional)

@ishpaul777
Copy link
Contributor

ishpaul777 commented Jan 9, 2025

@hoangzinh feel free to take over, just coming here from here to say that similar thing happen to filter button tooltip when we scroll the expenses on small screens, Make sure we fix it and other tooltips where this potentially happens

@ishpaul777
Copy link
Contributor

also the expected behaviour is not show tooltip it if element it is pointing to is not in the visible view and ideally we keep the tooltip on it while scrolling pointing to element. Or if that's too hard, have it disappear while scrolling and re-appear once they stop (if the chat is still visible) - https://expensify.slack.com/archives/C07NMDKEFMH/p1736277111466739?thread_ts=1736274954.289009&cid=C07NMDKEFMH

@flaviadefaria
Copy link
Contributor

Please assign me as a C+ for this issue. Thank you.

Done!

@M00rish
Copy link
Contributor

M00rish commented Jan 9, 2025

@ishpaul777 @hoangzinh ,
is it not more intuitive to make it appear when user is not on the active worksapce ? and when user clicks the workspace it appears on the composer ?

I guess it's supposed to be educational meaning it leads the user through the app ... I think:

bandicam.2025-01-09.16-56-43-609.mp4
just trying to check the expected outcome here

@ishpaul777
Copy link
Contributor

ishpaul777 commented Jan 10, 2025

is it not more intuitive to make it appear when user is not on the active worksapce

@M00rish i am sorry but i dont follow what you mean by this tooltip is only shown to active (default) workspace chat of user, what do you propose?

@linhvovan29546
Copy link
Contributor

linhvovan29546 commented Jan 12, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-01-12 12:42:58 UTC.

🚨 Edited by proposal-police: This proposal was edited at 2025-01-12 12:42:58 UTC.

Edited by proposal-police: This proposal was edited at 2025-01-12 12:42:58 UTC.

Proposal

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

Submit expenses and chat with approver tooltip appears at the top of the page sometimes

What is the root cause of that problem?

We are not updating the position of the tooltip when the layout of the content changes(Because the onLayout of cloneElement only fired once).

return React.cloneElement(children as React.ReactElement, {
onLayout: (e: LayoutChangeEventWithTarget) => {
if (!shouldMeasure) {
setShouldMeasure(true);
}
// e.target is specific to native, use e.nativeEvent.target on web instead
const target = e.target || e.nativeEvent.target;
show.current = () => measureTooltipCoordinate(target, updateTargetBounds, showTooltip);
},

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

To address this issue, we can use BoundsObserver to wrap the children of BaseEducationalTooltip, similar to the existing implementation here
Exemple:

       const elementRef = useRef();
        const [isVisibleElement, setIsVisibleElement] = useState(false);

        const getBounds = (bounds: DOMRect): LayoutRectangle => {
        const targetElement = elementRef.current?._childNode;
        const elementAtPoint = document.elementFromPoint(bounds.x, bounds.y + bounds.height / 2); //Consider increase x by + padding
        if (elementAtPoint && 'contains' in elementAtPoint && targetElement && 'contains' in targetElement) {

            if (shouldCheckBottomVisible) {// This flag is true in LHN
                const viewportHeight = window.innerHeight; // The height of the visible viewport
                const isBottomVisible = bounds.bottom + bounds.height <= viewportHeight; //Consider decrease viewportHeight by - padding
                const isInViewport = isBottomVisible;
                if (!isInViewport) {
                    setIsVisibleElement(false);
                    return bounds
                }
            }

            const isElementVisible =
                elementAtPoint instanceof HTMLElement &&
                (targetElement?.contains(elementAtPoint) || elementAtPoint?.contains(targetElement)); // We can update condition here, if we need other expected
            setIsVisibleElement(isElementVisible)
        }

        return bounds;
    };
...
<BoundsObserver
                        enabled={shouldRender}
                        onBoundsChange={(bounds) => {
                            updateTargetBounds(getBounds(bounds));
                        }}
                        ref={elementRef}
                    >
                        {React.cloneElement(children as React.ReactElement, {
                            onLayout: (e: LayoutChangeEventWithTarget) => {
                                if (!shouldMeasure) {
                                    setShouldMeasure(true);
                                }
                                // e.target is specific to native, use e.nativeEvent.target on web instead
                                const target = e.target || e.nativeEvent.target;
                                show.current = () => measureTooltipCoordinate(target, updateTargetBounds, showTooltip);
                            },
                        })}
                    </BoundsObserver>

We need isVisibleElement because we want to hide the tooltip when the content is not within the visible viewport.
Then we need to pass the isVisibleElement property to GenericTooltip.

            shouldRender={shouldRender && isVisibleElement}

For the detailed code, we can discuss it later in PR.

POC
Before After
Screen.Recording.2025-01-12.at.7.34.43.PM.mov
Screen.Recording.2025-01-12.at.7.19.50.PM.mov

Test branch

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

No, UI bug

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

Copy link

melvin-bot bot commented Jan 13, 2025

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

@bfitzexpensify bfitzexpensify added the External Added to denote the issue can be worked on by a contributor label Jan 13, 2025
@melvin-bot melvin-bot bot changed the title Submit expenses and chat with approver tooltip appears at the top of the page sometimes [$250] Submit expenses and chat with approver tooltip appears at the top of the page sometimes Jan 13, 2025
Copy link

melvin-bot bot commented Jan 13, 2025

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 13, 2025
Copy link

melvin-bot bot commented Jan 13, 2025

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

@bfitzexpensify bfitzexpensify moved this to First Cohort - MEDIUM or LOW in [#whatsnext] #migrate Jan 13, 2025
@flaviadefaria flaviadefaria moved this from First Cohort - MEDIUM or LOW to First Cohort - HIGH in [#whatsnext] #migrate Jan 13, 2025
@hoangzinh
Copy link
Contributor

hoangzinh commented Feb 12, 2025

Yes, I tried to not use GenericTooltip here but use native wrapper View with relative position and absolute position for tooltip. Something like this

<View style={styles.pRelative}>
        <View style={[styles.pAbsolute, {top: 0, left: 100}]}>Tooltip</View>
        {children}
    </View>

@mohit6789
Copy link
Contributor

mohit6789 commented Feb 12, 2025

@hoangzinh May be we can do this way (Still I am not sure, if this will work in Mobile or not), but this required major refactor that is what I am trying to say. So my first question is do we really want to change the architecture of the EducationalTooltip? @MonilBhavsar

We already spend quite huge amount of time to check few approaches. Now we are moving into completely different approach.

@hoangzinh
Copy link
Contributor

hoangzinh commented Feb 13, 2025

Hi @ishpaul777 how important is it for the migration project that you mentioned here? I mean how urgent is it?

@ishpaul777
Copy link
Contributor

@flaviadefaria would be the best person to judge the urgency but it seems like we are planning to add more educational tooltips so a general solution to this is pretty important

@hoangzinh
Copy link
Contributor

Thanks @ishpaul777. Hmm, It seems we still have enough time to look for a solution that makes the product training tooltips to follow the element that it is pointing to when scrolling. What do you think @MonilBhavsar? Or are you good with current suggestion from @mohit6789?

@parasharrajat
Copy link
Member

parasharrajat commented Feb 14, 2025

I am fine with the #54924 (comment) suggestion But it should only impact the tooltips where target is moving not the ones that are static (e.g. FAB tooltips).

@MonilBhavsar
Copy link
Contributor

Thanks @ishpaul777. Hmm, It seems we still have enough time to look for a solution that makes the product training tooltips to follow the element that it is pointing to when scrolling. What do you think @MonilBhavsar? Or are you good with current suggestion from @mohit6789?

I am concerned about the performance issues if we decide to display the tooltip while scrolling if it tries to re render often.

@parasharrajat
Copy link
Member

Just to clarify I was talking about this #54924 (comment) suggestion.

@hoangzinh
Copy link
Contributor

@MonilBhavsar Yep, with the current Educational tooltip implementation, it's hard (and the tooltip will blink or laggy) if we want to display the tooltip while scrolling.

@shawnborton
Copy link
Contributor

Yeah, that makes sense. I think it couldn't hurt to try it, but I am cool with the solution that hides it while scrolling and then makes it reappear - that feels pretty natural to me.

@MonilBhavsar
Copy link
Contributor

Yes, asked gpt about this and it also suggests hidding while scrolling. So I think we could go with @mohit6789's proposal

@MonilBhavsar
Copy link
Contributor

@mohit6789 feel free to open a PR

@parasharrajat
Copy link
Member

@mohit6789 @hoangzinh Do test your solution againstt all Tooltips in the App. Thank you.

@melvin-bot melvin-bot bot added the Overdue label Feb 18, 2025
Copy link

melvin-bot bot commented Feb 19, 2025

@hoangzinh, @mohit6789, @MonilBhavsar, @bfitzexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@mohit6789
Copy link
Contributor

Working on this

@hoangzinh
Copy link
Contributor

Cool. Let me know when it's ready to review

@melvin-bot melvin-bot bot removed the Overdue label Feb 20, 2025
Copy link

melvin-bot bot commented Feb 25, 2025

@hoangzinh Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot melvin-bot bot added the Overdue label Feb 25, 2025
@hoangzinh
Copy link
Contributor

@mohit6789 Do you have any timeframe for when the PR will be ready for review?

@melvin-bot melvin-bot bot removed the Overdue label Feb 25, 2025
@mohit6789
Copy link
Contributor

IOS and android builds are not working, I am working to fix this. I will create PR ASAP

@parasharrajat
Copy link
Member

parasharrajat commented Feb 26, 2025

@mohit6789 We will have to reassign this issue if PR is not created in a few hours. It's been 2 weeks. cc: @hoangzinh

We are dependent on this PR.

@mohit6789
Copy link
Contributor

mohit6789 commented Feb 26, 2025

@parasharrajat I can create PR by tomorrow. Changes are almost done. Please allow one more day.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Feb 26, 2025
@mohit6789
Copy link
Contributor

@hoangzinh @parasharrajat PR is ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Status: Second Cohort - HIGH
Development

No branches or pull requests