feat: improve Image preview accessibility#496
Open
claytonlin1110 wants to merge 6 commits intoreact-component:masterfrom
Open
feat: improve Image preview accessibility#496claytonlin1110 wants to merge 6 commits intoreact-component:masterfrom
claytonlin1110 wants to merge 6 commits intoreact-component:masterfrom
Conversation
|
@claytonlin1110 is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the accessibility of the Image preview component, aligning its behavior with WCAG requirements. It ensures that keyboard users can seamlessly interact with image previews, from activating thumbnails to navigating and controlling the preview dialog, by implementing proper ARIA roles, focus management, and native button elements. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough为 Image/Preview 增加键盘触发与对话框无障碍语义、将若干控件改为原生按钮并在打开/关闭预览时管理焦点；新增对应无障碍测试。（不超过 50 字） Changes
Sequence Diagram(s)sequenceDiagram
participant User as 用户
participant Image as Image 组件
participant Preview as Preview 对话框
participant DOM as 浏览器焦点
User->>Image: 按 Enter/Space 或 点击
Image->>Image: 检查 canPreview 并 调用 onPreviewKeyDown/onClick
Image->>Preview: 打开预览（传入 origin/位置）
Image->>DOM: 保存当前聚焦元素到 lastActiveRef
Preview->>DOM: 聚焦 wrapperRef (tabIndex=-1)
Note right of Preview: role="dialog", aria-modal="true", aria-label
User->>Preview: 与控件交互（按钮为原生 button）
Preview->>DOM: 关闭时恢复 lastActiveRef 的焦点
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Author
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request significantly improves the accessibility of the Image component by making the preview feature fully keyboard-operable and adhering to ARIA standards for dialogs. The changes are well-executed, including converting interactive divs to buttons and properly managing focus. I've identified one high-severity issue related to type safety when triggering the preview via the keyboard, which could lead to runtime errors. Addressing this will make the implementation more robust.
Comment on lines
+207
to
+216
| const onPreviewKeyDown: React.KeyboardEventHandler<HTMLDivElement> = event => { | ||
| if (!canPreview) { | ||
| return; | ||
| } | ||
|
|
||
| if (event.key === 'Enter' || event.key === ' ') { | ||
| event.preventDefault(); | ||
| onPreview(event as any); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
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);
}
}
};
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Preview/Footer.tsx (1)
23-23:⚠️ Potential issue | 🟡 Minor
onClick类型与实际元素不匹配
RenderOperationParams接口中onClick的类型定义为React.MouseEvent<HTMLDivElement>, 但现在元素已改为<button>，应更新为HTMLButtonElement。🔧 建议修复
interface RenderOperationParams { icon: React.ReactNode; type: OperationType; disabled?: boolean; - onClick: (e: React.MouseEvent<HTMLDivElement, MouseEvent>) => void; + onClick: (e: React.MouseEvent<HTMLButtonElement, MouseEvent>) => void; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Preview/Footer.tsx` at line 23, RenderOperationParams 的 onClick 类型仍声明为 React.MouseEvent<HTMLDivElement, MouseEvent>，但该回调现在绑定到 <button> 元素；请将该属性类型改为 React.MouseEvent<HTMLButtonElement, MouseEvent>（或简写为 React.MouseEvent<HTMLButtonElement>），并更新相关使用处/测试中对 RenderOperationParams.onClick 的类型假设以匹配新元素类型。
🧹 Nitpick comments (4)
tests/preview.test.tsx (1)
1151-1163: 建议补充 Space 键测试
src/Image.tsx中的onPreviewKeyDown同时支持 Enter 和 Space 键打开预览，但测试只覆盖了 Enter 键。建议补充 Space 键的测试以确保完整覆盖。🧪 建议添加测试
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 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(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/preview.test.tsx` around lines 1151 - 1163, Add a new test alongside the existing Enter-key case in tests/preview.test.tsx that verifies Space opens the preview: locate the Image render and the wrapper element query ('.rc-image') used in the current test, then simulate focus and fireEvent.keyDown(wrapper, { key: ' ' }) or { key: 'Space' } to trigger the component's onPreviewKeyDown handler, run timers with act(() => jest.runAllTimers()), and assert that document.querySelector('.rc-image-preview') is truthy; mirror the existing test structure so both Enter and Space key paths in Image.onPreviewKeyDown are covered.src/Preview/Footer.tsx (1)
98-109: aria-label 值对屏幕阅读器用户不够友好当前
aria-label={type}会输出技术术语如"flipY"、"rotateLeft"，对于屏幕阅读器用户来说不够直观。建议使用更具描述性的标签。♻️ 建议改进
+const operationLabels: Record<OperationType, string> = { + prev: 'Previous image', + next: 'Next image', + flipY: 'Flip vertically', + flipX: 'Flip horizontally', + rotateLeft: 'Rotate left', + rotateRight: 'Rotate right', + zoomOut: 'Zoom out', + zoomIn: 'Zoom in', +}; + const renderOperation = ({ type, disabled, onClick, icon }: RenderOperationParams) => { return ( <button type="button" key={type} className={clsx(actionCls, `${actionCls}-${type}`, { [`${actionCls}-disabled`]: !!disabled, })} onClick={onClick} disabled={!!disabled} - aria-label={type} + aria-label={operationLabels[type]} > {icon} </button> ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Preview/Footer.tsx` around lines 98 - 109, The aria-label currently uses the raw `type` (e.g., "flipY", "rotateLeft") which is not friendly for screen readers; update the `Footer` button rendering to map `type` to a human-readable label (e.g., via a lookup object like actionLabelMap keyed by `type`) and use that mapped string in the `aria-label` instead of `type`; keep using the existing props/variables (`type`, `icon`, `onClick`, `actionCls`, `disabled`) and ensure a sensible default label (e.g., the `type` transformed or `"Action"`) if the map has no entry.src/Preview/index.tsx (1)
244-256: 焦点管理时机可能与动画冲突当前焦点管理在
open状态变化时立即执行，但CSSMotion可能仍在执行进入/离开动画。这可能导致：
- 打开时：焦点可能在元素完全可见前就被设置
- 关闭时：焦点在对话框消失前就被恢复
考虑将焦点恢复逻辑移至
onVisibleChanged回调中，以确保动画完成后再处理焦点：♻️ 建议改进
// =========================== Focus ============================ useEffect(() => { if (open) { lastActiveRef.current = (document.activeElement as HTMLElement) || null; if (wrapperRef.current) { wrapperRef.current.focus(); } - } else if (!open && lastActiveRef.current) { - lastActiveRef.current.focus(); - lastActiveRef.current = null; } }, [open]); // ... existing code ... const onVisibleChanged = (nextVisible: boolean) => { if (!nextVisible) { setLockScroll(false); + // Restore focus after animation completes + if (lastActiveRef.current) { + lastActiveRef.current.focus(); + lastActiveRef.current = null; + } } afterOpenChange?.(nextVisible); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Preview/index.tsx` around lines 244 - 256, The focus handling currently in the useEffect tied to open fires immediately and can clash with CSSMotion animations; move the restore-and-set-focus logic out of that useEffect and into the component's onVisibleChanged callback so focus is applied only after enter/leave animations finish: keep lastActiveRef and wrapperRef usage but trigger wrapperRef.current.focus() when onVisibleChanged(true) reports visible, and restore lastActiveRef.focus() when onVisibleChanged(false) reports not visible, removing the immediate focus/restore from the useEffect that watches open.src/Image.tsx (1)
206-216: 键盘事件类型转换需要注意Line 214 将
KeyboardEvent强制转换为any传递给onPreview，而onPreview期望接收MouseEvent<HTMLDivElement>。虽然两种事件的target属性都指向同一元素且getBoundingClientRect()可正常工作，但这种类型转换可能隐藏潜在问题。建议考虑提取公共逻辑或创建单独的键盘处理函数：
♻️ 可选的重构方案
+ const triggerPreview = (target: HTMLElement) => { + const rect = target.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); + } + }; + const onPreview: React.MouseEventHandler<HTMLDivElement> = e => { - const rect = (e.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); - } - + triggerPreview(e.currentTarget); onClick?.(e); }; const onPreviewKeyDown: React.KeyboardEventHandler<HTMLDivElement> = event => { if (!canPreview) { return; } if (event.key === 'Enter' || event.key === ' ') { event.preventDefault(); - onPreview(event as any); + triggerPreview(event.currentTarget); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Image.tsx` around lines 206 - 216, The keyboard handler onPreviewKeyDown currently casts the KeyboardEvent to any and passes it to onPreview (which expects MouseEvent<HTMLDivElement>); instead extract the shared preview behavior into a new helper (e.g., triggerPreviewFromElement or handlePreviewAction) that accepts the target element (HTMLDivElement) or its bounding rect, then have onPreview call that helper using its MouseEvent.currentTarget and have onPreviewKeyDown call the helper using event.currentTarget (or event.currentTarget as HTMLDivElement) when canPreview is true and Enter/Space is pressed; update onPreviewKeyDown to remove the unsafe (event as any) cast and reference canPreview, onPreview and the new helper accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/preview.test.tsx`:
- Around line 1165-1173: The focus assertion in the test "Preview dialog should
have role dialog and receive focus" can race with the Preview component's
useEffect focus logic; update the test in tests/preview.test.tsx to advance
timers before asserting focus (or use an async waitFor) so the focus set inside
src/Preview/index.tsx's useEffect has run—specifically, after rendering the
<Image ... preview={{ open: true }} /> call, invoke jest.runAllTimers() (or
await waitFor(() => expect(document.activeElement).toBe(preview))) before the
expect(document.activeElement).toBe(preview) assertion to ensure deterministic
focus timing.
---
Outside diff comments:
In `@src/Preview/Footer.tsx`:
- Line 23: RenderOperationParams 的 onClick 类型仍声明为
React.MouseEvent<HTMLDivElement, MouseEvent>，但该回调现在绑定到 <button> 元素；请将该属性类型改为
React.MouseEvent<HTMLButtonElement, MouseEvent>（或简写为
React.MouseEvent<HTMLButtonElement>），并更新相关使用处/测试中对 RenderOperationParams.onClick
的类型假设以匹配新元素类型。
---
Nitpick comments:
In `@src/Image.tsx`:
- Around line 206-216: The keyboard handler onPreviewKeyDown currently casts the
KeyboardEvent to any and passes it to onPreview (which expects
MouseEvent<HTMLDivElement>); instead extract the shared preview behavior into a
new helper (e.g., triggerPreviewFromElement or handlePreviewAction) that accepts
the target element (HTMLDivElement) or its bounding rect, then have onPreview
call that helper using its MouseEvent.currentTarget and have onPreviewKeyDown
call the helper using event.currentTarget (or event.currentTarget as
HTMLDivElement) when canPreview is true and Enter/Space is pressed; update
onPreviewKeyDown to remove the unsafe (event as any) cast and reference
canPreview, onPreview and the new helper accordingly.
In `@src/Preview/Footer.tsx`:
- Around line 98-109: The aria-label currently uses the raw `type` (e.g.,
"flipY", "rotateLeft") which is not friendly for screen readers; update the
`Footer` button rendering to map `type` to a human-readable label (e.g., via a
lookup object like actionLabelMap keyed by `type`) and use that mapped string in
the `aria-label` instead of `type`; keep using the existing props/variables
(`type`, `icon`, `onClick`, `actionCls`, `disabled`) and ensure a sensible
default label (e.g., the `type` transformed or `"Action"`) if the map has no
entry.
In `@src/Preview/index.tsx`:
- Around line 244-256: The focus handling currently in the useEffect tied to
open fires immediately and can clash with CSSMotion animations; move the
restore-and-set-focus logic out of that useEffect and into the component's
onVisibleChanged callback so focus is applied only after enter/leave animations
finish: keep lastActiveRef and wrapperRef usage but trigger
wrapperRef.current.focus() when onVisibleChanged(true) reports visible, and
restore lastActiveRef.focus() when onVisibleChanged(false) reports not visible,
removing the immediate focus/restore from the useEffect that watches open.
In `@tests/preview.test.tsx`:
- Around line 1151-1163: Add a new test alongside the existing Enter-key case in
tests/preview.test.tsx that verifies Space opens the preview: locate the Image
render and the wrapper element query ('.rc-image') used in the current test,
then simulate focus and fireEvent.keyDown(wrapper, { key: ' ' }) or { key:
'Space' } to trigger the component's onPreviewKeyDown handler, run timers with
act(() => jest.runAllTimers()), and assert that
document.querySelector('.rc-image-preview') is truthy; mirror the existing test
structure so both Enter and Space key paths in Image.onPreviewKeyDown are
covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31280583-fa4b-4e1b-bb85-8ed8fbb64df3
📒 Files selected for processing (5)
src/Image.tsxsrc/Preview/Footer.tsxsrc/Preview/PrevNext.tsxsrc/Preview/index.tsxtests/preview.test.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/preview.test.tsx (1)
1165-1172:⚠️ Potential issue | 🟠 Major测试名与断言不一致：缺少“receive focus”校验
Line 1165 的用例名声明“receive focus”，但 Line 1168-1172 只校验了 ARIA 属性，没有校验焦点是否落入预览对话框，核心行为未被测试保护。
建议修改
it('Preview dialog should have role dialog and receive focus', () => { render(<Image src="src" alt="dialog a11y" preview={{ open: true }} />); + act(() => { + jest.runAllTimers(); + }); + 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'); + expect(document.activeElement).toBe(preview); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/preview.test.tsx` around lines 1165 - 1172, The test name promises to verify the preview "receive focus" but only asserts ARIA attributes; update the case that renders Image (render(<Image src="src" alt="dialog a11y" preview={{ open: true }} />)) to also assert focus on the preview element (the queried .rc-image-preview stored in preview) — e.g. ensure the preview is the active element by using expect(preview).toHaveFocus() or assert document.activeElement === preview, and if needed waitFor or ensure the preview is focusable (tabindex) before asserting.
🧹 Nitpick comments (1)
tests/preview.test.tsx (1)
1151-1163: 建议补充 Space 键触发预览的测试Line 1151-1163 目前只覆盖 Enter，未覆盖需求中的 Space 键路径，建议补一条用例避免回归。
建议修改
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 by 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(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/preview.test.tsx` around lines 1151 - 1163, Add a new test case alongside the existing "Pressing Enter on image wrapper should open preview" that verifies pressing the Space key also opens the preview: render the Image component (Image), query and focus the wrapper element ('.rc-image'), dispatch a Space keydown via fireEvent.keyDown on the wrapper (use the same pattern as the Enter test), run timers with jest.runAllTimers() inside act(), and assert the preview appears via document.querySelector('.rc-image-preview'); name the test something like "Pressing Space on image wrapper should open preview" to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/preview.test.tsx`:
- Around line 1165-1172: The test name promises to verify the preview "receive
focus" but only asserts ARIA attributes; update the case that renders Image
(render(<Image src="src" alt="dialog a11y" preview={{ open: true }} />)) to also
assert focus on the preview element (the queried .rc-image-preview stored in
preview) — e.g. ensure the preview is the active element by using
expect(preview).toHaveFocus() or assert document.activeElement === preview, and
if needed waitFor or ensure the preview is focusable (tabindex) before
asserting.
---
Nitpick comments:
In `@tests/preview.test.tsx`:
- Around line 1151-1163: Add a new test case alongside the existing "Pressing
Enter on image wrapper should open preview" that verifies pressing the Space key
also opens the preview: render the Image component (Image), query and focus the
wrapper element ('.rc-image'), dispatch a Space keydown via fireEvent.keyDown on
the wrapper (use the same pattern as the Enter test), run timers with
jest.runAllTimers() inside act(), and assert the preview appears via
document.querySelector('.rc-image-preview'); name the test something like
"Pressing Space on image wrapper should open preview" to prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 79ba0b31-aa32-406a-ae02-400ec9811c3e
⛔ Files ignored due to path filters (1)
tests/__snapshots__/basic.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
tests/preview.test.tsx
Author
|
@afc163 Please review when you have a chance. Thanks. |
Author
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Preview/Footer.tsx`:
- Line 106: 当前 Footer.tsx 中将 aria-label 直接设为内部标识 aria-label={type}，会把诸如
"flipY"/"zoomOut" 读作不可理解的技术词，需改为人类可读文本并与 PrevNext.tsx 的 “Previous image / Next
image” 风格一致。修改方案：在 Footer 组件中为传入的 type（或 action）建立一个映射函数/对象（例如
getAriaLabelForAction 或 actionAriaLabels）将每个内部标识（flipY, zoomOut,
等）映射到清晰的可读字符串（可本地化），然后把 aria-label 设置为该映射结果；保持未识别值的回退字符串并更新相应测试/注释以反映可读标签。
ensure references: aria-label prop in Footer.tsx and the type variable, and
align wording with PrevNext.tsx labels.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04fdde69-f225-49c2-a515-7f9e378eb841
📒 Files selected for processing (1)
src/Preview/Footer.tsx
Author
|
@afc163 Would you please review this? Thanks. |
Author
|
@yoyo837 Any feedback for this feature PR? |
Member
|
CI failed |
Member
|
Great accessibility direction overall — keyboard activation for preview, dialog semantics, and switching interactive controls to semantic Non-CI review feedback:
Thanks again — this is a solid accessibility improvement overall. |
yoyo837
reviewed
Mar 17, 2026
src/Image.tsx
Outdated
| onClick={canPreview ? onPreview : onClick} | ||
| role={canPreview ? 'button' : otherProps.role} | ||
| tabIndex={canPreview && otherProps.tabIndex == null ? 0 : otherProps.tabIndex} | ||
| aria-label={canPreview ? (alt || 'Preview image') : otherProps['aria-label']} |
Member
There was a problem hiding this comment.
Nit: keep consumer-provided aria-label highest priority when preview is enabled.
Suggested change
| aria-label={canPreview ? (alt || 'Preview image') : otherProps['aria-label']} | |
| aria-label={canPreview ? (otherProps['aria-label'] ?? alt ?? 'Preview image') : otherProps['aria-label']} |
yoyo837
reviewed
Mar 17, 2026
src/Image.tsx
Outdated
| role={canPreview ? 'button' : otherProps.role} | ||
| tabIndex={canPreview && otherProps.tabIndex == null ? 0 : otherProps.tabIndex} | ||
| aria-label={canPreview ? (alt || 'Preview image') : otherProps['aria-label']} | ||
| onKeyDown={onPreviewKeyDown} |
Member
There was a problem hiding this comment.
Suggestion: preserve external onKeyDown handlers to avoid behavior regressions for consumers attaching keyboard logic on the wrapper.
Suggested change
| onKeyDown={onPreviewKeyDown} | |
| onKeyDown={event => { | |
| onPreviewKeyDown(event); | |
| otherProps.onKeyDown?.(event); | |
| }} |
Author
|
@yoyo837 Thanks for your feedback, I just updated and pushed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Image.tsx (1)
206-229: 建议提取重复的预览触发逻辑
onPreviewKeyDown中的位置计算和预览触发逻辑（lines 215-227）与onPreview中的逻辑（lines 189-200）高度重复。可以考虑提取一个共享的辅助函数来减少冗余。♻️ 可选重构建议
+ const triggerPreview = (target: HTMLElement) => { + const rect = target.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); + } + }; + const onPreview: React.MouseEventHandler<HTMLDivElement> = e => { - const rect = (e.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); - } - + triggerPreview(e.target as HTMLDivElement); onClick?.(e); }; 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); - } + triggerPreview(event.target as HTMLDivElement); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Image.tsx` around lines 206 - 229, The keydown handler duplicates the center-position calculation and preview-trigger logic found in onPreview; extract a shared helper (e.g., computeCenterAndTriggerPreview) that accepts an element or rect plus imageId/src and performs the rect center calc and the two trigger paths (calling groupContext.onPreview(...) when groupContext exists, otherwise calling setMousePosition(...) and triggerPreviewOpen(true)); replace the duplicated blocks in onPreviewKeyDown and onPreview to call that helper so the logic is centralized and DRY while still using existing symbols (onPreviewKeyDown, onPreview, groupContext, groupContext.onPreview, setMousePosition, triggerPreviewOpen, imageId, src).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Image.tsx`:
- Around line 206-229: The keydown handler duplicates the center-position
calculation and preview-trigger logic found in onPreview; extract a shared
helper (e.g., computeCenterAndTriggerPreview) that accepts an element or rect
plus imageId/src and performs the rect center calc and the two trigger paths
(calling groupContext.onPreview(...) when groupContext exists, otherwise calling
setMousePosition(...) and triggerPreviewOpen(true)); replace the duplicated
blocks in onPreviewKeyDown and onPreview to call that helper so the logic is
centralized and DRY while still using existing symbols (onPreviewKeyDown,
onPreview, groupContext, groupContext.onPreview, setMousePosition,
triggerPreviewOpen, imageId, src).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41dbc003-8d4e-4a54-9361-c5caed9c83e5
📒 Files selected for processing (3)
assets/preview.lesssrc/Image.tsxtests/preview.test.tsx
Member
|
@coderabbitai Review it again please. |
Author
|
@yoyo837 About test issue, i could reproduce the same issue on master branch as well. |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/preview.test.tsx (1)
1179-1186:⚠️ Potential issue | 🟡 Minor用例名与断言不一致：未验证“receive focus”。
当前只校验了
role/aria，没有校验焦点是否落入对话框。建议要么重命名用例，要么在 Line 1182 之后补充对document.activeElement的断言（必要时先推进 timers）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/preview.test.tsx` around lines 1179 - 1186, The test "Preview dialog should have role dialog and receive focus" only asserts role/aria attributes on the rendered Image preview (variable preview) but doesn't verify focus; either rename the test to remove "receive focus" or add an assertion that document.activeElement equals the preview element (or that preview.contains(document.activeElement)) after render; if focus is set asynchronously, run pending timers (e.g., via jest.runAllTimers or jest.advanceTimersByTime) before checking document.activeElement to ensure the focus change has occurred.src/Preview/Footer.tsx (1)
98-107:⚠️ Potential issue | 🟠 Major
aria-label不应直接使用内部操作码。Line 106 现在会读出
flipY/zoomOut这类技术词，不利于读屏理解。建议映射为可读文本，并保持与 PrevNext 文案风格一致。建议修复
+ const operationAriaLabelMap: Record<OperationType, string> = { + prev: 'Previous image', + next: 'Next image', + flipY: 'Flip vertically', + flipX: 'Flip horizontally', + rotateLeft: 'Rotate left', + rotateRight: 'Rotate right', + zoomOut: 'Zoom out', + zoomIn: 'Zoom in', + }; + const renderOperation = ({ type, disabled, onClick, icon }: RenderOperationParams) => { return ( <button @@ - aria-label={type} + aria-label={operationAriaLabelMap[type] ?? 'Image action'} > {icon} </button> ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Preview/Footer.tsx` around lines 98 - 107, The aria-label currently uses the internal action code variable type (e.g., "flipY" or "zoomOut") which is unreadable to screen readers; change the button's aria-label to a human-readable mapping instead (e.g., map "flipY" -> "Flip vertically", "zoomOut" -> "Zoom out") by adding a small mapping object or helper (e.g., ARIA_LABELS or getAriaLabel) and using ARIA_LABELS[type] || type for the aria-label in the button inside Footer.tsx so the label matches the PrevNext copy style while preserving existing props like onClick, disabled, and className (actionCls and `${actionCls}-${type}`).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Preview/index.tsx`:
- Around line 245-257: Current open/close effect (useEffect watching open) only
moves focus in/out via lastActiveRef and wrapperRef but does not trap Tab, so
keyboard users can tab out of the modal; update the Preview component to add a
keydown handler attached to wrapperRef when open that intercepts Tab/Shift+Tab,
queries focusable elements within wrapperRef (use standard focusable selector:
a, button, input, select, textarea, [tabindex]:not([tabindex="-1"]) and
:not([disabled])), and when Tab would move past the last element move focus to
the first (and when Shift+Tab would move before the first move focus to the
last); ensure the handler is registered on open and removed on close and that
wrapperRef.current is focused as before (keep lastActiveRef behavior intact).
---
Duplicate comments:
In `@src/Preview/Footer.tsx`:
- Around line 98-107: The aria-label currently uses the internal action code
variable type (e.g., "flipY" or "zoomOut") which is unreadable to screen
readers; change the button's aria-label to a human-readable mapping instead
(e.g., map "flipY" -> "Flip vertically", "zoomOut" -> "Zoom out") by adding a
small mapping object or helper (e.g., ARIA_LABELS or getAriaLabel) and using
ARIA_LABELS[type] || type for the aria-label in the button inside Footer.tsx so
the label matches the PrevNext copy style while preserving existing props like
onClick, disabled, and className (actionCls and `${actionCls}-${type}`).
In `@tests/preview.test.tsx`:
- Around line 1179-1186: The test "Preview dialog should have role dialog and
receive focus" only asserts role/aria attributes on the rendered Image preview
(variable preview) but doesn't verify focus; either rename the test to remove
"receive focus" or add an assertion that document.activeElement equals the
preview element (or that preview.contains(document.activeElement)) after render;
if focus is set asynchronously, run pending timers (e.g., via jest.runAllTimers
or jest.advanceTimersByTime) before checking document.activeElement to ensure
the focus change has occurred.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b582e2a-92dd-4fe8-8e89-d02994682b40
⛔ Files ignored due to path filters (1)
tests/__snapshots__/basic.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
assets/preview.lesssrc/Image.tsxsrc/Preview/Footer.tsxsrc/Preview/PrevNext.tsxsrc/Preview/index.tsxtests/preview.test.tsx
src/Preview/index.tsx
Outdated
Comment on lines
+245
to
+257
| useEffect(() => { | ||
| if (open) { | ||
| lastActiveRef.current = (document.activeElement as HTMLElement) || null; | ||
|
|
||
| if (wrapperRef.current) { | ||
| wrapperRef.current.focus(); | ||
| } | ||
| } else if (!open && lastActiveRef.current) { | ||
| lastActiveRef.current.focus(); | ||
| lastActiveRef.current = null; | ||
| } | ||
| }, [open]); | ||
|
|
There was a problem hiding this comment.
对话框缺少焦点陷阱，Tab 仍可逃逸到弹层外。
目前实现了“打开聚焦 + 关闭恢复”，但没有 Tab 循环约束；这会让键盘用户离开 modal 语境。建议在预览容器上补充 Tab/Shift+Tab 循环处理。
建议方向（最小实现）
+ const onDialogKeyDown: React.KeyboardEventHandler<HTMLDivElement> = event => {
+ if (event.key !== 'Tab' || !wrapperRef.current) return;
+ const focusables = wrapperRef.current.querySelectorAll<HTMLElement>(
+ 'button:not([disabled]), [href], input:not([disabled]), select:not([disabled]), textarea:not([disabled]), [tabindex]:not([tabindex="-1"])',
+ );
+ if (!focusables.length) return;
+ const first = focusables[0];
+ const last = focusables[focusables.length - 1];
+ const active = document.activeElement as HTMLElement | null;
+ if (!event.shiftKey && active === last) {
+ event.preventDefault();
+ first.focus();
+ } else if (event.shiftKey && active === first) {
+ event.preventDefault();
+ last.focus();
+ }
+ };
@@
- <div
+ <div
ref={wrapperRef}
@@
tabIndex={-1}
+ onKeyDown={onDialogKeyDown}
>Also applies to: 442-446
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Preview/index.tsx` around lines 245 - 257, Current open/close effect
(useEffect watching open) only moves focus in/out via lastActiveRef and
wrapperRef but does not trap Tab, so keyboard users can tab out of the modal;
update the Preview component to add a keydown handler attached to wrapperRef
when open that intercepts Tab/Shift+Tab, queries focusable elements within
wrapperRef (use standard focusable selector: a, button, input, select, textarea,
[tabindex]:not([tabindex="-1"]) and :not([disabled])), and when Tab would move
past the last element move focus to the first (and when Shift+Tab would move
before the first move focus to the last); ensure the handler is registered on
open and removed on close and that wrapperRef.current is focused as before (keep
lastActiveRef behavior intact).
Author
|
@yoyo837 Please review |
Member
Let me fix that on master before merge this. |
Member
|
@aojunhao123 Please help to check this. |
Author
|
@zombieJ Please check |
Member
|
/rebase |
claytonlin1110
force-pushed
the
feat/image-preview-a11y
branch
from
824ecce to
46c5259
Compare
Author
|
@yoyo837 rebased |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Preview/Footer.tsx (1)
96-107:⚠️ Potential issue | 🟠 Major
aria-label不应直接使用内部标识。Line 106 当前会读出
flipY/zoomOut这类技术标识，读屏可理解性较差，建议映射为可读文案并保留兜底。建议修复
+const operationAriaLabelMap: Record<OperationType, string> = { + prev: 'Previous image', + next: 'Next image', + flipY: 'Flip vertically', + flipX: 'Flip horizontally', + rotateLeft: 'Rotate left', + rotateRight: 'Rotate right', + zoomOut: 'Zoom out', + zoomIn: 'Zoom in', +}; + const renderOperation = ({ type, disabled, onClick, icon }: RenderOperationParams) => { return ( <button type="button" key={type} @@ - aria-label={type} + aria-label={operationAriaLabelMap[type] ?? 'Image action'} > {icon} </button> ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Preview/Footer.tsx` around lines 96 - 107, The aria-label in renderOperation currently uses the raw internal operation type (e.g., "flipY", "zoomOut"), which is unfriendly for screen readers; change renderOperation to map the incoming type (RenderOperationParams.type) to a human-readable label via a lookup (e.g., an operationLabelMap or a getOperationLabel function) and use that string for aria-label, keeping a safe fallback to the raw type when no mapping exists; update any tests or usages that assume the old value and ensure the mapping covers all known operation types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Preview/Footer.tsx`:
- Around line 96-107: The aria-label in renderOperation currently uses the raw
internal operation type (e.g., "flipY", "zoomOut"), which is unfriendly for
screen readers; change renderOperation to map the incoming type
(RenderOperationParams.type) to a human-readable label via a lookup (e.g., an
operationLabelMap or a getOperationLabel function) and use that string for
aria-label, keeping a safe fallback to the raw type when no mapping exists;
update any tests or usages that assume the old value and ensure the mapping
covers all known operation types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55ffa002-6c6f-4310-a5cc-c3f653ebd48a
⛔ Files ignored due to path filters (1)
tests/__snapshots__/basic.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
assets/preview.lesssrc/Image.tsxsrc/Preview/Footer.tsxsrc/Preview/PrevNext.tsxsrc/Preview/index.tsxtests/preview.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Preview/PrevNext.tsx
- tests/preview.test.tsx
- src/Preview/index.tsx
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #496 +/- ##
==========================================
+ Coverage 99.41% 99.44% +0.03%
==========================================
Files 17 17
Lines 511 541 +30
Branches 153 165 +12
==========================================
+ Hits 508 538 +30
Misses 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Author
|
@yoyo837 please re-run CI |
Author
|
@yoyo837 please run CI again |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
previewis enabled (role="button", tab focus, Enter/Space to open).role="dialog",aria-modal="true", and managed focus.<button>elements to support keyboard focus and interaction.Motivation
This PR aligns the
Imagepreview behavior with WCAG requirements by ensuring keyboard users can:It also serves as the underlying change required for Ant Design’s
Imagecomponent accessibility improvement requested in ant-design/ant-design#57254.Breaking Changes
divtobuttonbut classNames are preserved, so styles should remain compatible.Related
Closes ant-design/ant-design#57254
Summary by CodeRabbit
发布说明
新功能
无障碍改进
样式
测试