fix: lock focus inside preview when opened via keyboard#505
fix: lock focus inside preview when opened via keyboard#505zombieJ merged 2 commits 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. |
Walkthrough在 Preview 组件中新增 Changes
Sequence Diagram(s)sequenceDiagram
participant User as 用户
participant TriggerEl as 触发元素
participant Preview as Preview 组件
participant FocusLock as 焦点锁定 Hook
participant WrapperEl as Wrapper 元素
User->>TriggerEl: 按 Enter 键或激活触发器
TriggerEl->>Preview: 触发 open=true
Preview->>Preview: useLayoutEffect 捕获 document.activeElement -> triggerRef
Preview->>FocusLock: 激活焦点锁定 (open && portalRender)
FocusLock->>WrapperEl: 将焦点限制在 Wrapper 内
User->>TriggerEl: 尝试重新聚焦触发元素
FocusLock->>User: 阻止焦点离开 Wrapper
User->>Preview: 按 Escape 关闭预览
Preview->>Preview: onVisibleChanged(nextVisible=false)
Preview->>TriggerEl: 若可聚焦则 restore focus to triggerRef.current
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.
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 162 +1
=======================================
+ 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
src/Preview/index.tsx
Outdated
|
|
||
| // =========================== Focus ============================ | ||
| useLockFocus(open && portalRender, () => wrapperRef.current); | ||
| useEffect(() => { |
There was a problem hiding this comment.
改成 useLayouEffect，这样就不会强依赖于 useLockFocus 的先后顺序了。
src/Preview/index.tsx
Outdated
| const imgRef = useRef<HTMLImageElement>(); | ||
| const wrapperRef = useRef<HTMLDivElement>(null); | ||
| const triggerRef = useRef<HTMLElement>(null); | ||
| const [wrapperEl, setWrapperEl] = useState<HTMLDivElement>(null); |
There was a problem hiding this comment.
这里有点奇怪，ref 是正解。setState 是因为 useLockFocus 调用的时候拿不到 dom 吗？
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Preview/index.tsx (1)
394-400:⚠️ Potential issue | 🟠 Major焦点陷阱根因仍可能未完全修复（首次目标为
null的场景）Line 400 仍依赖
wrapperRef.current，当前实现没有在 wrapper 挂载时触发一次“节点就绪后的重计算”。若首次激活时CSSMotion仍返回null，useLockFocus可能继续错过锁定时机。建议把 wrapper 改为 callback ref + state，并把该 state 作为锁焦目标。可选修复示例
- const wrapperRef = useRef<HTMLDivElement>(null); + const [wrapperNode, setWrapperNode] = useState<HTMLDivElement | null>(null); - useLockFocus(open && portalRender, () => wrapperRef.current); + useLockFocus(open && portalRender, () => wrapperNode); ... - <div - ref={wrapperRef} + <div + ref={setWrapperNode} className={clsx(prefixCls, rootClassName, classNames.root, motionClassName, {#!/bin/bash # 只读核验：确认当前实现是否仍是 useRef + useLockFocus(wrapperRef.current) rg -n -C2 'const wrapperRef = useRef|useLockFocus\(|ref=\{wrapperRef\}|setWrapperNode' src/Preview/index.tsx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Preview/index.tsx` around lines 394 - 400, The focus-trap can miss the mount when wrapperRef.current is null — replace the ref-based wrapperRef with a callback ref + state (e.g., setWrapperNode / wrapperNode) and pass that state value into useLockFocus instead of wrapperRef.current so the hook re-runs when the node becomes available; update the element ref passed to the wrapper's JSX (currently using ref={wrapperRef}) to the callback ref, and keep triggerRef/useLayoutEffect logic as-is so first-active-element handling (triggerRef) still works when CSSMotion initially returns null.
🤖 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/index.tsx`:
- Around line 394-400: The focus-trap can miss the mount when wrapperRef.current
is null — replace the ref-based wrapperRef with a callback ref + state (e.g.,
setWrapperNode / wrapperNode) and pass that state value into useLockFocus
instead of wrapperRef.current so the hook re-runs when the node becomes
available; update the element ref passed to the wrapper's JSX (currently using
ref={wrapperRef}) to the callback ref, and keep triggerRef/useLayoutEffect logic
as-is so first-active-element handling (triggerRef) still works when CSSMotion
initially returns null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cbc4eced-27c0-4ff8-8994-d3abf76f589c
📒 Files selected for processing (2)
package.jsonsrc/Preview/index.tsx
✅ Files skipped from review due to trivial changes (1)
- package.json
3 participants
Screenshot
Before
After
CSSMotion 导致的渲染时序问题，useLockfoucs 执行时 wrapper dom 还没被挂载，但是该 hooks 的 deps 是［id,lock]，导致 wrapper dom 挂载后也不会重新触发，导致 focus trap 失效。
改成 state callback ref，re-render 一次让 useLockFocus 触发在 util 侧解了：https://github.com/react-component/util/pull/748/changes🤓 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
Summary by CodeRabbit
发布说明
Bug Fixes
Tests