Skip to content
Merged
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
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
"dependencies": {
"@rc-component/motion": "^1.0.0",
"@rc-component/portal": "^2.1.2",
"@rc-component/util": "^1.3.0",
"@rc-component/util": "^1.10.0",
"clsx": "^2.1.1"
},
"devDependencies": {
Expand Down
35 changes: 34 additions & 1 deletion src/Image.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export interface PreviewConfig extends Omit<InternalPreviewConfig, 'countRender'
export type SemanticName = 'root' | 'image' | 'cover';

export interface ImageProps
extends Omit<React.ImgHTMLAttributes<HTMLImageElement>, 'placeholder' | 'onClick'> {
extends Omit<React.ImgHTMLAttributes<HTMLImageElement>, 'placeholder' | 'onClick' | 'onKeyDown'> {
// Misc
prefixCls?: string;
previewPrefixCls?: string;
Expand Down Expand Up @@ -73,6 +73,7 @@ export interface ImageProps
// Events
onClick?: (e: React.MouseEvent<HTMLDivElement>) => void;
onError?: (e: React.SyntheticEvent<HTMLImageElement, Event>) => void;
onKeyDown?: (e: React.KeyboardEvent<HTMLDivElement>) => void;
}

interface CompoundedComponent<P> extends React.FC<P> {
Expand Down Expand Up @@ -108,6 +109,7 @@ const ImageInternal: CompoundedComponent<ImageProps> = props => {
// Events
onClick,
onError,
onKeyDown,
...otherProps
} = props;

Expand Down Expand Up @@ -203,6 +205,33 @@ const ImageInternal: CompoundedComponent<ImageProps> = props => {
onClick?.(e);
};

// ======================= Keyboard Preview =====================
const onPreviewKeyDown: React.KeyboardEventHandler<HTMLDivElement> = event => {
onKeyDown?.(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 +209 to +233
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 +241,10 @@ 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 : otherProps['aria-label']}
onKeyDown={onPreviewKeyDown}
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
18 changes: 12 additions & 6 deletions src/Preview/PrevNext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,24 +21,30 @@ export default function PrevNext(props: PrevNextProps) {

const switchCls = `${prefixCls}-switch`;

const prevDisabled = current === 0;
const nextDisabled = current === count - 1;

return (
<>
<div
<button
className={clsx(switchCls, `${switchCls}-prev`, {
[`${switchCls}-disabled`]: current === 0,
[`${switchCls}-disabled`]: prevDisabled,
})}
onClick={() => onActive(-1)}
disabled={prevDisabled}
>
{prev ?? left}
</div>
<div
</button>
<button
type="button"
className={clsx(switchCls, `${switchCls}-next`, {
[`${switchCls}-disabled`]: current === count - 1,
[`${switchCls}-disabled`]: nextDisabled,
})}
onClick={() => onActive(1)}
disabled={nextDisabled}
>
{next ?? right}
</div>
</button>
</>
);
}
10 changes: 10 additions & 0 deletions src/Preview/index.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import CSSMotion from '@rc-component/motion';
import Portal, { type PortalProps } from '@rc-component/portal';
import { useEvent } from '@rc-component/util';
import { useLockFocus } from '@rc-component/util/lib/Dom/focus';
import useLayoutEffect from '@rc-component/util/lib/hooks/useLayoutEffect';
import KeyCode from '@rc-component/util/lib/KeyCode';
import { clsx } from 'clsx';
Expand Down Expand Up @@ -195,6 +196,7 @@ const Preview: React.FC<PreviewProps> = props => {
} = props;

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

// =========================== Focus ============================
useLockFocus(open && portalRender, () => wrapperRef.current);

// ========================== Render ==========================
const bodyStyle: React.CSSProperties = {
...styles.body,
Expand Down Expand Up @@ -418,10 +423,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}
tabIndex={-1}
>
{/* Mask */}
<div
Expand Down
2 changes: 2 additions & 0 deletions tests/__snapshots__/basic.test.tsx.snap
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
exports[`Basic snapshot 1`] = `
<div
class="rc-image"
role="button"
style="width: 200px;"
tabindex="0"
>
<img
class="rc-image-img"
Expand Down
94 changes: 93 additions & 1 deletion tests/preview.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ import RotateLeftOutlined from '@ant-design/icons/RotateLeftOutlined';
import RotateRightOutlined from '@ant-design/icons/RotateRightOutlined';
import ZoomInOutlined from '@ant-design/icons/ZoomInOutlined';
import ZoomOutOutlined from '@ant-design/icons/ZoomOutOutlined';
import Dialog from '@rc-component/dialog';
import { spyElementPrototypes } from '@rc-component/util/lib/test/domHook';
import { act, createEvent, fireEvent, render } from '@testing-library/react';
import React from 'react';
import Dialog from '@rc-component/dialog';

jest.mock('../src/Preview', () => {
const MockPreview = (props: any) => {
Expand Down Expand Up @@ -1144,4 +1144,96 @@ 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 wrapper should be focusable after portal renders', () => {
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);

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

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

const preview = document.querySelector('.rc-image-preview') as HTMLElement;

expect(preview.contains(document.activeElement)).toBeTruthy();

rectSpy.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