Skip to content

fix skip tooltip rendering when element is not focused #61708

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

Merged
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/components/Tooltip/BaseTooltip/index.tsx
Original file line number Diff line number Diff line change
@@ -1,18 +1,17 @@
import {useIsFocused} from '@react-navigation/native';
import {BoundsObserver} from '@react-ng/bounds-observer';
import type {ForwardedRef} from 'react';
import React, {forwardRef, memo, useCallback, useRef} from 'react';
import type {LayoutRectangle} from 'react-native';
import Hoverable from '@components/Hoverable';
import GenericTooltip from '@components/Tooltip/GenericTooltip';
import type TooltipProps from '@components/Tooltip/types';
import * as DeviceCapabilities from '@libs/DeviceCapabilities';
import {hasHoverSupport} from '@libs/DeviceCapabilities';

type MouseEvents = {
onMouseEnter: (e: MouseEvent) => void | undefined;
};

const hasHoverSupport = DeviceCapabilities.hasHoverSupport();

/**
* A component used to wrap an element intended for displaying a tooltip. The term "tooltip's target" refers to the
* wrapped element, which, upon hover, triggers the tooltip to be shown.
Expand Down Expand Up @@ -53,6 +52,7 @@ function chooseBoundingBox(target: HTMLElement, clientX: number, clientY: number
function Tooltip({children, shouldHandleScroll = false, ...props}: TooltipProps, ref: ForwardedRef<BoundsObserver>) {
const target = useRef<HTMLElement | null>(null);
const initialMousePosition = useRef({x: 0, y: 0});
const isFocused = useIsFocused();

const updateTargetAndMousePosition = useCallback((e: MouseEvent) => {
if (!(e.currentTarget instanceof HTMLElement)) {
Expand Down Expand Up @@ -87,8 +87,8 @@ function Tooltip({children, shouldHandleScroll = false, ...props}: TooltipProps,
[children, updateTargetAndMousePosition],
);

// Skip the tooltip and return the children if the device does not support hovering
if (!hasHoverSupport) {
// If the device doesn't support hover or the element isn't focused, skip rendering the tooltip and just return the children.
if (!hasHoverSupport || !isFocused) {
return children;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasHoverSupport is a function, it should be invoked here. Adding isFocused causing the tooltip to not show on the LHN.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI, we had a long discussion before about this Safari-only issue here and decided to do nothing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reference. It looks like we’re missing a test case on LHN @huult could you please take a look on this one

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also crashes when open a report context menu. Context menu lives outside the navigation container.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, I'm checking

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bernhardoj thank you for reporting this crash. Could you share the steps to reproduce it? Many thanks.


Expand Down
Loading