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

[$250] mWeb - Chat - Tooltip appears below composer after returning from "Go back to home page" link #55114

Open
2 of 8 tasks
IuliiaHerets opened this issue Jan 11, 2025 · 32 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review 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: @mollfpr
@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 added the Overdue label Jan 24, 2025
Copy link

melvin-bot bot commented Jan 25, 2025

@johncschuster @mollfpr this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

Copy link

melvin-bot bot commented Jan 27, 2025

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

Copy link

melvin-bot bot commented Jan 28, 2025

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

@johncschuster
Copy link
Contributor

@mollfpr bump on the comment above!

@mollfpr
Copy link
Contributor

mollfpr commented Jan 29, 2025

Sorry for the delay, I miss the notification 🙏


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

@huult, it doesn't solve the position of the text changes in the composer. Also, why should we delay the execution with requestAnimationFrame?

Screen.Recording.2025-01-29.at.20.37.42.mp4

@melvin-bot melvin-bot bot removed the Overdue label Jan 29, 2025
Copy link

melvin-bot bot commented Feb 3, 2025

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

@melvin-bot melvin-bot bot added the Overdue label Feb 3, 2025
Copy link

melvin-bot bot commented Feb 4, 2025

@johncschuster, @mollfpr Eep! 4 days overdue now. Issues have feelings too...

@huult
Copy link
Contributor

huult commented Feb 5, 2025

@mollfpr

it doesn't solve the position of the text changes in the composer.

Your tested case has a different RCA than this issue and may be related to #54924.

Also, why should we delay the execution with requestAnimationFrame?

We can use InteractionManager or something similar to calculate isOverlappingAtTargetCenterX after the composer finishes rendering. If the composer has not finished rendering, isOverlappingAtTargetCenterX will calculate the wrong value, causing this issue.

Copy link

melvin-bot bot commented Feb 6, 2025

@johncschuster, @mollfpr Still overdue 6 days?! Let's take care of this!

@johncschuster
Copy link
Contributor

@mollfpr bump on the above!

@mollfpr
Copy link
Contributor

mollfpr commented Feb 6, 2025

Okay, the issue on pasting text might be related to #54924.

I'm more inclined into solution from @bernhardoj because it's more reasonable to render the tooltip after the transition ends.

🎀👀🎀 C+ reviewed

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Status: Bugs and Follow Up Issues
Development

No branches or pull requests

7 participants