-
Notifications
You must be signed in to change notification settings - Fork 3k
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] Tooltip appeared on the global create disappear and did not appear back #54925
Comments
Triggered auto assignment to @mallenexpensify ( |
Need to test mobile app and mweb. |
@rayane-djouah will you be able to look for fix for this one? @mallenexpensify me and @rayane-djouah will take over we are working on polish issues for #migrate project |
still looking for fix this seems to be dupe (or atleast same root cause) of #54867 |
awaiting proposals |
@ishpaul777 , will you two be able to start work on this soon? |
@mallenexpensify we aren't not able to figure this out, chatted with @rayane-djouah root cause is still unclear to us so i asked to put "help wanted" here https://expensify.slack.com/archives/C07NMDKEFMH/p1736532130999349?thread_ts=1736462858.720569&cid=C07NMDKEFMH |
Job added to Upwork: https://www.upwork.com/jobs/~021878927417137220974 |
Current assignees @rayane-djouah and @ishpaul777 are eligible for the External assigner, not assigning anyone new. |
@ishpaul777 @rayane-djouah I can reproduce this issue. I tested it with my proposal here, and the issue is resolved. |
This comment has been minimized.
This comment has been minimized.
@linhvovan29546 can you please move relevant part from your proposal here for me to review? |
🚨 Edited by proposal-police: This proposal was edited at 2025-01-16 00:30:54 UTC. 🚨 Edited by proposal-police: This proposal was edited at 2025-01-16 00:30:54 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Tooltip appeared on the global create disappear and did not appear back What is the root cause of that problem?The tooltip layout is retrieved using onLayout from
When navigating back from another screen, getTooltipStyles re-renders to display the tooltip.App/src/components/Tooltip/BaseGenericTooltip/index.tsx Lines 69 to 71 in 5133090
However, getTooltipStyles re-renders before the screen translation is complete, causing elementAtTargetCenterX to retrieve the incorrect element (the overlay of the workspace switcher). As a result, isOverlappingAtTop returns true, which causes the tooltip to be displayed below the FAB button, making it invisible.
App/src/styles/utils/generators/TooltipStyleUtils/isOverlappingAtTop/index.ts Lines 23 to 31 in 5133090
App/src/styles/utils/generators/TooltipStyleUtils/index.ts Lines 155 to 157 in 5133090
What changes do you think we should make in order to solve the problem?To address this issue, we can use const elementRef = useRef();
const [isVisibleElement, setIsVisibleElement] = useState(false);
const getBounds = (bounds: DOMRect): LayoutRectangle => {
const targetElement = elementRef.current?._childNode;
const elementAtPoint = document.elementFromPoint(bounds.x, bounds.y);
if (elementAtPoint && 'contains' in elementAtPoint && targetElement && 'contains' in targetElement) {
const isElementVisible =
elementAtPoint instanceof HTMLElement &&
(targetElement?.contains(elementAtPoint) || elementAtPoint?.contains(targetElement)); // We can update condition here, if we need other expected
setIsVisibleElement(isElementVisible)
}
return bounds;
};
...
<BoundsObserver
enabled={shouldRender}
onBoundsChange={(bounds) => {
updateTargetBounds(getBounds(bounds));
}}
ref={elementRef}
>
{React.cloneElement(children as React.ReactElement, {
onLayout: (e: LayoutChangeEventWithTarget) => {
if (!shouldMeasure) {
setShouldMeasure(true);
}
// e.target is specific to native, use e.nativeEvent.target on web instead
const target = e.target || e.nativeEvent.target;
show.current = () => measureTooltipCoordinate(target, updateTargetBounds, showTooltip);
},
})}
</BoundsObserver> We need
shouldRender={shouldRender && isVisibleElement} For the detailed code, we can discuss it later in PR. POC
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?No 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. |
@ishpaul777 I used the steps from this issue to reproduce the problem on mWeb Android and Web. |
Updated RCA after reviewing the details. |
@linhvovan29546 can you please share a test branch, Proposal sounds good to me just want to run some tests before i recommend the proposal |
Sure, the test branch is available here |
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.81-6
Reproducible in staging?: y
Reproducible in production?: y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
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
Expensify/Expensify Issue URL:
Issue reported by: @flaviadefaria
Slack conversation (hyperlinked to channel name): migrate
Action Performed:
Expected Result:
tooltip should display again
Actual Result:
tooltip dismissed and never appears back
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @The text was updated successfully, but these errors were encountered: