Skip to content

fix: lock focus inside preview when opened via keyboard #505

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

Open
aojunhao123 wants to merge 1 commit into
base: master
Choose a base branch
from
+55 −3
Open

fix: lock focus inside preview when opened via keyboard #505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 15 additions & 3 deletions src/Preview/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,9 @@ const Preview: React.FC<PreviewProps> = props => {
} = props;

const imgRef = useRef<HTMLImageElement>();
const wrapperRef = useRef<HTMLDivElement>(null);
const triggerRef = useRef<HTMLElement>(null);
const [wrapperEl, setWrapperEl] = useState<HTMLDivElement>(null);

const groupContext = useContext(PreviewGroupContext);
const showLeftOrRightSwitches = groupContext && count > 1;
const showOperationsProgress = groupContext && count >= 1;
Expand Down Expand Up @@ -366,6 +368,10 @@ const Preview: React.FC<PreviewProps> = props => {
const onVisibleChanged = (nextVisible: boolean) => {
if (!nextVisible) {
setLockScroll(false);

// Restore focus to the trigger element after leave animation
triggerRef.current?.focus?.();
triggerRef.current = null;
Comment on lines +373 to +374
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Apr 3, 2026

Choose a reason for hiding this comment

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

medium

When restoring focus, it's safer to check if the element is still in the document. Additionally, using { preventScroll: true } prevents the page from jumping if the trigger element has moved out of the viewport while the preview was open.

      if (triggerRef.current && document.body.contains(triggerRef.current)) {
        triggerRef.current.focus({ preventScroll: true });
      }
      triggerRef.current = null;

}
afterOpenChange?.(nextVisible);
};
Expand All @@ -385,7 +391,13 @@ const Preview: React.FC<PreviewProps> = props => {
};

// =========================== Focus ============================
useLockFocus(open && portalRender, () => wrapperRef.current);
useEffect(() => {
if (open) {
triggerRef.current = document.activeElement as HTMLElement;
}
Comment on lines +395 to +397
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Apr 3, 2026

Choose a reason for hiding this comment

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

medium

To avoid overwriting the trigger element with an element from inside the preview (which can happen if open is toggled quickly during the closing animation), we should only capture the active element if it's currently outside the preview wrapper.

Suggested change
if (open) {
triggerRef.current = document.activeElement as HTMLElement;
}
if (open && !wrapperEl?.contains(document.activeElement)) {
triggerRef.current = document.activeElement as HTMLElement;
}

Copy link
Copy Markdown
Member

@afc163 afc163 Apr 4, 2026

Choose a reason for hiding this comment

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

Check this one

Copy link
Copy Markdown
Contributor Author

@aojunhao123 aojunhao123 Apr 5, 2026

Choose a reason for hiding this comment

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

Thanks for the suggestion! After investigation, I believe this is a false positive in our scenario.

When the user presses Escape to close the preview, open is set to false immediately, but focus is only restored to the trigger element in onVisibleChanged — which fires after the CSSMotion leave animation completes. Before that point, useLockFocus is still active and traps focus inside the preview, so the user cannot interact with the outer trigger to reopen the preview.

The actual sequence is:

  1. Escape → open = false (immediate)
  2. CSSMotion leave animation plays (focus still locked in preview)
  3. Animation ends → onVisibleChanged(false) → focus restored to trigger
  4. Only now can the user press Enter to reopen → open = true → useEffect captures document.activeElement, which is the trigger itself

So there's no window where document.activeElement could be an element inside the preview when open transitions to true.

}, [open]);

useLockFocus(open && !!wrapperEl, () => wrapperEl);

// ========================== Render ==========================
const bodyStyle: React.CSSProperties = {
Expand Down Expand Up @@ -423,7 +435,7 @@ const Preview: React.FC<PreviewProps> = props => {

return (
<div
ref={wrapperRef}
ref={setWrapperEl}
className={clsx(prefixCls, rootClassName, classNames.root, motionClassName, {
[`${prefixCls}-movable`]: movable,
[`${prefixCls}-moving`]: isMoving,
Expand Down
40 changes: 40 additions & 0 deletions tests/preview.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1256,4 +1256,44 @@ describe('Preview', () => {

expect(document.querySelector('.rc-image-preview')).toBeFalsy();
});

it('Focus should be trapped inside preview after keyboard open and restored on close', () => {
const rectSpy = jest.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockReturnValue({
x: 0, y: 0, width: 100, height: 100,
top: 0, right: 100, bottom: 100, left: 0,
toJSON: () => undefined,
} as DOMRect);

const { container } = render(<Image src="src" alt="focus trap" />);
const wrapper = container.querySelector('.rc-image') as HTMLElement;

// Open preview via keyboard
wrapper.focus();
expect(document.activeElement).toBe(wrapper);

fireEvent.keyDown(wrapper, { key: 'Enter' });
act(() => {
jest.runAllTimers();
});

// Focus should be inside the preview
const preview = document.querySelector('.rc-image-preview') as HTMLElement;
expect(preview).toBeTruthy();
expect(preview.contains(document.activeElement)).toBeTruthy();

// Focus should not escape when trying to focus outside
wrapper.focus();
expect(preview.contains(document.activeElement)).toBeTruthy();

// Close preview via Escape
fireEvent.keyDown(window, { key: 'Escape' });
act(() => {
jest.runAllTimers();
});

// Focus should return to the trigger element
expect(document.activeElement).toBe(wrapper);

rectSpy.mockRestore();
});
});
Loading