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

[Due for payment 2025-02-19] [$250] mWeb - Chat - Tooltip appears below composer after returning from "Go back to home page" link #55114

Closed
2 of 8 tasks
IuliiaHerets opened this issue Jan 11, 2025 · 42 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Jan 11, 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: 9.0.84-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail: Exp #54869
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team
Device used: iPhone 15 Pro Max / iOS 18.2
App Component: Chat Report View

Action Performed:

  1. Go to staging.new.expensify.com
  2. Log in with a new account.
  3. Create a workspace.
  4. Go to workspace chat.
  5. Do not tap + as the tooltip should not be dismissed.
  6. Send this link https://staging.new.expensify.com/r/6732929228344392/trip/2905990518128580779/0
  7. Tap on the link.
  8. Tap "Go back to home page"

Expected Result:

The educational tooltip should appear above the composer.

Actual Result:

The educational tooltip appears below the composer.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6711604_1736574805440.ScreenRecording_01-11-2025_13-28-55_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021878937549151677268
  • Upwork Job ID: 1878937549151677268
  • Last Price Increase: 2025-02-03
Issue OwnerCurrent Issue Owner: @johncschuster
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 11, 2025
Copy link

melvin-bot bot commented Jan 11, 2025

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

@linhvovan29546
Copy link
Contributor

linhvovan29546 commented Jan 11, 2025

Edited by proposal-police: This proposal was edited at 2025-01-11 12:50:26 UTC.

Proposal

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

mWeb - Chat - Tooltip appears below composer after returning from "Go back to home page" link

What is the root cause of that problem?

When returning from another link, the isOverlappingAtTop calculation is incorrect. This causes the function to return true, setting shouldShowBelow to true, which makes the tooltip render below the composer instead of above it.

!!(tooltip && isOverlappingAtTop(tooltip, xOffset, yOffset, tooltipTargetWidth, tooltipTargetHeight)) ||

The issue arises from the calculation based on elementAtTargetCenterX. When navigating to another link, the point at targetCenterX becomes inaccurate.
const elementAtTargetCenterX = document.elementFromPoint(targetCenterX, yOffset);
// Ensure it's not the already rendered element of this very tooltip, so the tooltip doesn't try to "avoid" itself
if (!elementAtTargetCenterX || ('contains' in tooltip && tooltip.contains(elementAtTargetCenterX))) {
return false;
}
const rectAtTargetCenterX = elementAtTargetCenterX.getBoundingClientRect();
// Ensure it's not overlapping with another element by checking if the yOffset is greater than the top of the element
// and less than the bottom of the element. Also ensure the tooltip target is not completely inside the elementAtTargetCenterX by vertical direction
const isOverlappingAtTargetCenterX = yOffset > rectAtTargetCenterX.top && yOffset < rectAtTargetCenterX.bottom && yOffset + tooltipTargetHeight > rectAtTargetCenterX.bottom;

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

We can easily fix this by introducing a new prop shouldForceRenderingTop, similar to the existing shouldForceRenderingBelow.

shouldShowBelow =
shouldForceRenderingBelow ||
yOffset - tooltipHeight - POINTER_HEIGHT < GUTTER_WIDTH + titleBarHeight ||
!!(tooltip && isOverlappingAtTop(tooltip, xOffset, yOffset, tooltipTargetWidth, tooltipTargetHeight)) ||
anchorAlignment.vertical === CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP;

            if (!shouldForceRenderingTop) {
                shouldShowBelow =
                    (shouldForceRenderingBelow ||
                        yOffset - tooltipHeight - POINTER_HEIGHT < GUTTER_WIDTH + titleBarHeight ||
                        !!(tooltip && isOverlappingAtTop(tooltip, xOffset, yOffset, tooltipTargetWidth, tooltipTargetHeight)) ||
                        anchorAlignment.vertical === CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP) && shouldForceRenderingBelow;
            }

And set shouldForceRenderingTop flag is true in here, then the tooltip will always render at the top. Since we are certain that the tooltip will not overlap at the top.

                   <EducationalTooltip
                        ...
                        shouldForceRenderingTop // new code
                    >
POC
Before After
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2025-01-11.at.19.36.05.mp4
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2025-01-11.at.19.21.57.mp4

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)

After spending some time looking into the issue here, I can confirm that this proposal also resolves the issue.

@bernhardoj
Copy link
Contributor

Proposal

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

Composer tooltip shows below the composer after going back from not found page.

What is the root cause of that problem?

When we go to the not found page, the tooltip isn't rendered. When we go back, the tooltip is rendered again and recalculates the position.

<EducationalTooltip
shouldRender={shouldShowProductTrainingTooltip}

shouldShowEducationalTooltip && isScreenFocused,

However, isOverlappingAtTop is true, so the tooltip is shown below the element.

shouldShowBelow =
shouldForceRenderingBelow ||
yOffset - tooltipHeight - POINTER_HEIGHT < GUTTER_WIDTH + titleBarHeight ||
!!(tooltip && isOverlappingAtTop(tooltip, xOffset, yOffset, tooltipTargetWidth, tooltipTargetHeight)) ||
anchorAlignment.vertical === CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP;

isOverlappingAtTop works by checking the element at the x-center position of the tooltip target element, that is the element that is wrapped with the tooltip, in this case the composer, and check if they are overlapping.

const targetCenterX = xOffset + tooltipTargetWidth / 2;
const elementAtTargetCenterX = document.elementFromPoint(targetCenterX, yOffset);
// Ensure it's not the already rendered element of this very tooltip, so the tooltip doesn't try to "avoid" itself
if (!elementAtTargetCenterX || ('contains' in tooltip && tooltip.contains(elementAtTargetCenterX))) {
return false;
}
const rectAtTargetCenterX = elementAtTargetCenterX.getBoundingClientRect();
// Ensure it's not overlapping with another element by checking if the yOffset is greater than the top of the element
// and less than the bottom of the element. Also ensure the tooltip target is not completely inside the elementAtTargetCenterX by vertical direction
const isOverlappingAtTargetCenterX = yOffset > rectAtTargetCenterX.top && yOffset < rectAtTargetCenterX.bottom && yOffset + tooltipTargetHeight > rectAtTargetCenterX.bottom;
return isOverlappingAtTargetCenterX;

In our case, the overlapping element is the not found page. It's because while the page is closing, the calculation is happening.

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

We need to wait until the page is fully closed. We can't use transitionEnd event because it's not fired. We can follow the doc by using InteractionManager.

const [isScreenTransitionEnded, setIsScreenTransitionEnded] = useState(false);

useEffect(() => {
    if (!isScreenFocused) {
        setIsScreenTransitionEnded(false);
        return;
    }
    InteractionManager.runAfterInteractions(() => {
        setIsScreenTransitionEnded(true);
    });
}, [isScreenFocused]);

Then update the shouldRender condition to only show if transition ends.

<EducationalTooltip
shouldRender={shouldShowProductTrainingTooltip}

shouldRender={shouldShowProductTrainingTooltip && isScreenTransitionEnded}

We can create a useIsFullyFocused hook if needed.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

@melvin-bot melvin-bot bot added the Overdue label Jan 13, 2025
@huult
Copy link
Contributor

huult commented Jan 13, 2025

Proposal

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

Tooltip appears below composer after returning from "Go back to home page" link

What is the root cause of that problem?

This issue occurs because we calculate the x-center position of the target before the compose box has finished rendering

const rectAtTargetCenterX = elementAtTargetCenterX.getBoundingClientRect();

For this reason, the calculated value for isOverlappingAtTop is incorrect, returning true. As a result, the condition shouldShowBelow also evaluates to true, causing this issue.

!!(tooltip && isOverlappingAtTop(tooltip, xOffset, yOffset, tooltipTargetWidth, tooltipTargetHeight)) ||

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

This issue can occur in other cases with similar conditions. To fix this, we should update the logic to calculate isOverlappingAtTop using requestAnimationFrame to delay execution until the next render cycle, ensuring the x-center position of the target is correct.

const isOverlappingAtTop: IsOverlappingAtTop = (tooltip, xOffset, yOffset, tooltipTargetWidth, tooltipTargetHeight) => {
if (typeof document.elementFromPoint !== 'function') {
return false;
}
// Use the x center position of the target to prevent wrong element returned by elementFromPoint
// in case the target has a border radius or is a multiline text.
const targetCenterX = xOffset + tooltipTargetWidth / 2;
const elementAtTargetCenterX = document.elementFromPoint(targetCenterX, yOffset);
// Ensure it's not the already rendered element of this very tooltip, so the tooltip doesn't try to "avoid" itself
if (!elementAtTargetCenterX || ('contains' in tooltip && tooltip.contains(elementAtTargetCenterX))) {
return false;
}
const rectAtTargetCenterX = elementAtTargetCenterX.getBoundingClientRect();
// Ensure it's not overlapping with another element by checking if the yOffset is greater than the top of the element
// and less than the bottom of the element. Also ensure the tooltip target is not completely inside the elementAtTargetCenterX by vertical direction
const isOverlappingAtTargetCenterX = yOffset > rectAtTargetCenterX.top && yOffset < rectAtTargetCenterX.bottom && yOffset + tooltipTargetHeight > rectAtTargetCenterX.bottom;
return isOverlappingAtTargetCenterX;
};

Update to:

  const isOverlappingAtTop: IsOverlappingAtTop = (tooltip, xOffset, yOffset, tooltipTargetWidth, tooltipTargetHeight) => {
      if (typeof document.elementFromPoint !== 'function') {
          return false;
      }

      // Use the x center position of the target to prevent wrong element returned by elementFromPoint
      // in case the target has a border radius or is a multiline text.
      const targetCenterX = xOffset + tooltipTargetWidth / 2;
      const elementAtTargetCenterX = document.elementFromPoint(targetCenterX, yOffset);

      // Ensure it's not the already rendered element of this very tooltip, so the tooltip doesn't try to "avoid" itself
      if (!elementAtTargetCenterX || ('contains' in tooltip && tooltip.contains(elementAtTargetCenterX))) {
          return false;
      }

      const rectAtTargetCenterX = elementAtTargetCenterX.getBoundingClientRect();

      let isOverlappingAtTargetCenterX = false; // add this line

      requestAnimationFrame(() => { // add this line
          isOverlappingAtTargetCenterX = yOffset > rectAtTargetCenterX.top && yOffset < rectAtTargetCenterX.bottom && yOffset + tooltipTargetHeight > rectAtTargetCenterX.bottom; // add this line
      }); // add this line

      // Ensure it's not overlapping with another element by checking if the yOffset is greater than the top of the element
      // and less than the bottom of the element. Also ensure the tooltip target is not completely inside the elementAtTargetCenterX by vertical direction

      return isOverlappingAtTargetCenterX;
  };
POC
Screen.Recording.2025-01-13.at.22.48.52.mov

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

This is a UI bug; no tests are needed.

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.

@johncschuster johncschuster 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 mWeb - Chat - Tooltip appears below composer after returning from "Go back to home page" link [$250] mWeb - Chat - Tooltip appears below composer after returning from "Go back to home page" link Jan 13, 2025
Copy link

melvin-bot bot commented Jan 13, 2025

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

@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

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

@melvin-bot melvin-bot bot removed the Overdue label Jan 13, 2025
@garrettmknight garrettmknight moved this to Bugs and Follow Up Issues in [#whatsnext] #expense Jan 14, 2025
Copy link

melvin-bot bot commented Jan 17, 2025

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

@melvin-bot melvin-bot bot added the Overdue label Jan 17, 2025
@mollfpr
Copy link
Contributor

mollfpr commented Jan 20, 2025

The issue arises from the calculation based on elementAtTargetCenterX. When navigating to another link, the point at targetCenterX becomes inaccurate.

@linhvovan29546 What is the cause of the inaccuracy? Also, I'm not onboard yet with the workaround solution.


In our case, the overlapping element is the not found page. It's because while the page is closing, the calculation is happening.

@bernhardoj I also found the same issue on the desktop web with smallscreen when try pasting the text. Is this the same root cause?

Screen.Recording.2025-01-20.at.18.07.17.mp4

This issue occurs because we calculate the x-center position of the target before the compose box has finished rendering

@huult Same question with the issue of pasting the text.

Simulator.Screen.Recording.-.iPhone.15.17.2.-.2025-01-20.at.18.20.57.mov

@melvin-bot melvin-bot bot removed the Overdue label Jan 20, 2025
@linhvovan29546
Copy link
Contributor

@linhvovan29546 What is the cause of the inaccuracy?

@mollfpr The RCA is the same as described here: #54925 (comment). This issue may be resolved by the selected proposal mentioned here. #54924 (comment)

@mollfpr
Copy link
Contributor

mollfpr commented Jan 20, 2025

@linhvovan29546 You're RCA mentioned that the inaccuracy happened on screen transition. How about this issue? There's no screen transition and the compose box is already rendered.

Screen.Recording.2025-01-20.at.18.07.17.mp4

@linhvovan29546
Copy link
Contributor

@mollfpr You can add a log here and observe that the elementAtTargetCenterX is not correct. I haven't checked the issue with pasted text, but I think it’s the same.

@mollfpr
Copy link
Contributor

mollfpr commented Jan 20, 2025

When navigating to another link, the point at targetCenterX becomes inaccurate.

@linhvovan29546 Even the elementAtTargetCenterX is not correct on the pasting text, the root is not caused because we navigate from other screen right? So, I'm not sure what the cause elementAtTargetCenterX return incorrect element.

@linhvovan29546
Copy link
Contributor

@mollfpr

the root is not caused because we navigate from other screen right?

Yes, my RCA is outdated

So, I'm not sure what the cause elementAtTargetCenterX return incorrect element.

The tooltip style re-render too soon, as the layout isn’t fully ready (navigation, parent layout change, etc...)

@huult
Copy link
Contributor

huult commented Jan 20, 2025

@mollfpr Yes, it looks like the same RCA, and applying my solution will fix it.

Screen.Recording.2025-01-20.at.22.31.46.mov

Copy link

melvin-bot bot commented Jan 20, 2025

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@bernhardoj
Copy link
Contributor

bernhardoj commented Jan 21, 2025

@mollfpr it's a different RCA. For the pasting case, it overlaps with the composer content instead. It's simply because the yOffset is never updated.

const elementAtTargetCenterX = document.elementFromPoint(targetCenterX, yOffset);

yOffset is the y position of the composer, but it's never updated even when the composer is expanded.

a.mp4

It will be fixed by #54924

Btw, somehow I can't repro this issue anymore. (it's still overlapping with not found page, but isOverlappingAtTop is false)

Copy link

melvin-bot bot commented Jan 24, 2025

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

@melvin-bot melvin-bot bot removed the Overdue label Feb 6, 2025
Copy link

melvin-bot bot commented Feb 6, 2025

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

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

grgia commented Feb 6, 2025

Thanks for your proposal @bernhardoj, assigned

@huult
Copy link
Contributor

huult commented Feb 6, 2025

@mollfpr could you review my feedback and my proposal?

@bernhardoj
Copy link
Contributor

PR is ready

cc: @mollfpr

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Feb 12, 2025
@melvin-bot melvin-bot bot changed the title [$250] mWeb - Chat - Tooltip appears below composer after returning from "Go back to home page" link [Due for payment 2025-02-19] [$250] mWeb - Chat - Tooltip appears below composer after returning from "Go back to home page" link Feb 12, 2025
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Feb 12, 2025
Copy link

melvin-bot bot commented Feb 12, 2025

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

Copy link

melvin-bot bot commented Feb 12, 2025

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

If no regressions arise, payment will be issued on 2025-02-19. 🎊

For reference, here are some details about the assignees on this issue:

  • @mollfpr requires payment through NewDot Manual Requests
  • @bernhardoj requires payment through NewDot Manual Requests

Copy link

melvin-bot bot commented Feb 12, 2025

@mollfpr @johncschuster @mollfpr The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

@johncschuster
Copy link
Contributor

BugZero Checklist:

  • [Contributor] Classify the bug:
Bug classification

Source of bug:

  • 1a. Result of the original design (eg. a case wasn't considered)
  • 1b. Mistake during implementation
  • 1c. Backend bug
  • 1z. Other:

Where bug was reported:

  • 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
  • 2b. Reported on staging (eg. found during regression or PR testing)
  • 2d. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other:
  • [Contributor] 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:

  • [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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:

  • [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

Regression Test Proposal Template
  • [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

Precondition:

Test:

Do we agree 👍 or 👎

@johncschuster
Copy link
Contributor

Payment Summary

Contributor: @bernhardoj owed $250 via NewDot
Contributor+: @mollfpr owed $250 via NewDot

@bernhardoj
Copy link
Contributor

Requested in ND.

@mollfpr
Copy link
Contributor

mollfpr commented Feb 14, 2025

BugZero Checklist:

  • [Contributor] Classify the bug:
Bug classification

Source of bug:

  • 1a. Result of the original design (eg. a case wasn't considered)
  • 1b. Mistake during implementation
  • 1c. Backend bug
  • 1z. Other:

Where bug was reported:

  • 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
  • 2b. Reported on staging (eg. found during regression or PR testing)
  • 2d. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other:
  • [Contributor] 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: I don't think there's any offending PR here.

  • [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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: Not a critical issue.

  • [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

  • [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

Precondition:

Test:

  1. Login as a new user
  2. Have at least one workspace (create a new one if none)
  3. Open the workspace chat (don't press the + button so the tooltip keep shows)
  4. Send any invalid link to the chat (for example, https://staging.new.expensify.com/r/6732929228344392/trip/2905990518128580779/0)
  5. Press the link
  6. Go back
  7. Verify the composer tooltip is shown above the composer

Do we agree 👍 or 👎

@johncschuster
Copy link
Contributor

QA issue created. Payment summary is here! Both contributors are due payment via NewDot after the regression window has passed. Thank you!

@github-project-automation github-project-automation bot moved this from Bugs and Follow Up Issues to Done in [#whatsnext] #expense Feb 14, 2025
@JmillsExpensify
Copy link

$250 approved for @bernhardoj

@garrettmknight
Copy link
Contributor

$250 approved for @mollfpr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
Status: Done
Development

No branches or pull requests

9 participants