[HOLD for payment 2023-06-01] [Regression Hold] The tooltip is not aligned with the arrow if the arrow position is at right edge of the tooltip.

[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. Open Search page, try to hover the three dot of any chat group that has many member.
2. Notice that there's a space between arrow and the tooltip.

Expected Result:

Tooltip should be positioned above the arrow

Actual Result:

The tooltip is not aligned with the arrow

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: v1.3.13-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

Screen.Recording.2023-05-12.at.4.41.53.AM.mov

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

View all open jobs on GitHub

[melvin-bot]

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

[melvin-bot]

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

[allroundexperts]

Proposal

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

The tooltip is not aligned with the arrow correctly

What is the root cause of that problem?

The root cause of the issue is that we're using an incorrect ref to figure out the the tooltip content width. We're using the ref of the nested Text here instead of the wrapping Text. This is not an issue normally but when the text wraps to the next line with some space at the right, using the inner ref causes the space at the end of Text to be not accounted.

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

We just need to use the ref of the wrapping Text here.

content = (
            <Text
                ref={contentRef}
                numberOfLines={props.numberOfLines}
                style={tooltipTextStyle}
            >
                <Text
                    style={tooltipTextStyle}
                >
                    {props.text}
                </Text>
            </Text>
        );

Result

Screen.Recording.2023-05-13.at.2.51.33.PM.mov

What alternative solutions did you explore? (Optional)

I'm not sure why we have a wrapping Text here anyway. We can remove the wrapping text and it will work fine as well.

[hoangzinh]

Proposal

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

The tooltip is not aligned with the arrow if the arrow position is at right edge of the tooltip.

What is the root cause of that problem?

We're using useLayoutEffect to calculate tooltipWidth and tooltipHeight since this PR #18189. It improves a lot UX of showing position of tooltip. Even though, it's almost same with the example in React document useLayoutEffect. But we're using Animated.View as a wrapper component. So calling wrapper.current.getBoundingClientRect to get the size of element is not totally correct. Because useLayoutEffect is triggered before the layout and animation are finished, so the size of the element may not be updated yet.

Take a look at the console.log here

Because tooltipWidth is not correct, it leads to calculate horizontalShift is not correct too.

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

Given the fact that, by using useLayoutEffect gives us better UX for tooltip in most of cases. I think we need to keep to use it. But we need to also use onLayout callback of Animated.View same as previous, as a 2nd guard to update tooltip width/height to more correct values after layout changed (We do further by checking if they're same value, we can skip updates)

Result

Screen.Recording.2023-05-13.at.16.42.00.mov

Screen.Recording.2023-05-13.at.16.43.36.mov

What alternative solutions did you explore? (Optional)

• We can use the measure() method as a replacement for getBoundingClientRect() to get the current size and position of the animated view. But the UX result is not great as using getBoundingClientRect() as a 1st update
• Or we can use the measure() method as a 2nd update same as use onLayout callback above. But I prefer using onLayout callback for officially and supported by RN.

[strepanier03]

I am unable to recreate this any longer, I created a group with at least 8 individuals as in the Slack thread and still no luck.

@fedirjh @kavimuru, are you both still able to recreate this? I wonder if we fixed this elsewhere?

[fedirjh]

@strepanier03 Your screenshot is showing the bug , the tooltip text should be aligned with tooltip arrow .

[bernhardoj]

As this is a regression from my PR, I think I should take care of the issue? If yes, I will open the PR to fix the regression.

[fedirjh]

@bernhardoj I think yes since the PR #18189 is still under regression period.

[strepanier03]

@fedirjh -- aaaahaa, I see. I thought that the below was the bug, where it's really not lined up.

[strepanier03]

I need to figure out the proper labels to apply here so that this is treated properly since it's a regression @bernhardoj is going to take care of. Will do that as soon as I confirm.

[strepanier03]

@bernhardoj - I just want to make sure I am 100% confident I understand before taking any action.

This bug in this GH was introduced when your PR here was deployed to staging, is that correct?

[bernhardoj]

Yes, that is correct @strepanier03

[bernhardoj]

I have open a PR to handle this regression #19097.

[strepanier03]

Weird assignment behavior today, sorry about that everyone.

[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.17-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Remove multiple sources of tooltip size #19097

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

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 - @fedirjh - $250
• Contributor that fixed the issue - @bernhardoj - not eligible for regression fix payment, was done within 7 days of deploy to prod.
• Contributor+ that helped on the issue and/or PR - @mananjadhav - not eligible for regression fix payment, was done within 7 days of deploy to prod.

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.
• [@strepanier03] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[strepanier03]

@fedirjh - I've hired you to a reporting job for this, please accept when you can and I'll pay out.

[fedirjh]

@strepanier03 Thank you. Accepted!

[strepanier03]

All wrapped up with payment. I don't believe we need to do the checklist either because it's a regression from another GH. I'm going to close out.