Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
7 changes: 7 additions & 0 deletions assets/preview.less
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,12 @@
height: 40px;
color: #fff;
background: rgba(0, 0, 0, 0.3);
border: 0;
padding: 0;
border-radius: 9999px;
transform: translateY(-50%);
cursor: pointer;
font: inherit;

&-disabled {
cursor: default;
Expand Down Expand Up @@ -104,6 +107,10 @@
&-action {
color: #fff;
cursor: pointer;
border: 0;
padding: 0;
background: transparent;
font: inherit;

&-disabled {
cursor: default;
Expand Down
34 changes: 34 additions & 0 deletions src/Image.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,31 @@ const ImageInternal: CompoundedComponent<ImageProps> = props => {
onClick?.(e);
};

// ======================= Keyboard Preview =====================
const onPreviewKeyDown: React.KeyboardEventHandler<HTMLDivElement> = event => {
if (!canPreview) {
return;
}

if (event.key === 'Enter' || event.key === ' ') {
event.preventDefault();

const rect = (event.target as HTMLDivElement).getBoundingClientRect();
const left = rect.x + rect.width / 2;
const top = rect.y + rect.height / 2;

if (groupContext) {
groupContext.onPreview(imageId, src, left, top);
} else {
setMousePosition({
x: left,
y: top,
});
triggerPreviewOpen(true);
}
}
};
Comment on lines +207 to +229
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The use of event as any when calling onPreview from this keyboard handler conceals a type mismatch that could lead to runtime errors. The onPreview function is typed as a React.MouseEventHandler and is designed to handle mouse events. It also calls the component's onClick prop, which explicitly expects a React.MouseEvent.

Passing a KeyboardEvent from onPreviewKeyDown violates this contract. If a consumer of the Image component provides an onClick handler that relies on mouse-specific properties (e.g., clientX, button), their code will fail when the preview is triggered by a key press.

To fix this and maintain type safety, the logic for opening the preview should be handled within this function, ensuring that the onClick prop is not called for keyboard events. While this introduces some code duplication from onPreview, it correctly separates the concerns of click and keyboard interactions and prevents the bug.

  const onPreviewKeyDown: React.KeyboardEventHandler<HTMLDivElement> = event => {
    if (!canPreview) {
      return;
    }

    if (event.key === 'Enter' || event.key === ' ') {
      event.preventDefault();

      const rect = (event.target as HTMLDivElement).getBoundingClientRect();
      const left = rect.x + rect.width / 2;
      const top = rect.y + rect.height / 2;

      if (groupContext) {
        groupContext.onPreview(imageId, src, left, top);
      } else {
        setMousePosition({
          x: left,
          y: top,
        });
        triggerPreviewOpen(true);
      }
    }
  };


// =========================== Render ===========================
return (
<>
Expand All @@ -212,6 +237,15 @@ const ImageInternal: CompoundedComponent<ImageProps> = props => {
[`${prefixCls}-error`]: status === 'error',
})}
onClick={canPreview ? onPreview : onClick}
role={canPreview ? 'button' : otherProps.role}
tabIndex={canPreview && otherProps.tabIndex == null ? 0 : otherProps.tabIndex}
aria-label={
canPreview ? (otherProps['aria-label'] ?? alt ?? 'Preview image') : otherProps['aria-label']
}
onKeyDown={event => {
onPreviewKeyDown(event);
(otherProps as any).onKeyDown?.(event);
}}
style={{
width,
height,
Expand Down
9 changes: 6 additions & 3 deletions src/Preview/Footer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ interface RenderOperationParams {
icon: React.ReactNode;
type: OperationType;
disabled?: boolean;
onClick: (e: React.MouseEvent<HTMLDivElement, MouseEvent>) => void;
onClick: React.MouseEventHandler<HTMLButtonElement>;
}

export interface FooterProps extends Actions {
Expand Down Expand Up @@ -95,15 +95,18 @@ export default function Footer(props: FooterProps) {

const renderOperation = ({ type, disabled, onClick, icon }: RenderOperationParams) => {
return (
<div
<button
type="button"
key={type}
className={clsx(actionCls, `${actionCls}-${type}`, {
[`${actionCls}-disabled`]: !!disabled,
})}
onClick={onClick}
disabled={!!disabled}
aria-label={type}
>
{icon}
</div>
</button>
);
};

Expand Down
14 changes: 10 additions & 4 deletions src/Preview/PrevNext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,22 +23,28 @@ export default function PrevNext(props: PrevNextProps) {

return (
<>
<div
<button
type="button"
className={clsx(switchCls, `${switchCls}-prev`, {
[`${switchCls}-disabled`]: current === 0,
})}
onClick={() => onActive(-1)}
disabled={current === 0}
aria-label="Previous image"
>
{prev ?? left}
</div>
<div
</button>
<button
type="button"
className={clsx(switchCls, `${switchCls}-next`, {
[`${switchCls}-disabled`]: current === count - 1,
})}
onClick={() => onActive(1)}
disabled={current === count - 1}
aria-label="Next image"
>
{next ?? right}
</div>
</button>
</>
);
}
23 changes: 23 additions & 0 deletions src/Preview/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,8 @@ const Preview: React.FC<PreviewProps> = props => {
} = props;

const imgRef = useRef<HTMLImageElement>();
const wrapperRef = useRef<HTMLDivElement>(null);
const lastActiveRef = useRef<HTMLElement | null>(null);
const groupContext = useContext(PreviewGroupContext);
const showLeftOrRightSwitches = groupContext && count > 1;
const showOperationsProgress = groupContext && count >= 1;
Expand Down Expand Up @@ -382,6 +384,22 @@ const Preview: React.FC<PreviewProps> = props => {
}
};

// =========================== Focus ============================
useEffect(() => {
if (open) {
lastActiveRef.current = (document.activeElement as HTMLElement) || null;

// When `open` is initially true, the portal content is rendered in a later effect.
// Depend on `portalRender` so we can focus once the wrapper is actually mounted.
if (wrapperRef.current && portalRender) {
wrapperRef.current.focus();
}
} else if (!open && lastActiveRef.current) {
lastActiveRef.current.focus();
lastActiveRef.current = null;
}
}, [open, portalRender]);

// ========================== Render ==========================
const bodyStyle: React.CSSProperties = {
...styles.body,
Expand Down Expand Up @@ -418,10 +436,15 @@ const Preview: React.FC<PreviewProps> = props => {

return (
<div
ref={wrapperRef}
className={clsx(prefixCls, rootClassName, classNames.root, motionClassName, {
[`${prefixCls}-moving`]: isMoving,
})}
style={mergedStyle}
role="dialog"
aria-modal="true"
aria-label={alt || 'Image preview'}
tabIndex={-1}
>
{/* Mask */}
<div
Expand Down
3 changes: 3 additions & 0 deletions tests/__snapshots__/basic.test.tsx.snap
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@

exports[`Basic snapshot 1`] = `
<div
aria-label="Preview image"
class="rc-image"
role="button"
style="width: 200px;"
tabindex="0"
>
<img
class="rc-image-img"
Expand Down
79 changes: 79 additions & 0 deletions tests/preview.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1144,4 +1144,83 @@ describe('Preview', () => {
expect(baseElement.querySelector('.rc-image-preview')).toHaveClass(customClassnames.popup.root);
expect(baseElement.querySelector('.rc-image-preview')).toHaveStyle(customStyles.popup.root);
});

it('Image wrapper should be keyboard focusable when preview enabled', () => {
const { container } = render(<Image src="src" alt="keyboard test" />);

const wrapper = container.querySelector('.rc-image') as HTMLElement;
expect(wrapper).toHaveAttribute('role', 'button');
expect(wrapper).toHaveAttribute('tabindex', '0');
});

it('Pressing Enter on image wrapper should open preview', () => {
const { container } = render(<Image src="src" alt="keyboard open" />);

const wrapper = container.querySelector('.rc-image') as HTMLElement;
wrapper.focus();
fireEvent.keyDown(wrapper, { key: 'Enter' });

act(() => {
jest.runAllTimers();
});

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

it('Pressing Space on image wrapper should open preview', () => {
const { container } = render(<Image src="src" alt="keyboard open space" />);

const wrapper = container.querySelector('.rc-image') as HTMLElement;
wrapper.focus();
fireEvent.keyDown(wrapper, { key: ' ' });

act(() => {
jest.runAllTimers();
});

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

it('Preview dialog should have role dialog and receive focus', () => {
render(<Image src="src" alt="dialog a11y" preview={{ open: true }} />);

const preview = document.querySelector('.rc-image-preview') as HTMLElement;
expect(preview).toHaveAttribute('role', 'dialog');
expect(preview).toHaveAttribute('aria-modal', 'true');
expect(preview).toHaveAttribute('aria-label', 'dialog a11y');
});

it('Preview should focus wrapper after portal renders', () => {
const focusSpy = jest.spyOn(HTMLElement.prototype, 'focus');

render(<Image src="src" alt="focus portal" preview={{ open: true }} />);

act(() => {
jest.runAllTimers();
});

expect(focusSpy).toHaveBeenCalled();
focusSpy.mockRestore();
});

it('Preview open should render focusable wrapper', () => {
render(<Image src="src" alt="focus test" preview={{ open: true }} />);

const preview = document.querySelector('.rc-image-preview') as HTMLElement;
expect(preview).toHaveAttribute('tabindex', '-1');
});

it('Pressing Enter should not open preview when preview is disabled', () => {
const { container } = render(<Image src="src" alt="disabled preview" preview={false} />);

const wrapper = container.querySelector('.rc-image') as HTMLElement;
wrapper.focus();
fireEvent.keyDown(wrapper, { key: 'Enter' });

act(() => {
jest.runAllTimers();
});

expect(document.querySelector('.rc-image-preview')).toBeFalsy();
});
});
19 changes: 19 additions & 0 deletions tests/previewGroup.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,25 @@ describe('PreviewGroup', () => {
expect(document.querySelector('.rc-image-preview')).toBeFalsy();
});

it('Keyboard Enter should open preview from group image', () => {
const { container } = render(
<Image.PreviewGroup>
<Image src="src1" alt="first" />
<Image src="src2" alt="second" />
</Image.PreviewGroup>,
);

const first = container.querySelector('.rc-image') as HTMLElement;
first.focus();
fireEvent.keyDown(first, { key: 'Enter' });

act(() => {
jest.runAllTimers();
});

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

it('Preview with Custom Preview Property', () => {
const { container } = render(
<Image.PreviewGroup
Expand Down
Loading