[HOLD for payment 2023-06-30] [$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 contributors@expensify.com 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
• MacOS / Chrome / Safari
• 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

Recording.259.mp4

231447362-8279e374-dd33-46fc-86d7-43d2cd6a6895.mov

Expensify/Expensify Issue URL:
Issue reported by: @dukenv0307
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)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• 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
• 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.
• 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.
• 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:

App/src/components/Tooltip/TooltipRenderedOnPageBody.js

Line 101 in 5738864

this.setState({tooltipContentWidth: undefined}, this.updateTooltipContentWidth);

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.

App/src/components/Tooltip/TooltipRenderedOnPageBody.js

Lines 177 to 178 in 6fb0969

	<Animated.View
	onLayout={this.measureTooltip}

App/src/components/Tooltip/TooltipRenderedOnPageBody.js

Lines 119 to 124 in 6fb0969

	measureTooltip({nativeEvent}) {
	this.setState({
	tooltipWidth: nativeEvent.layout.width,
	tooltipHeight: nativeEvent.layout.height,
	});
	}

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
   

   App/src/styles/getTooltipStyles.js

   Line 187 in 6fb0969

   left: xOffset + ((componentWidth / 2) - (tooltipWidth / 2)) + horizontalShift + manualShiftHorizontal,

2. Add translateX -50% here (translateX should be put before the scale)
   

   App/src/styles/getTooltipStyles.js

   Lines 139 to 146 in 6fb0969

   	animationStyle: {
   	// remember Transform causes a new Local cordinate system
   	// https://drafts.csswg.org/css-transforms-1/#transform-rendering
   	// so Position fixed children will be relative to this new Local cordinate system
   	transform: [{
   	scale: currentSize,
   	}],
   	},

3. Change the left to 50% and add transform: [{translateX: '-50%'}, {translateX: -horizontalShift}] here
   

   App/src/styles/getTooltipStyles.js

   Line 222 in 6fb0969

   left: -horizontalShift + ((tooltipWidth / 2) - (POINTER_WIDTH / 2)),

Screen.Recording.2023-04-27.at.14.20.45.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

Screen.Recording.2023-04-27.at.18.32.43.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.

[melvin-bot]

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

[melvin-bot]

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

• Fix tooltip wrong height and recalculate tooltip size when locale changes #20340

If no regressions arise, payment will be issued on 2023-06-30. 🎊

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]

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:

• [@mananjadhav] The PR that introduced the bug has been identified. Link to the PR:
• [@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:
• [@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:
• [@mananjadhav] Determine if we should create a regression test for this bug.
• [@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.
• [@michaelhaxhiu] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/296910

[anmurali]

Paid @mananjadhav on New Dot

[melvin-bot]

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

[melvin-bot]

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

[michaelhaxhiu]

@dukenv0307 can you share your upwork profile, or apply to the upword job directly?
https://www.upwork.com/jobs/~0139362b8c0838a40f

[michaelhaxhiu]

I invite @bernhardoj to the job just now.

[dukenv0307]

@michaelhaxhiu My Upwork profile: https://www.upwork.com/freelancers/~01f5cbe690701118a2.
I've also applied to the job, thank you!

[mananjadhav]

Closest I could find the PR was #8494 but I am not confident, hence I am not posting it on the PR. This is because I am not sure if we had Copy to Clipboard and we also made multiple changes in the component while fixing #13146

I don't think this specific issue required a regression, but I think it makes sense to have a regression suite for Tooltip atleast? I have mentioned some cases here and we've got two additional cases here and here. @michaelhaxhiu Could you help with this? I am not sure if adding regression suite would be considered and what would be the steps here?

[michaelhaxhiu]

Re: the regression, I already made the regression test and linked it (yesterday). All set there.

[mananjadhav]

Thanks @michaelhaxhiu. Is everyone paid out here? If yes, then we are good to close here?

[michaelhaxhiu]

@dukenv0307 lmk when you accept the job offer. Just need to pay you & this can be closed.

[dukenv0307]

@michaelhaxhiu I accepted, thank you!

[melvin-bot]

@mananjadhav, @amyevans, @michaelhaxhiu, @bernhardoj Eep! 4 days overdue now. Issues have feelings too...

[mananjadhav]

@michaelhaxhiu are we done with the payout? Can we close this one out?

[michaelhaxhiu]

paid now