Skip to content

fix: close tooltip on Escape #540

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.

Sign up for GitHub

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

Closed
nrps9909 wants to merge 2 commits into from
Closed

fix: close tooltip on Escape #540

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a0625c
fix: close tooltip on Escape
nrps9909 Aug 26, 2026
a005347
revert: defer Escape handling to trigger
nrps9909 Aug 27, 2026
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
35 changes: 30 additions & 5 deletions src/Tooltip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type {
TriggerRef,
} from '@rc-component/trigger';
import Trigger from '@rc-component/trigger';
import { useId } from '@rc-component/util';
import { useControlledState, useEvent, useId } from '@rc-component/util';
import { clsx } from 'clsx';
import * as React from 'react';
import { useImperativeHandle, useRef } from 'react';
Expand Down Expand Up @@ -77,6 +77,8 @@ const Tooltip = React.forwardRef<TooltipRef, TooltipProps>((props, ref) => {
align = {},
destroyOnHidden = false,
defaultVisible,
visible,
popupVisible,
getTooltipContainer,
arrowContent,
overlay,
Expand All @@ -89,14 +91,36 @@ const Tooltip = React.forwardRef<TooltipRef, TooltipProps>((props, ref) => {

const mergedId = useId(id);
const triggerRef = useRef<TriggerRef>(null);
const controlledVisible = 'visible' in props ? visible : popupVisible;
const [mergedVisible, setMergedVisible] = useControlledState(
defaultVisible ?? false,
controlledVisible,
);

const onInternalVisibleChange = useEvent((nextVisible: boolean) => {
setMergedVisible(nextVisible);
onVisibleChange?.(nextVisible);
});

useImperativeHandle(ref, () => triggerRef.current);

const extraProps: Partial<TooltipProps & TriggerProps> = { ...restProps };

if ('visible' in props) {
extraProps.popupVisible = props.visible;
}
React.useEffect(() => {
if (!mergedVisible) {

@yoyo837 yoyo837 Aug 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While disabled, Trigger hides the popup but preserves its raw open state. This effect still listens because mergedVisible is true, so Escape clears that state and the popup no longer returns when re-enabled.

return undefined;
}

const targetWindow = triggerRef.current?.nativeElement?.ownerDocument.defaultView;
const onKeyDown = (event: KeyboardEvent) => {
if (event.key === 'Escape') {

@yoyo837 yoyo837 Aug 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This bypasses Portal's IME guards (event.isComposing and the post-compositionend lock), so Escape used to cancel CJK composition also closes the tooltip.

onInternalVisibleChange(false);
}
};

targetWindow?.addEventListener('keydown', onKeyDown);

@yoyo837 yoyo837 Aug 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Trigger/Portal already handles Escape with a topmost-overlay guard. This listener runs for every visible tooltip, so one keypress can close background tooltips and call onVisibleChange(false) twice in controlled mode. Could we reuse Trigger's existing Escape path?

return () => targetWindow?.removeEventListener('keydown', onKeyDown);
}, [mergedVisible, onInternalVisibleChange]);

// ========================= Arrow ==========================
// Process arrow configuration
Expand Down Expand Up @@ -148,7 +172,8 @@ const Tooltip = React.forwardRef<TooltipRef, TooltipProps>((props, ref) => {
ref={triggerRef}
popupAlign={align}
getPopupContainer={getTooltipContainer}
onOpenChange={onVisibleChange}
popupVisible={mergedVisible}

@yoyo837 yoyo837 Aug 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Always passing popupVisible forces Trigger into controlled mode. With unique and enter/leave delays, Trigger delays before onOpenChange, then UniqueProvider applies the delay again, effectively doubling it.

onOpenChange={onInternalVisibleChange}
afterOpenChange={afterVisibleChange}
popupMotion={motion}
defaultPopupVisible={defaultVisible}
Expand Down
41 changes: 41 additions & 0 deletions tests/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,47 @@ describe('rc-tooltip', () => {
);
expect(domRef.current).toBeTruthy();
});

it('closes an uncontrolled tooltip with Escape', () => {
const onVisibleChange = jest.fn();
const { container } = render(
<Tooltip trigger={['click']} overlay="Tooltip content" onVisibleChange={onVisibleChange}>
<button type="button">Show tooltip</button>
</Tooltip>,
);

fireEvent.click(container.querySelector('button'));
expect(onVisibleChange).toHaveBeenLastCalledWith(true);
expect(container.querySelector('.rc-tooltip')).not.toHaveClass('rc-tooltip-hidden');

fireEvent.keyDown(window, { key: 'Escape' });
expect(onVisibleChange).toHaveBeenLastCalledWith(false);
expect(container.querySelector('.rc-tooltip')).toHaveClass('rc-tooltip-hidden');

fireEvent.click(container.querySelector('button'));
expect(onVisibleChange).toHaveBeenLastCalledWith(true);
expect(container.querySelector('.rc-tooltip')).not.toHaveClass('rc-tooltip-hidden');
});

it('requests closure without overriding controlled visibility', () => {
const onVisibleChange = jest.fn();
const { container, rerender } = render(
<Tooltip visible overlay="Tooltip content" onVisibleChange={onVisibleChange}>
<button type="button">Show tooltip</button>
</Tooltip>,
);

fireEvent.keyDown(window, { key: 'Escape' });
expect(onVisibleChange).toHaveBeenLastCalledWith(false);
expect(container.querySelector('.rc-tooltip')).not.toHaveClass('rc-tooltip-hidden');

rerender(
<Tooltip visible={false} overlay="Tooltip content" onVisibleChange={onVisibleChange}>
<button type="button">Show tooltip</button>
</Tooltip>,
);
expect(container.querySelector('.rc-tooltip')).toHaveClass('rc-tooltip-hidden');
});
});

describe('destroyOnHidden', () => {
Expand Down