fix: lock focus inside preview when opened via keyboard#505
fix: lock focus inside preview when opened via keyboard#505aojunhao123 wants to merge 1 commit intoreact-component:masterfrom
Conversation
…eyboard The preview wrapper DOM element is not immediately available due to CSSMotion's deferred rendering (styleReady='NONE' on first render). Using useRef meant useLockFocus could never re-evaluate after the DOM appeared. Switch to useState callback ref so the component re-renders once the wrapper mounts, allowing useLockFocus to activate correctly. Also restores focus to the trigger element after the preview closes.
|
@aojunhao123 is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
概览Preview 组件的焦点管理逻辑进行了重构。用状态管理的 变更
序列图sequenceDiagram
participant User as 用户
participant Preview as Preview 组件
participant TriggerEl as Trigger 元素
participant FocusLock as 焦点锁定 Hook
participant WrapperEl as Wrapper 元素
User->>TriggerEl: 按 Enter 键打开预览
TriggerEl->>Preview: 触发打开事件 (open=true)
Preview->>Preview: 捕获 document.activeElement 到 triggerRef
Preview->>FocusLock: 执行 (open && !!wrapperEl)
FocusLock->>WrapperEl: 在 wrapper 内锁定焦点
User->>Preview: 尝试聚焦 trigger 元素
FocusLock->>FocusLock: 焦点困禁，保持在 wrapper 内
User->>Preview: 按 Escape 关闭预览
Preview->>Preview: onVisibleChanged(nextVisible=false)
Preview->>TriggerEl: 恢复焦点到 triggerRef
Preview->>Preview: 清空 triggerRef
预计代码审查工作量🎯 3 (Moderate) | ⏱️ ~20 分钟 相关联的 PR
建议审查者
诗歌
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements focus management for the Preview component, ensuring that focus is captured when the preview opens and restored to the original trigger element upon closing. It also includes a new test case to verify focus trapping and restoration behavior. Feedback focuses on making the focus restoration more robust by checking if the trigger element is still in the document and using the preventScroll option, as well as refining the logic for capturing the trigger element to avoid overwriting it with elements from within the preview itself.
| triggerRef.current?.focus?.(); | ||
| triggerRef.current = null; |
There was a problem hiding this comment.
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;
| if (open) { | ||
| triggerRef.current = document.activeElement as HTMLElement; | ||
| } |
There was a problem hiding this comment.
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.
| if (open) { | |
| triggerRef.current = document.activeElement as HTMLElement; | |
| } | |
| if (open && !wrapperEl?.contains(document.activeElement)) { | |
| triggerRef.current = document.activeElement as HTMLElement; | |
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #505 +/- ##
=======================================
Coverage 99.43% 99.44%
=======================================
Files 17 17
Lines 532 538 +6
Branches 161 161
=======================================
+ Hits 529 535 +6
Misses 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Preview/index.tsx (1)
200-200: TypeScript 类型定义不完整
useState<HTMLDivElement>(null)的泛型参数应包含null，否则在严格模式下会有类型错误。建议修复
- const [wrapperEl, setWrapperEl] = useState<HTMLDivElement>(null); + const [wrapperEl, setWrapperEl] = useState<HTMLDivElement | null>(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Preview/index.tsx` at line 200, The state declaration for wrapperEl uses useState<HTMLDivElement>(null) which omits null in the generic and causes type errors in strict mode; update the useState generic to include null (e.g., useState<HTMLDivElement | null>(null)) so wrapperEl and setWrapperEl have the correct nullable type and TypeScript no longer complains.
🤖 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/Preview/index.tsx`:
- Line 200: The state declaration for wrapperEl uses
useState<HTMLDivElement>(null) which omits null in the generic and causes type
errors in strict mode; update the useState generic to include null (e.g.,
useState<HTMLDivElement | null>(null)) so wrapperEl and setWrapperEl have the
correct nullable type and TypeScript no longer complains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7bc3dd1b-f133-42cf-a4f8-449bf22035e9
📒 Files selected for processing (2)
src/Preview/index.tsxtests/preview.test.tsx
2 participants
Screenshot
Before
After
CSSMotion 导致的渲染时序问题，useLockfoucs 执行时 wrapper dom 还没被挂载，但是该 hooks 的 deps 是［id,lock]，导致 wrapper dom 挂载后也不会重新触发，导致 focus trap 失效。
改成 state callback ref，re-render 一次让 useLockFocus 触发
🤓 Generated with 我自己
Summary
nullon first render (styleReady='NONE'during appear phase), so the wrapper DOM element is not available whenuseLockFocusfirst activates. SwitchedwrapperReffromuseReftouseStatecallback ref so the component re-renders once the wrapper mounts, allowinguseLockFocusto correctly lock focus.document.activeElementas the trigger whenopenbecomestrue, then callstriggerRef.current.focus()inonVisibleChangedafter the leave animation completes.Test plan
🤖 Generated with Claude Code