Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.98k stars 2.99k forks source link

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

Open IuliiaHerets opened 2 weeks ago

IuliiaHerets commented 2 weeks ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com 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 https://github.com/Expensify/App/pull/54869 Email or phone of affected tester (no customers): applausetester+010108kh@applause.expensifail.com 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:

Screenshots/Videos

https://github.com/user-attachments/assets/e9f44fc2-24a1-40b6-ac45-769b3cd5af16

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-01-20
Issue OwnerCurrent Issue Owner: @mollfpr
melvin-bot[bot] commented 2 weeks ago

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 commented 2 weeks ago

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. https://github.com/Expensify/App/blob/4491e856b1692cdae4379d875713efc8429e1fc6/src/styles/utils/generators/TooltipStyleUtils/index.ts#L132 The issue arises from the calculation based on elementAtTargetCenterX. When navigating to another link, the point at targetCenterX becomes inaccurate. https://github.com/Expensify/App/blob/4491e856b1692cdae4379d875713efc8429e1fc6/src/styles/utils/generators/TooltipStyleUtils/isOverlappingAtTop/index.ts#L31-L42

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.

https://github.com/Expensify/App/blob/4491e856b1692cdae4379d875713efc8429e1fc6/src/styles/utils/generators/TooltipStyleUtils/index.ts#L129-L133

            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

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 commented 2 weeks ago

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. https://github.com/Expensify/App/blob/fb487a59dada9d0d0f8215408287ba86f0eac98b/src/pages/home/report/ReportActionCompose/ReportActionCompose.tsx#L429-L430 https://github.com/Expensify/App/blob/fb487a59dada9d0d0f8215408287ba86f0eac98b/src/pages/home/report/ReportActionCompose/ReportActionCompose.tsx#L143

However, isOverlappingAtTop is true, so the tooltip is shown below the element. https://github.com/Expensify/App/blob/fb487a59dada9d0d0f8215408287ba86f0eac98b/src/styles/utils/generators/TooltipStyleUtils/index.ts#L129-L133

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. https://github.com/Expensify/App/blob/fb487a59dada9d0d0f8215408287ba86f0eac98b/src/styles/utils/generators/TooltipStyleUtils/isOverlappingAtTop/index.ts#L30-L44

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. https://github.com/Expensify/App/blob/fb487a59dada9d0d0f8215408287ba86f0eac98b/src/pages/home/report/ReportActionCompose/ReportActionCompose.tsx#L429-L430

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

huult commented 1 week ago

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

https://github.com/Expensify/App/blob/f6c3290613c3c6cf321b648ac9c0e8bdedacce47/src/styles/utils/generators/TooltipStyleUtils/isOverlappingAtTop/index.ts#L38

https://github.com/Expensify/App/blob/c7a9752ad6782d2cd8b24dd8967cdcd3b90c3f13/src/pages/home/report/ReportActionCompose/ReportActionCompose.tsx#L440

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.

https://github.com/Expensify/App/blob/13891aa1ca99e2f8ba42916548ee76fbf5cc11c5/src/styles/utils/generators/TooltipStyleUtils/index.ts#L132

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.

https://github.com/Expensify/App/blob/f6c3290613c3c6cf321b648ac9c0e8bdedacce47/src/styles/utils/generators/TooltipStyleUtils/isOverlappingAtTop/index.ts#L23-L45

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 https://github.com/user-attachments/assets/7211e9ab-9cd7-435b-9ed2-d4375decb8c3

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.

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

mollfpr commented 5 days ago

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?

https://github.com/user-attachments/assets/9507b5ef-0df8-45eb-80bf-958c534000a5


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.

https://github.com/user-attachments/assets/60755ec8-1437-4099-9f91-ccd7adfe2298

linhvovan29546 commented 5 days ago

@linhvovan29546 What is the cause of the inaccuracy?

@mollfpr The RCA is the same as described here: https://github.com/Expensify/App/issues/54925#issuecomment-2594189558. This issue may be resolved by the selected proposal mentioned here. https://github.com/Expensify/App/issues/54924#issuecomment-2602080434

mollfpr commented 5 days ago

@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.

https://github.com/user-attachments/assets/24622ea4-f672-4b7c-990c-67ad9e2b26ca

linhvovan29546 commented 5 days ago

@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 commented 5 days ago

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 commented 5 days ago

@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 commented 5 days ago

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

https://github.com/user-attachments/assets/a3422352-5094-46cd-bdf2-dcdc0b221f8a

melvin-bot[bot] commented 5 days ago

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

bernhardoj commented 5 days ago

@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. https://github.com/Expensify/App/blob/bec182c7eeecec84a7a72fefd63220866edae96b/src/styles/utils/generators/TooltipStyleUtils/isOverlappingAtTop/index.ts#L31

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

https://github.com/user-attachments/assets/82b8fdb6-405a-4d78-8877-745c9da0b82c

It will be fixed by https://github.com/Expensify/App/issues/54924

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

melvin-bot[bot] commented 2 days ago

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

melvin-bot[bot] commented 22 hours ago

@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!