feat: Support useLockFocus #712
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough新增焦点锁定实现：引入 Changes
序列图sequenceDiagram
participant User as 用户
participant Component as React 组件 (useLockFocus)
participant FocusMgr as 焦点管理 (lockFocus)
participant DOM as 浏览器 DOM
User->>Component: 设置 lock = true
activate Component
Component->>FocusMgr: 调用 lockFocus(element)
activate FocusMgr
FocusMgr->>DOM: 注册 focusin 和 keydown 监听器
User->>DOM: 按 Tab 或 点击元素
DOM->>FocusMgr: 触发 focusin / keydown 事件
alt 焦点落在锁定容器内
FocusMgr->>DOM: 允许焦点移动（不干预）
else 尝试移到容器外
FocusMgr->>DOM: 阻止/重置焦点到已定义元素
end
User->>Component: 设置 lock = false
Component->>FocusMgr: 注销监听器并移除锁定
deactivate FocusMgr
deactivate Component
代码审查工作量评估🎯 4 (复杂) | ⏱️ ~30–90 分钟 需要额外关注的区域：
诗
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
Summary of ChangesHello @zombieJ, 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 introduces a robust and modern solution for managing focus within specific UI elements, preventing focus from inadvertently leaving a designated area. This enhancement significantly improves keyboard accessibility for interactive components by ensuring that tab navigation and direct focus attempts are contained, providing a more predictable and user-friendly experience. Highlights
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
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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 introduces a new useLockFocus hook and its underlying lockFocus utility to manage focus within a specific DOM element, preventing users from tabbing out of it. It also includes a demo and tests for this new functionality. The changes involve removing deprecated focus-related functions and adding the new focus lock logic. While the core functionality seems to work as intended, there are some concerns regarding global state management and potential runtime errors that should be addressed for robustness and maintainability.
| lock: boolean, | ||
| getElement: () => HTMLElement | null, | ||
| ) { | ||
| useEffect(() => { |
There was a problem hiding this comment.
The lockFocus function expects an HTMLElement as its argument, but getElement() can return null. If getElement() returns null, this will lead to a runtime error when lockFocus is called. A null check should be performed before attempting to call lockFocus.
if (lock) {
const element = getElement();
if (element) {
return lockFocus(element);
}
}| let lastFocusElement: HTMLElement | null = null; | ||
| let focusElements: HTMLElement[] = []; |
There was a problem hiding this comment.
The lastFocusElement and focusElements variables are global mutable states. The current implementation of getLastElement() always returns the most recently added element to focusElements, and syncFocus and onWindowKeyDown exclusively operate on this single "last" element. This design implies that only one focus lock can be truly active at any given time, with newer locks overriding older ones. If the intention is to support multiple independent focus locks simultaneously (e.g., for nested modals or multiple active components), this global state management will lead to incorrect and unpredictable behavior. Consider encapsulating the state for each lockFocus instance or redesigning the global state to properly manage multiple active locks, perhaps by iterating through focusElements or using a more robust stack/queue approach that respects the order and context of active locks.
src/Dom/focus.ts
Outdated
| window.addEventListener('focusin', syncFocus); | ||
| window.addEventListener('keydown', onWindowKeyDown, true); |
There was a problem hiding this comment.
The window.addEventListener calls are executed every time lockFocus is invoked. While the cleanup logic correctly removes listeners when no elements are locked, repeatedly calling addEventListener for the same function reference on the same target (window) will register the listener multiple times. This can lead to syncFocus and onWindowKeyDown being executed redundantly for a single event, potentially impacting performance or causing unexpected behavior. These listeners should ideally only be added once when the first element is locked and removed when the last element is unlocked.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
docs/examples/focus.tsx (1)
1-8: 建议统一 React hooks 的导入风格。
useRef使用了解构导入，但useState使用的是React.useState。建议保持一致的导入风格。-import React, { useRef } from 'react'; +import React, { useRef, useState } from 'react';然后将第 8 行改为：
- const [locking, setLocking] = React.useState(true); + const [locking, setLocking] = useState(true);tests/focus.test.tsx (1)
62-98: 测试覆盖了基本场景，建议补充更多测试用例。当前测试验证了锁定状态下焦点会被限制在容器内，这是核心功能。建议后续补充以下测试场景：
- 解锁后焦点可以正常移出容器
- Tab 键循环导航行为
- Shift+Tab 反向循环导航
- 多个嵌套 focus lock 的行为
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/demo/focus.md(1 hunks)docs/examples/focus.tsx(1 hunks)src/Dom/focus.ts(2 hunks)tests/focus.test.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Dom/focus.ts (2)
src/Dom/isVisible.ts (1)
element(1-27)src/hooks/useEffect.ts (1)
useEffect(4-18)
tests/focus.test.tsx (1)
src/Dom/focus.ts (1)
useLockFocus(173-182)
docs/examples/focus.tsx (1)
src/Dom/focus.ts (1)
useLockFocus(173-182)
🪛 GitHub Actions: ✅ test
src/Dom/focus.ts
[warning] 181-181: React Hook useEffect has a missing dependency: 'getElement'. Either include it or remove the dependency array.
tests/focus.test.tsx
[error] 2-2: JSX is defined but never used. (@typescript-eslint/no-unused-vars)
[error] 3-3: 'cleanup' is defined but never used. (no-unused-vars)
🪛 GitHub Check: CodeQL
tests/focus.test.tsx
[notice] 2-2: Unused variable, import, function or class
Unused import JSX.
[notice] 3-3: Unused variable, import, function or class
Unused import cleanup.
🔇 Additional comments (2)
docs/demo/focus.md (1)
1-9: 文档结构清晰。文档格式正确，正确引用了示例文件。
src/Dom/focus.ts (1)
103-104: 模块级可变状态可能在 SSR 或多实例场景下产生问题。
lastFocusElement和focusElements作为模块级变量，在服务端渲染（SSR）环境或同时存在多个 focus lock 实例时可能产生状态污染。当前实现适用于客户端单实例场景，但如果后续需要支持 SSR 或更复杂的场景，建议考虑使用 Context 或其他隔离方案管理状态。
| export function useLockFocus( | ||
| lock: boolean, | ||
| getElement: () => HTMLElement | null, | ||
| ) { | ||
| useEffect(() => { | ||
| if (lock) { | ||
| return lockFocus(getElement()); | ||
| } | ||
| }, [lock]); |
There was a problem hiding this comment.
修复 useEffect 缺少依赖项的问题。
Pipeline 报告 useEffect 缺少 getElement 依赖。虽然 getElement 通常是稳定的回调函数，但为了符合 React hooks 规范并消除警告，建议添加到依赖数组中。
export function useLockFocus(
lock: boolean,
getElement: () => HTMLElement | null,
) {
useEffect(() => {
if (lock) {
return lockFocus(getElement());
}
- }, [lock]);
+ }, [lock, getElement]);
}如果担心 getElement 引用不稳定导致不必要的重新执行，可以考虑使用 useCallback 包装调用方的 getter，或在此处使用 ref 存储 getter。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function useLockFocus( | |
| lock: boolean, | |
| getElement: () => HTMLElement | null, | |
| ) { | |
| useEffect(() => { | |
| if (lock) { | |
| return lockFocus(getElement()); | |
| } | |
| }, [lock]); | |
| export function useLockFocus( | |
| lock: boolean, | |
| getElement: () => HTMLElement | null, | |
| ) { | |
| useEffect(() => { | |
| if (lock) { | |
| return lockFocus(getElement()); | |
| } | |
| }, [lock, getElement]); | |
| } |
🧰 Tools
🪛 GitHub Actions: ✅ test
[warning] 181-181: React Hook useEffect has a missing dependency: 'getElement'. Either include it or remove the dependency array.
🤖 Prompt for AI Agents
In src/Dom/focus.ts around lines 173 to 181, the useEffect hook is missing
getElement in its dependency array which triggers the React hooks warning;
update the dependencies to include getElement (i.e. [lock, getElement]) so the
effect responds correctly to changes, and if caller-side instability is a
concern, advise wrapping the getter with useCallback or storing it in a ref to
stabilize its identity.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #712 +/- ##
==========================================
+ Coverage 90.53% 91.57% +1.03%
==========================================
Files 37 37
Lines 909 938 +29
Branches 306 306
==========================================
+ Hits 823 859 +36
+ Misses 84 77 -7
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Dom/focus.ts (1)
176-188:useEffect缺少getElement依赖项。此问题在之前的审查中已被指出但尚未修复。虽然
getElement通常是稳定的回调，但为符合 React Hooks 规范并消除 lint 警告，应将其添加到依赖数组中。useEffect(() => { if (lock) { const element = getElement(); if (element) { return lockFocus(element); } } - }, [lock]); + }, [lock, getElement]); }如果担心调用方传入的
getElement引用不稳定导致不必要的重新执行，可以在调用方使用useCallback包装，或在此处使用useRef存储 getter。
🧹 Nitpick comments (3)
tests/focus.test.tsx (1)
62-98: 测试覆盖了基本场景，建议补充边界情况。当前测试验证了焦点锁定的核心行为，但建议补充以下测试用例：
lock=false时焦点不应被锁定- 组件卸载时清理函数是否正确执行
lock从true切换到false后焦点行为示例测试用例：
it('should not lock focus when lock is false', () => { const { getByTestId } = render(<TestComponent lock={false} />); const outerButton = getByTestId('outer-button') as HTMLButtonElement; outerButton.focus(); expect(document.activeElement).toBe(outerButton); });src/Dom/focus.ts (2)
103-108: 全局状态设计支持嵌套场景，但需注意限制。当前设计使用栈结构管理多个焦点锁定元素，但
syncFocus和onWindowKeyDown仅操作栈顶元素。这意味着嵌套的焦点锁定（如嵌套模态框）会正确工作，但同级的多个焦点锁定会相互覆盖。如果这是预期行为，建议在 JSDoc 中说明此限制。
132-147: 建议添加防御性检查。虽然事件监听器仅在
focusElements非空时注册，但存在边界情况：如果在事件触发过程中执行了清理，getLastElement()可能返回undefined，导致getFocusNodeList接收到无效参数。function onWindowKeyDown(e: KeyboardEvent) { if (e.key === 'Tab') { const { activeElement } = document; const lastElement = getLastElement(); + if (!lastElement) { + return; + } const focusableList = getFocusNodeList(lastElement);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/examples/focus.tsx(1 hunks)src/Dom/focus.ts(2 hunks)tests/focus.test.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/examples/focus.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/Dom/focus.ts (2)
src/Dom/isVisible.ts (1)
element(1-27)src/hooks/useEffect.ts (1)
useEffect(4-18)
tests/focus.test.tsx (1)
src/Dom/focus.ts (1)
useLockFocus(176-188)
🔇 Additional comments (3)
tests/focus.test.tsx (1)
2-5: 导入修复看起来正确。之前的审查中标记的未使用导入 (
JSX和cleanup) 已被移除，新增了useLockFocus的导入。src/Dom/focus.ts (2)
115-130:syncFocus实现逻辑正确。焦点同步逻辑会在焦点离开锁定区域时，优先恢复到上次聚焦的元素，否则聚焦第一个可聚焦元素。可选链操作符的使用也处理了空列表的边界情况。
153-174:lockFocus实现已正确处理 null 参数。Line 154 的
if (element)检查解决了之前提出的 null 参数问题。事件监听器的处理也是正确的——addEventListener对相同的函数引用、事件类型和捕获标志组合会自动去重。
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/Dom/focus.ts (2)
160-161: 重复注册事件监听器导致性能问题。之前的审查已经指出，每次调用
lockFocus时都会执行window.addEventListener。虽然清理逻辑在没有元素锁定时会正确移除监听器，但对同一目标（window）重复调用addEventListener会多次注册同一个监听器。这会导致syncFocus和onWindowKeyDown在单个事件中被冗余执行多次，可能影响性能或导致意外行为。理想情况下，这些监听器应该只在第一个元素被锁定时添加一次，在最后一个元素解锁时移除。
+let listenersAttached = false; + export function lockFocus(element: HTMLElement): VoidFunction { if (element) { // Refresh focus elements focusElements = focusElements.filter(ele => ele !== element); focusElements.push(element); - // Just add event since it will de-duplicate - window.addEventListener('focusin', syncFocus); - window.addEventListener('keydown', onWindowKeyDown, true); + // Only add listeners once + if (!listenersAttached) { + window.addEventListener('focusin', syncFocus); + window.addEventListener('keydown', onWindowKeyDown, true); + listenersAttached = true; + } syncFocus(); } // Always return unregister function return () => { lastFocusElement = null; focusElements = focusElements.filter(ele => ele !== element); if (focusElements.length === 0) { window.removeEventListener('focusin', syncFocus); window.removeEventListener('keydown', onWindowKeyDown, true); + listenersAttached = false; } }; }基于之前的审查意见。
181-192: useEffect 仍缺少 getElement 依赖项。之前的审查和 Pipeline 都已指出，
useEffect缺少getElement依赖。虽然getElement通常是稳定的回调函数，但为了符合 React Hooks 规范并消除警告，应将其添加到依赖数组中。应用以下差异修复：
export function useLockFocus( lock: boolean, getElement: () => HTMLElement | null, ) { useEffect(() => { if (lock) { const element = getElement(); if (element) { return lockFocus(element); } } - }, [lock]); + }, [lock, getElement]); }如果担心
getElement引用不稳定导致不必要的重新执行，可以考虑使用useCallback包装调用方的 getter，或在此处使用 ref 存储 getter。基于之前的审查意见和 Pipeline 警告。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Dom/focus.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Dom/focus.ts (1)
src/Dom/isVisible.ts (1)
element(1-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test / react component workflow
- GitHub Check: test / react component workflow
| let lastFocusElement: HTMLElement | null = null; | ||
| let focusElements: HTMLElement[] = []; |
There was a problem hiding this comment.
全局状态设计仍然存在多锁冲突问题。
之前的审查已经指出，lastFocusElement 和 focusElements 作为全局可变状态，getLastElement() 总是返回最新添加的元素，syncFocus 和 onWindowKeyDown 只对这个"最后"的元素进行操作。这意味着同一时间只有一个焦点锁真正生效，新的锁会覆盖旧的锁。
如果需要支持多个独立的焦点锁同时存在（例如嵌套的模态框），当前的全局状态管理会导致不正确和不可预测的行为。建议为每个 lockFocus 实例封装状态，或重新设计全局状态以正确管理多个活动锁（例如使用栈/队列方式来尊重活动锁的顺序和上下文）。
基于之前的审查意见。
🤖 Prompt for AI Agents
In src/Dom/focus.ts around lines 103-104, the globals lastFocusElement and
focusElements create multi-lock conflicts because
getLastElement()/syncFocus/onWindowKeyDown only operate on the most recently
added element, allowing newer locks to override older ones; refactor to
encapsulate focus lock state per lock (e.g., a FocusLock class or factory) and
maintain a central stack/registry of active locks so only the top (active) lock
handles keyboard/window events, and ensure locks register/unregister themselves
(push on lock creation, remove on release) and perform cleanup; update syncFocus
and onWindowKeyDown to consult the central stack for the current active lock
rather than global arrays so nested modals/focus regions behave predictably.
| lastFocusElement = null; | ||
| focusElements = focusElements.filter(ele => ele !== element); | ||
| if (focusElements.length === 0) { | ||
| window.removeEventListener('focusin', syncFocus); | ||
| window.removeEventListener('keydown', onWindowKeyDown, true); | ||
| } |
There was a problem hiding this comment.
清理函数过早重置 lastFocusElement 导致焦点追踪失效。
在第 167 行，清理函数无条件地将 lastFocusElement 设置为 null，即使还存在其他被锁定的元素（focusElements.length > 0）。这会破坏焦点追踪机制。
当多个元素依次被锁定和解锁时，如果某个中间元素解锁，lastFocusElement 会被重置，导致剩余锁定元素的焦点恢复逻辑失效。
应用以下差异修复：
// Always return unregister function
return () => {
- lastFocusElement = null;
focusElements = focusElements.filter(ele => ele !== element);
if (focusElements.length === 0) {
+ lastFocusElement = null;
window.removeEventListener('focusin', syncFocus);
window.removeEventListener('keydown', onWindowKeyDown, true);
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| lastFocusElement = null; | |
| focusElements = focusElements.filter(ele => ele !== element); | |
| if (focusElements.length === 0) { | |
| window.removeEventListener('focusin', syncFocus); | |
| window.removeEventListener('keydown', onWindowKeyDown, true); | |
| } | |
| lastFocusElement = null; | |
| focusElements = focusElements.filter(ele => ele !== element); | |
| if (focusElements.length === 0) { | |
| lastFocusElement = null; | |
| window.removeEventListener('focusin', syncFocus); | |
| window.removeEventListener('keydown', onWindowKeyDown, true); | |
| } |
| lastFocusElement = null; | |
| focusElements = focusElements.filter(ele => ele !== element); | |
| if (focusElements.length === 0) { | |
| window.removeEventListener('focusin', syncFocus); | |
| window.removeEventListener('keydown', onWindowKeyDown, true); | |
| } | |
| focusElements = focusElements.filter(ele => ele !== element); | |
| if (focusElements.length === 0) { | |
| lastFocusElement = null; | |
| window.removeEventListener('focusin', syncFocus); | |
| window.removeEventListener('keydown', onWindowKeyDown, true); | |
| } |
🤖 Prompt for AI Agents
In src/Dom/focus.ts around lines 167 to 172, the cleanup unconditionally sets
lastFocusElement = null which breaks focus tracking when other locked elements
remain; only clear lastFocusElement and remove the window listeners when
focusElements.length === 0. Move the lastFocusElement = null assignment inside
the existing if block (so it only runs when focusElements becomes empty) and
keep the event listener removals there as well.
Summary by CodeRabbit
新功能
文档
重构
测试
✏️ Tip: You can customize this high-level summary in your review settings.