[$1000] Web - Tooltip position keeps on shifting for long user names on request money details page

[kbecciv]

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 the app
2. Click on plus and click on request money
3. Enter any amount and continue
4. Select user with long user name
5. On details page, hover on username, observe current tooltip position
6. Change amount of change description text and again hover on username, observe that for same user, tooltip position is now changed

Expected Result:

App should display tooltip constantly at center of username on request money details page as it does on search page

Actual Result:

Tooltip position changes when we change amount or description and is not at center of username on request money details page

Workaround:

Unknown

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.46-0
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

tooltip.position.changes.on.change.in.amount.description.1.mp4

Recording.3954.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690428525912479

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0133c32537626b0505
• Upwork Job ID: 1685946951677566976
• Last Price Increase: 2023-07-31
• Automatic offers:
• s77rt | Reviewer | 25876640
• bernhardoj | Contributor | 25876641
• dhanashree-sawant | Reporter | 25876644

[melvin-bot]

Triggered auto assignment to @laurenreidexpensify (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

[alitoshmatov]

The issue is caused by getTooltipShiftX not working as expected

App/src/components/DisplayNames/index.js

Line 49 in ddc19e0

getTooltipShiftX(index) {

Here you can see the inconsistent outputs it is generating

[spcheema]

Proposed solution

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

Web - Tooltip position keeps on shifting for long user names on request money details page

What is the root cause of that problem?

There are two root causes of this issue.

The first one is that after increasing the amount, let say $100 -> $1000, the display name container size shrunk which causes the tooltip to move. I believe this is normal behavior and there is nothing we can do.

The second issue is that onLayout callback returns an incorrect element left position. Due to that getTooltipShiftX(index) returns 0 which pushes the tooltip center to the right side., similar to this issue #21100

In this particular case, I noticed that the child element left offset is 1151 and the onLayout return wrapping element left offset is 1440.786376953125 which is too higher.

For the same selected users, onLayout returned different values because the element is not visible yet due to the animation transistion.

Output 1

Output 2

After refreshing reloading the browser on cash parent and child node left offset is consistent

#23784 (comment)

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

Update getTooltipShiftX function should calculate horizontal shift based on the parent wrapper and fallback to the onLayout if the parent element is not set.

App/src/components/DisplayNames/index.js

Line 54 in ddc19e0

const {width: containerWidth, left: containerLeft} = this.containerLayout;

Updated code here:

    let containerWidth;
    let containerLeft;

  
    if (this.childRefs[index].offsetParent) {
        const parent = this.childRefs[index].offsetParent;
        ({ width: containerWidth, left: containerLeft } = parent.getBoundingClientRect());
    } else {
        ({ width: containerWidth, left: containerLeft } = this.containerLayout);
    }

  or 
  
    if (this.containerRef) {
       ({ width: containerWidth, left: containerLeft } = this.containerRef.getBoundingClientRect();
    } else {
        ({ width: containerWidth, left: containerLeft } = this.containerLayout);
    }

What alternative solutions did you explore? (Optional)

N/A

[bernhardoj]

Proposal

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

The tooltip position in the money request confirmation page is placed at the very end of the text and it changes after opening the edit amount page. So, the problem is not about editing the amount, simply opening the edit amount or description page is enough.

What is the root cause of that problem?

In DisplayNames, we have getTooltipShiftX which is useful to shift the tooltip to the center of the text in case the text itself is really long.

App/src/components/DisplayNames/index.js

Lines 54 to 57 in eb02006

	const {width: containerWidth, left: containerLeft} = this.containerLayout;
	// We have to return the value as Number so we can't use `measureWindow` which takes a callback
	const {width: textNodeWidth, left: textNodeLeft} = this.childRefs[index].getBoundingClientRect();

The function depends on the width and left position of the text (from getBoundingClientRect) and the container of the text (from containerLayout that is set from onLayout of the container).

The problem is, onLayout is called even before the navigation animation is done. We can validate this one by logging the onLayout and onEntryTransitionEnd. This means the onLayout position is wrong.

That's why, when we first open the confirmation page, the left position of the container is bigger than the text itself (by bigger, this means it is positioned more to the right than the text. it makes sense because the navigation animation slides from the right). And when we go to the edit amount page and go back, the container's left position is smaller than the text because the navigation animation now slides from the left.

If we refresh the page, there is no animation going on, so the value is correct.

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

Don't rely on the onLayout value. Instead, use the container ref and getBoundingClientRect to get the width and left position, the same as what we did with the text.

this.containerRef.getBoundingClientRect()

[melvin-bot]

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

[melvin-bot]

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

[laurenreidexpensify]

not overdue, added to upwork

[melvin-bot]

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

[s77rt]

@spcheema Thanks for the proposal. Your RCA does not seem accurate enough, I don't see how the display name container is related to changing the amount. Also onLayout returns different values but it has nothing to do with the element being visible or not, the callback just seems to fire during animation.

[spcheema]

@s77rt Thanks for reviewing. I'll come back to more explanation. Kindly allow sometime.

[spcheema]

@s77rt Here is the first video of amount related issue I mentioned in RCA:

https://www.loom.com/share/7568b39882da43b49b9d47b692f74c16

If you noticed, the amount container size increased after changing the amount. So that name container (parent actually) width is smaller now. Due to that tooltip shifted to the right.

Working on second point and update here shortly.

[spcheema]

Also onLayout returns different values but it has nothing to do with the element being visible or not, the callback just seems to fire during animation.

@s77rt Here is a bit more explanation of why I said incorrect value.

LayoutEvent is triggered immediately when the layout changes in other words component is mounted and it does not wait for RMN (ScreenWrapper) animation transition to complete. Due to that this.containerLayout holds the element position which is outside the viewport.

The element is first visible (of course outside the viewport) then navigate right -> left to make it visible. When it becomes visible, the Layout event stores incorrect layout details. This is the reason I mentioned onLayout returns incorrect values. When refreshing the page, animation does not occur so values are correct, but when a user navigates from the user selector to the confirmation page values are different. This is the second root cause.

This is the reason I mentioned the onLayout returns incorrect values. RCA is correct but the terminology I used to explain is different.

Incorrect value due to onLayout triggered first and becomes visible in the viewport after the animation is complete.

I have attached a screenshot of the value returns in both cases. Kindly note the left value 1193 vs 1505.47509765625 Also notice, the transition end is triggered after onLayout which is already highlighted by another proposal.

On page refresh:

On navigate

I hope it makes sense. I completely agree I should have explained all the theories earlier.

That's why the solution is to use parent element reference because the child span element is overflowing the parent container.

I thought of explaining the solution details as well.
Yellow is the parent which is the block element and space is the inline element overflow of the parent DOM.

Another approach is to delay setting up containerLayout value until the transition event is complete but I found using the parent DOM is the neat approach.

[s77rt]

@spcheema Thanks for the update. I see you are testing in Split money flow, this explains the display name container changing width. However I am afraid your explanation for the onLayout came a little late as this was explained by @bernhardoj already.

That's why the solution is to use parent element reference because the child span element is overflowing the parent container.

This solution is also misleading, we already use the parent element for calculation, it's just that we use one static value that is calculated on mount (via onLayout) and the correct solution is to calculate that value every time before showing the tooltip (by calling getBoundingClientRect).

I think @bernhardoj had the first complete proposal so it seems fair to go with their proposal.

[s77rt]

@bernhardoj Thanks for the proposal. Your RCA is correct, this is solely dependent on the onLayout being called too early. Using getBoundingClientRect to get the correct value just before rendering the tooltip (instead of using the stale value that was calculated on mount) looks good to me.

🎀 👀 🎀 C+ reviewed
Link to proposal

[melvin-bot]

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

[spcheema]

@s77rt Happy with your decision since I want to submit proposal first no doubt honestly. Because RCA was quite clear to me.

But I won't buy misleading thing you are selling😉

Go for it @bernhardoj 🚀🚀🚀

[melvin-bot]

📣 @s77rt 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer 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 📖

[melvin-bot]

📣 @dhanashree-sawant 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[bernhardoj]

PR is ready

cc: @s77rt

[melvin-bot]

🎯 ⚡️ Woah @s77rt / @bernhardoj, great job pushing this forwards! ⚡️

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

• when @bernhardoj got assigned: 2023-08-01 09:25:00 Z
• when the PR got merged: 2023-08-02 12:30:14 UTC

On to the next one 🚀

[laurenreidexpensify]

#23990 - this was deployed to prod on 9 august - I am working on payment now

[laurenreidexpensify]

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:

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

[laurenreidexpensify]

Payment Summary:

• External issue reporter - @dhanashree-sawant, paid in Upwork $250
• Contributor that fixed the issue - @bernhardoj, paid in Upwork $1000 + 3 day bonus $500 = $1500
• Contributor+ that helped on the issue and/or PR - @s77rt, paid in Upwork $1000 + 3 day bonus $500 = $1500

[laurenreidexpensify]

@s77rt please complete the regression checklist above and then we can close this out. Thanks

[s77rt]

• The PR that introduced the bug has been identified: This existed since day one Added Tooltips to the report users' titles.  #1632
• The offending PR has been commented on: I can't comment as the PR is locked
• A discussion in #expensify-bugs has been started: Not needed. Not much to discuss
• Determine if we should create a regression test for this bug: No

[s77rt]

@laurenreidexpensify ^