[$1000] Jittery transition of tooltip when the inner content changes

[kavimuru]

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

---

Action Performed:

1. Navigate to any chat > Click on chat header
2. Click on the clipboard icon at the details page
3. Keep the cursor at clipboard and wait for the tooltip to change again to "Copy to clipboard"

Expected Result:

The tooltip transition is Jittery when the tooltip text changes.

Actual Result:

You shouldn't see any shaking/jitter in the transition.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• [ ] Android / native
• [ ] Android / Chrome
• [ ] iOS / native
• [ ] iOS / Safari
• [x] MacOS / Chrome / Safari
• [x] MacOS / Desktop

Version Number: 1.3.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: Any additional supporting documentation

https://user-images.githubusercontent.com/43996225/232654771-8182efd6-d634-40e9-b1b8-fc5f8a4d8efd.mp4

https://user-images.githubusercontent.com/43996225/232654786-3a83e4f3-19b9-4403-8df5-a8f864b32925.mov

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b7a7c818708f281d
• Upwork Job ID: 1651051677043404800
• Last Price Increase: 2023-04-26

[MelvinBot]

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

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [x] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [x] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[MelvinBot]

@michaelhaxhiu Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[MelvinBot]

@michaelhaxhiu 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[michaelhaxhiu]

Going external, this one is reproducible & valid!

[alitoshmatov]

Might be helpful I think issue is caused here: https://github.com/Expensify/App/blob/57388640eb97255c0089c4c316d13747b6a1dd34/src/components/Tooltip/TooltipRenderedOnPageBody.js#L101

When tooltip content changes we let the width be determined itself by setting width to undefined and immediately after that recalculate the width and provide it manually.

[bernhardoj]

Proposal

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

Tooltip flickers when the content of the tooltip changes.

What is the root cause of that problem?

If we slow down the video, we can see that when the tooltip content changes, the position of the tooltip is late to update.

Currently, we position the tooltip based on the component width and the tooltip width itself. When the tooltip content changes, the onLayout callback will be called to update the new tooltip width. https://github.com/Expensify/App/blob/6fb096948d862ee008445f809cff3ecd51b36f55/src/components/Tooltip/TooltipRenderedOnPageBody.js#L177-L178 https://github.com/Expensify/App/blob/6fb096948d862ee008445f809cff3ecd51b36f55/src/components/Tooltip/TooltipRenderedOnPageBody.js#L119-L124

As we can see, we will update the state inside onLayout callback, which could be late because the state is updated after the text content is updated.

content updated -> rendered -> onLayout -> width/height updated -> rendered

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

To calculate the size earlier, we can use useLayoutEffect (even the doc example uses tooltip as an example 🤣) which means we need to migrate the component to functional. Here is what we need to do:

1. Migrate TooltipRenderedOnPageBody to functional component
2. Add useLayoutEffect with props.text as the dependency array. Inside the hook: 2a. Get the width and height from getBoundingClientRect 2b. If both height & width is not 0, set the tooltip width and height

here is how it looks in functional component

const TooltipRenderedOnPageBody2 = (props) => {
    const [tooltipWidth, setTooltipWidth] = React.useState(0);
    const [tooltipHeight, setTooltipHeight] = React.useState(0);
    const [tooltipContentWidth , setTooltipContentWidth] = React.useState(undefined);

    const contentRef = React.useRef();
    const wrapper = React.useRef();

    const updateTooltipContentWidth = () => {
        setTooltipContentWidth(contentRef.current.offsetWidth);
    }

    React.useLayoutEffect(() => {
        const rect = wrapper.current.getBoundingClientRect();
        if (rect.width !== 0 && rect.height !== 0) {
            setTooltipWidth(rect.width);
            setTooltipHeight(rect.height);
        }
    }, [props.text]);

    React.useEffect(() => {
        // We need to re-calculate the tooltipContentWidth if it is greater than maxWidth.
        // So that the wrapperWidth still be updated again with correct value
        if (tooltipContentWidth === undefined || tooltipContentWidth > props.maxWidth) {
            updateTooltipContentWidth();
        }
    }, [tooltipContentWidth])

    React.useEffect(() => {
        // Reset the tooltip text width to 0 so that we can measure it again.
        // eslint-disable-next-line react/no-did-update-set-state
        setTooltipContentWidth(undefined);
    }, [props.text, props.renderTooltipContent]);

    const {
        animationStyle,
        tooltipWrapperStyle,
        tooltipTextStyle,
        pointerWrapperStyle,
        pointerStyle,
    } = getTooltipStyles(
        props.animation,
        props.windowWidth,
        props.xOffset,
        props.yOffset,
        props.wrapperWidth,
        props.wrapperHeight,
        props.maxWidth,
        tooltipWidth,
        tooltipHeight,
        tooltipContentWidth,
        props.shiftHorizontal,
        props.shiftVertical,
    );

    let content;
    if (props.renderTooltipContent) {
        content = (
            <View ref={contentRef}>
                {props.renderTooltipContent()}
            </View>
        );
    } else {
        content = (
            <Text numberOfLines={props.numberOfLines} style={tooltipTextStyle}>
                <Text style={tooltipTextStyle} ref={contentRef}>
                    {props.text}
                </Text>
            </Text>
        );
    }

    return ReactDOM.createPortal(
        <Animated.View
            ref={wrapper}
            onLayout={e => {
                setTooltipWidth(e.nativeEvent.layout.width);
                setTooltipHeight(e.nativeEvent.layout.height);
            }}
            style={[tooltipWrapperStyle, animationStyle]}
        >
            {content}
            <View style={pointerWrapperStyle}>
                <View style={pointerStyle} />
            </View>
        </Animated.View>,
        document.querySelector('body'),
    );
}

What alternative solutions did you explore? (Optional)

Previously the main proposal

Instead of relying on a fixed unit (tooltip width), we can use a percentage value to position the tooltip.

1. Remove tooltipWidth calculation from here https://github.com/Expensify/App/blob/6fb096948d862ee008445f809cff3ecd51b36f55/src/styles/getTooltipStyles.js#L187

2. Add translateX -50% here (translateX should be put before the scale) https://github.com/Expensify/App/blob/6fb096948d862ee008445f809cff3ecd51b36f55/src/styles/getTooltipStyles.js#L139-L146

3. Change the left to 50% and add transform: [{translateX: '-50%'}, {translateX: -horizontalShift}] here https://github.com/Expensify/App/blob/6fb096948d862ee008445f809cff3ecd51b36f55/src/styles/getTooltipStyles.js#L222

https://user-images.githubusercontent.com/50919443/234776646-2ef430b4-0b37-4998-a6c1-7b0e461c2401.mov

[mananjadhav]

I think @bernhardoj proposal looks good. We'll just have to check any regression absolute tooltips.

@amyevans all youts.

[bernhardoj]

Just tested that my proposal won't cover the case when the tooltip is at the edge of the screen. That is because we still use tooltipWidth to calculate horizontalShift

https://user-images.githubusercontent.com/50919443/234837345-570f6380-d70b-4935-9be7-c44fab59b48f.mov

[bernhardoj]

Updated my proposal to cover all cases with useLayoutEffect

[michaelhaxhiu]

Niiiice one @bernhardoj thanks for being proactive on that. Let's get final 👍 from @amyevans and I'll proceed with assigning you.

[amyevans]

Proposal looks great, assigning you @bernhardoj 🎉

We should be able to close out https://github.com/Expensify/App/issues/16211 as a byproduct of the bug fix as well 💪, 2 🐦 1 🥌

[MelvinBot]

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

[bernhardoj]

Will open the PR soon

[bernhardoj]

PR is ready.

[michaelhaxhiu]

Thanks for the fast work @bernhardoj, next step is to get this reviewed by @mananjadhav

[mananjadhav]

I've started with the review and going through the updated changes by @bernhardoj. Relatively larger than expected as we're changing the class component to functional component.

[melvin-bot[bot]]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.12-0 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

• https://github.com/Expensify/App/pull/18189

If no regressions arise, payment will be issued on 2023-05-16. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• [ ] External issue reporter
• [ ] Contributor that fixed the issue
• [ ] Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot[bot]]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [x] [@mananjadhav] The PR that introduced the bug has been identified. Link to the PR:
• [x] [@mananjadhav] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [x] [@mananjadhav] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [x] [@mananjadhav] Determine if we should create a regression test for this bug.
• [x] [@mananjadhav] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [x] [@michaelhaxhiu] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[mananjadhav]

@amyevans I couldn't pinpoint to the exact PR causing this. We've been using measureTooltip since a long time. I am suspecting this PR must've caused it https://github.com/Expensify/App/pull/8494 but I couldn't confirm by running the specific branch.

Also, considering this isn't going to change that often, and we've moved to a functional component, I don't think we need a regression test here. @amyevans @michaelhaxhiu if you still feel we should add one, here's a proposal.

Regression Test Proposal

1. Open any chat
2. Click on their profile image of any participant
3. Hover over the Copy icon next to the user email/phone number
4. Verify a tooltip shows with a text "Copy to clipboard"
5. Click the Copy icon
6. Verify the tooltip text changes to "Copied"
7. Wait for a while and verify the text changes back to "Copy to clipboard"
8. Verify that the tooltip doesn't reposition when the text reverts.
9. Repeat the steps 5-8 few times to verify step 9 for each iteration.

[bernhardoj]

I think this is not a regression as the initial implementation of the Tooltip has been using onLayout.

Btw, we have a regression (my bad) here https://github.com/Expensify/App/issues/18878#issuecomment-1550365511. I have found the issue and let me know if I should open the PR now.

cc: @mananjadhav @amyevans

[mananjadhav]

Yeah @bernhardoj please open the PR.

[bernhardoj]

PR is ready. https://github.com/Expensify/App/pull/19097

[michaelhaxhiu]

Please nudge me if I don't respond when it's time to process payment here (e.g. after the regression PR is merged and cleared)

[michaelhaxhiu]

Putting a date in the title that's more realistic, just so this doesn't appear behind.