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 · 57 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

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
@ishpaul777
Copy link
Contributor

@hoangzinh @mohit6789 Does the selected proposal fix #54925 by any chance?

@mohit6789
Copy link
Contributor

@ishpaul777 looks like related issue, but I will test and let you know.

Copy link

melvin-bot bot commented Jan 22, 2025

@hoangzinh @MonilBhavsar @bfitzexpensify 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!

@MonilBhavsar
Copy link
Contributor

Assigning @mohit6789 as their proposals fixes this issue. It would be great if it fixes other related issue

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

melvin-bot bot commented Jan 22, 2025

📣 @mohit6789 You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

@mohit6789
Copy link
Contributor

I will create PR before end of this week.

@mohit6789
Copy link
Contributor

mohit6789 commented Jan 24, 2025

@hoangzinh @MonilBhavsar Current solution by using BoundsObserver does not work for Android or IOS. https://www.npmjs.com/package/@react-ng/bounds-observer library is only compatible with the browser. So any solution using BoundsObserver does not work.

Currently we have two places where BaseEducationalTooltip is used during scrolling 1) SearchTypeMenuNarrow 2) OptionRowLHN. So to fix tooltip issue during scrolling, below is my suggested solution. Please let me know your thoughts or if you have any other better solution than this.

  1. We need to capture the scroll position here onScroll.
  2. Then pass scroll position into BaseEducationalTooltip.tsx by using props or by creating new Context.
  3. We need to update the logic into BaseEducationalTooltip.tsx so whenever scroll position is changed we update the UI.

Same applied for SearchTypeMenuNarrow component.

Also to performance I believe we should hide the tooltip during scrolling and show it once scrolling completed. Because Scrolling event fire very fast and it will made lots of calculation. So it is better to hide the tooltip during scrolling and show when scrolling completes.

Note: Currently we are using BoundsObserver in tooltip which is only used for web so we haven't faced any issue till date.

@mohit6789
Copy link
Contributor

@hoangzinh @MonilBhavsar any thoughts on above comment?

@melvin-bot melvin-bot bot added the Overdue label Jan 27, 2025
@hoangzinh
Copy link
Contributor

hi @mohit6789 sorry for the late response. I wasn't aware that BoundsObserver doesn't work on native too. The thing I don't like above solution is we have to capture position in the parent component, then pass it onto EducationalTooltip, if we extend to use it in other parent components we have to capture it too. Btw, can you share a recording video of that solution? I'm curious whether it would work.

@melvin-bot melvin-bot bot removed the Overdue label Jan 28, 2025
@mohit6789
Copy link
Contributor

@hoangzinh Okay I will make demo and send to you tomorrow.

@mohit6789
Copy link
Contributor

@hoangzinh do you have any other option apart from passing the data from the Parent component. I tried to find out but could not get success.

@mohit6789
Copy link
Contributor

I am working on creating demo but still I am facing some issues. I will create demo soon.

@hoangzinh
Copy link
Contributor

Thanks for the update @mohit6789. I tried some options but none of them work for me.

@ishpaul777
Copy link
Contributor

just a suggestion if that helps i see we have event emitter for scrolling, can we catch that and hide/reposition the tooltip using this

const onScroll = (event: NativeSyntheticEvent<NativeScrollEvent>) => {
onScrollProp(event);
if (!updateInProgress.current) {
updateInProgress.current = true;
DeviceEventEmitter.emit(CONST.EVENTS.SCROLLING, true);
}
};
/**
* Emits when the scrolling has ended.
*/
const onScrollEnd = () => {
DeviceEventEmitter.emit(CONST.EVENTS.SCROLLING, false);
updateInProgress.current = false;
};

@hoangzinh @mohit6789

@hoangzinh
Copy link
Contributor

Thank you @ishpaul777. Sounds like a great suggestion. What do you think? @mohit6789

@mohit6789
Copy link
Contributor

Indeed, it is great suggestion. Let me check this and I will get back to you. Thank you! @ishpaul777 @hoangzinh

@mohit6789
Copy link
Contributor

@hoangzinh here is the screen recording of the solution @ishpaul777 suggested. And this is #56209 draft PR.

IOS.Tooltip.mov

@hoangzinh
Copy link
Contributor

@MonilBhavsar @bfitzexpensify what do you think about the recording above ^? Because of performance, we need to hide the tooltip during scrolling and show it once scrolling is completed.

@mohit6789
Copy link
Contributor

mohit6789 commented Feb 4, 2025

@bfitzexpensify @MonilBhavsar any thoughts?

Copy link

melvin-bot bot commented Feb 5, 2025

@hoangzinh @mohit6789 @MonilBhavsar @bfitzexpensify this issue is now 4 weeks old, please consider:

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

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

Not overdue, awaiting @bfitzexpensify @MonilBhavsar share thoughts on #54924 (comment)

@mohit6789
Copy link
Contributor

@MonilBhavsar @bfitzexpensify can you please look into above comments and provide your thoughts?

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. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
Status: Second Cohort - HIGH
Development

No branches or pull requests

10 participants