feat: Support focusable #561
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 significant refactoring of the Drawer component's focus management system. By consolidating auto-focus and focus trapping functionalities into a dedicated 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. 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 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
Sequence Diagram(s)sequenceDiagram
autonumber
%% Styling note: focus flow changed (highlighted)
participant User
participant Drawer as Drawer Component
participant Popup as DrawerPopup
participant Hook as useFocusable
participant Document
Note over User,Drawer: 用户交互触发打开/关闭
User->>Drawer: open/close props
Drawer->>Popup: render with props (autoFocus, focusTrap, mask)
Popup->>Hook: useFocusable(getContainer, open, autoFocus, focusTrap, mask)
alt open == true
Hook->>Hook: enable focus lock if mergedFocusTrap
Hook->>Popup: autofocus container (preventScroll)
Popup->>Document: focus trapped inside popup
else close
Hook->>Hook: disable focus lock
alt focusTriggerAfterClose !== false
Drawer->>Document: restore lastActiveElement focus
end
end
预估代码审查工作量🎯 3 (中等复杂度) | ⏱️ ~25 分钟 可能相关的 PR
诗歌
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #561 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 7 +1
Lines 217 205 -12
Branches 79 75 -4
=========================================
- Hits 217 205 -12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (3)
src/Drawer.tsx (1)
52-52: 新增的公共 API 属性文档完善。新增的
focusTriggerAfterClose属性应该添加 JSDoc 注释说明其作用和默认行为，特别是要明确当值为undefined时的默认行为（根据代码第 121 行的逻辑，默认为true）。建议添加 JSDoc 注释
+ /** + * Whether to focus the trigger element after the drawer closes. + * @default true + */ focusTriggerAfterClose?: boolean;src/hooks/useFocusable.ts (1)
11-11: 焦点陷阱默认逻辑可能令人困惑。第 11 行的逻辑
focusTrap ?? mask !== false在mask为undefined时会得到true（因为undefined !== false为true）。这可能不是预期的行为。建议明确处理
mask为undefined的情况：建议的修复方案
- const mergedFocusTrap = focusTrap ?? mask !== false; + const mergedFocusTrap = focusTrap ?? (mask ?? true);或者，如果希望 mask 为 undefined 时不启用焦点陷阱：
- const mergedFocusTrap = focusTrap ?? mask !== false; + const mergedFocusTrap = focusTrap ?? mask === true;src/DrawerPopup.tsx (1)
25-27: FocusableConfig 类型定义不完整。
FocusableConfig接口只包含autoFocus属性，但代码中还使用了focusTrap属性（见第 40 行和第 158 行）。建议将focusTrap也添加到FocusableConfig中以保持类型定义的完整性。建议的类型定义
export interface FocusableConfig { autoFocus?: boolean; + focusTrap?: boolean; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
assets/index.lessdocs/examples/base.tsxdocs/examples/motion.tspackage.jsonsrc/Drawer.tsxsrc/DrawerPopup.tsxsrc/hooks/useFocusable.tstests/index.spec.tsx
💤 Files with no reviewable changes (1)
- tests/index.spec.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/DrawerPopup.tsx (4)
src/Drawer.tsx (1)
Placement(15-15)src/index.ts (1)
Placement(5-5)src/DrawerPanel.tsx (2)
DrawerPanelEvents(11-18)DrawerPanelAccessibility(20-23)src/hooks/useFocusable.ts (1)
useFocusable(4-22)
🔇 Additional comments (7)
assets/index.less (1)
35-35: 样式更改合理。为
.rc-drawer-content-wrapper添加pointer-events: auto是合理的，这确保了抽屉内容包装器可以接收指针事件，与新的焦点管理机制配合使用。src/Drawer.tsx (1)
119-126: 焦点恢复逻辑实现正确。焦点恢复的实现逻辑合理：
- 仅在关闭时触发（
!nextVisible）- 通过
focusTriggerAfterClose !== false实现默认启用- 检查 lastActiveRef 有效性且不在弹窗内部
- 使用
preventScroll: true避免滚动干扰注意：默认启用此行为（当
focusTriggerAfterClose为undefined时）可能对现有用户造成行为变更。建议在发版说明中标注此变更。src/DrawerPopup.tsx (2)
158-158: 焦点管理重构合理。使用
useFocusablehook 集中管理焦点逻辑是很好的重构，代码更清晰易维护。传递() => panelRef.current作为getContainer参数是合理的，因为它确保每次调用都能获取到最新的 DOM 引用。注意：确保
useFocusable内部正确处理了getContainer函数的调用时机。
322-322: 简化的回调传递更清晰。直接将
afterOpenChange传递给onVisibleChanged简化了代码，移除了不必要的包装函数，提高了代码可读性。docs/examples/base.tsx (1)
30-31: 测试用例增强合理。在 Drawer 内容中添加可聚焦的按钮元素，有助于测试新的焦点管理功能（如焦点陷阱、自动聚焦等）。这是合理的示例更新。
docs/examples/motion.ts (1)
13-13:motionDeadline是@rc-component/motion中CSSMotionProps的有效属性，无需修改。此属性用于设置强制完成动画的超时时间（毫秒），当 CSS 过渡/动画事件未触发时生效。package.json (1)
47-47: @rc-component/portal@^2.1.3 版本已验证有效且安全。版本 2.1.3 存在于 npm，已于 2025-12-29 发布，未被标记为废弃版本。多个安全扫描服务（Snyk、ReversingLabs、Socket.dev）确认该版本无已知安全漏洞。此依赖更新可以安全合并。
| React.useEffect(() => { | ||
| if (open && autoFocus === true) { | ||
| getContainer()?.focus({ preventScroll: true }); | ||
| } | ||
| }, [open]); |
There was a problem hiding this comment.
useEffect 依赖数组不完整。
第 17-21 行的 useEffect 依赖数组只包含 [open]，但实际上还依赖于 autoFocus 和 getContainer() 的返回值。
当 autoFocus 从 false 变为 true 时，或者 getContainer() 返回不同的元素时，effect 不会重新执行，可能导致焦点行为不符合预期。
建议的修复方案
React.useEffect(() => {
if (open && autoFocus === true) {
getContainer()?.focus({ preventScroll: true });
}
- }, [open]);
+ }, [open, autoFocus]);注意：getContainer 函数本身应该保持稳定（通过 useCallback 或其他方式），因此不需要添加到依赖数组中。如果 getContainer 不稳定，建议在调用处使用 useCallback 包装。
📝 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.
| React.useEffect(() => { | |
| if (open && autoFocus === true) { | |
| getContainer()?.focus({ preventScroll: true }); | |
| } | |
| }, [open]); | |
| React.useEffect(() => { | |
| if (open && autoFocus === true) { | |
| getContainer()?.focus({ preventScroll: true }); | |
| } | |
| }, [open, autoFocus]); |
🤖 Prompt for AI Agents
In src/hooks/useFocusable.ts around lines 17 to 21, the useEffect dependency
array only lists [open] but the effect also depends on autoFocus and the element
returned by getContainer(); update the dependency array to include autoFocus and
getContainer (or the stable reference used to obtain the container) so the
effect reruns when autoFocus flips or the container changes, and if getContainer
is not already stable, wrap it with useCallback at its source so it can safely
be included in the deps.
| expect(container.contains(document.activeElement)).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('tab should always in the content', () => { |
There was a problem hiding this comment.
换成 rc-util 的 useFocusLock 了，这个用不到了。
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/DrawerPopup.tsx (1)
34-36: 建议为focusTrap添加默认行为说明。新增的焦点相关属性类型定义正确，但
focusTrap的默认行为（当未指定时默认为mask !== false）可能不够直观。建议在 JSDoc 注释中明确说明此默认逻辑，以便使用者理解。🔎 建议的 JSDoc 注释
// Focus + /** Whether to auto-focus the drawer when opened */ autoFocus?: boolean; + /** Whether to trap focus within the drawer. Defaults to `true` when mask is enabled */ focusTrap?: boolean;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/DrawerPopup.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/DrawerPopup.tsx (1)
src/hooks/useFocusable.ts (1)
useFocusable(4-22)
🔇 Additional comments (3)
src/DrawerPopup.tsx (3)
17-17: 导入语句正确。新的
useFocusable钩子导入清晰且使用得当。
102-105: 属性解构正确。焦点相关属性的解构符合组件模式，注释清晰。
318-318: 回调签名兼容，简化有效。
afterOpenChange类型(open: boolean) => void与CSSMotion的onVisibleChanged回调签名(visible: boolean) => void完全匹配，直接传递是安全的。
| // ========================= Focusable ========================== | ||
| useFocusable(() => panelRef.current, open, autoFocus, focusTrap, mask); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the DrawerPopup.tsx file around the mentioned lines
cat -n src/DrawerPopup.tsx | sed -n '150,160p'Repository: react-component/drawer
Length of output: 525
🏁 Script executed:
# Check line 374 where panelRef is assigned
cat -n src/DrawerPopup.tsx | sed -n '370,380p'Repository: react-component/drawer
Length of output: 368
🏁 Script executed:
# Now examine the useFocusable hook
if [ -f "src/hooks/useFocusable.ts" ]; then
cat -n src/hooks/useFocusable.ts
else
echo "Checking for useFocusable location..."
find . -name "*useFocusable*" -type f
fiRepository: react-component/drawer
Length of output: 754
🏁 Script executed:
# Also search for the DrawerPanel component to understand structure
rg -n "DrawerPanel" src/DrawerPopup.tsx | head -20Repository: react-component/drawer
Length of output: 305
🏁 Script executed:
# Let's examine DrawerPanel to understand the component structure
fd "DrawerPanel.tsx" -x cat -n {} | head -100Repository: react-component/drawer
Length of output: 2210
🏁 Script executed:
# Search for other usages of useFocusable to understand the usage pattern
rg -n "useFocusable" --type=ts --type=tsxRepository: react-component/drawer
Length of output: 93
🏁 Script executed:
# Check if there are any tests that show the expected behavior
fd ".*\.(test|spec)\.(ts|tsx)$" -x grep -l "useFocusable\|focus" {} \;Repository: react-component/drawer
Length of output: 48
🏁 Script executed:
# Correct the rg search for useFocusable
rg "useFocusable" -A 3 -B 1Repository: react-component/drawer
Length of output: 938
🏁 Script executed:
# Check for test files
find . -name "*.test.ts" -o -name "*.spec.ts" -o -name "*.test.tsx" -o -name "*.spec.tsx" | head -10Repository: react-component/drawer
Length of output: 154
🏁 Script executed:
# Look at RefContext to understand the panel ref setup
rg "RefContext" -B 5 -A 10Repository: react-component/drawer
Length of output: 3740
🏁 Script executed:
# Let's look at the Drawer.tsx to see what refContext contains
cat -n src/Drawer.tsx | grep -A 20 "refContext"Repository: react-component/drawer
Length of output: 1289
🏁 Script executed:
# Check if panelRef in DrawerPopup is the same as panel ref in RefContext
cat -n src/DrawerPopup.tsx | grep -B 10 "new RefContext\|refContext"Repository: react-component/drawer
Length of output: 48
🏁 Script executed:
# Look at the entire DrawerPopup.tsx to understand the full structure
wc -l src/DrawerPopup.tsxRepository: react-component/drawer
Length of output: 89
🏁 Script executed:
# Search around where refContext is created in DrawerPopup
rg "refContext" src/DrawerPopup.tsx -B 5 -A 5Repository: react-component/drawer
Length of output: 48
🏁 Script executed:
# Let's look at the full DrawerPopup to understand the panelRef assignment and structure better
cat -n src/DrawerPopup.tsx | sed -n '120,160p'Repository: react-component/drawer
Length of output: 1226
🏁 Script executed:
# Let's also check what panelNode is
cat -n src/DrawerPopup.tsx | sed -n '300,340p'Repository: react-component/drawer
Length of output: 1539
🏁 Script executed:
# Let's look at the entire focus setup in DrawerPopup
cat -n src/DrawerPopup.tsx | sed -n '100,135p'Repository: react-component/drawer
Length of output: 778
🏁 Script executed:
# Let's check if there's any documentation or comments about the focus behavior
grep -n "focus" src/DrawerPopup.tsx -B 2 -A 2Repository: react-component/drawer
Length of output: 468
🏁 Script executed:
# Check the context.ts file to understand RefContext better
cat -n src/context.tsRepository: react-component/drawer
Length of output: 580
🏁 Script executed:
# Let's verify by examining how panelRef is used throughout DrawerPopup
rg "panelRef" src/DrawerPopup.tsx -B 2 -A 2Repository: react-component/drawer
Length of output: 574
🏁 Script executed:
# Check if DrawerPanel has any focus-related attributes or handling
rg "focus|tabIndex" src/DrawerPanel.tsx -B 2 -A 2Repository: react-component/drawer
Length of output: 126
🏁 Script executed:
# Let's check the useLockFocus hook to understand the complete focus behavior
rg "useLockFocus" -B 5 -A 15Repository: react-component/drawer
Length of output: 1172
🏁 Script executed:
# Let's look for any related issues or comments about focus in the codebase
rg "focus|Focus" src/DrawerPopup.tsx | head -20Repository: react-component/drawer
Length of output: 356
🏁 Script executed:
# Let's check the complete context.ts to see if there's a separate panel ref
cat -n src/context.ts | tail -20Repository: react-component/drawer
Length of output: 580
🏁 Script executed:
# Let's look at test files to understand the expected focus behavior
cat -n tests/index.spec.tsx | grep -A 10 -B 2 "focus\|autoFocus" | head -40Repository: react-component/drawer
Length of output: 480
🏁 Script executed:
# Let's check if there are any other drawer implementations or similar components for reference
find . -name "*.tsx" -o -name "*.ts" | xargs grep -l "useLockFocus\|useFocusable" | head -10Repository: react-component/drawer
Length of output: 115
🏁 Script executed:
# Let's trace through the complete flow - check what happens in the outer container render
cat -n src/DrawerPopup.tsx | sed -n '360,390p'Repository: react-component/drawer
Length of output: 936
🏁 Script executed:
# Let's verify one more time by looking at similar focus hook patterns in rc-component libraries
rg "useEffect.*\[open\]" -B 3 -A 3 --type=ts --type=tsxRepository: react-component/drawer
Length of output: 93
useEffect 缺失 autoFocus 依赖项，导致闭包问题。
useFocusable 钩子（src/hooks/useFocusable.ts 第 17-21 行）的 useEffect 使用了 autoFocus 参数，但依赖数组仅包含 [open]。当抽屉打开状态下 autoFocus 属性发生变化时，副作用不会重新执行，导致焦点行为与预期不符。
在 useEffect 的依赖数组中添加 autoFocus：
React.useEffect(() => {
if (open && autoFocus === true) {
getContainer()?.focus({ preventScroll: true });
}
- }, [open]);
+ }, [open, autoFocus]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/hooks/useFocusable.ts around lines 17 to 21 (and called from
src/DrawerPopup.tsx lines 153-154), the useEffect relies on the autoFocus
parameter but its dependency array only contains [open], causing a stale-closure
when autoFocus changes; update the effect's dependency array to include
autoFocus (and any other used props/refs) so the effect re-runs when autoFocus
changes, ensuring correct focus behavior when the drawer is open.
ant-design/ant-design#56360
属性打平提供，在 antd 里聚合，免得 antd 里和 rc 都要聚合一遍
Summary by CodeRabbit
发布说明
New Features
Refactor
Style
Docs / Examples
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.