feat: manage focus for accessible click/context popups#613
feat: manage focus for accessible click/context popups#613claytonlin1110 wants to merge 5 commits intoreact-component:masterfrom
Conversation
|
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在 Trigger/Popup 链路中新增可选聚焦管理配置 Changes
Sequence Diagram(s)sequenceDiagram
participant Trigger
participant UniqueProvider
participant Popup
participant Document
Trigger->>UniqueProvider: 请求打开（包含 focusPopup 选项）
UniqueProvider->>Popup: 渲染并传入 focusPopup
Popup->>Popup: 挂载 rootRef，记录 prevOpen
Popup->>Document: useLayoutEffect（closed→open） 调用 focusPopupRootOrFirst(root)
Popup->>Popup: 根节点注册 onKeyDownCapture -> handlePopupTabTrap
Popup->>Document: 关闭时（open→closed）若 active 在 popup 内且 trigger 仍 connected，则聚焦 trigger
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
@meet-student Please review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/focusUtils.ts`:
- Line 13: The instanceof check in isTabbable should use the frame's globals
rather than the top-level ones: change references from the global
HTMLInputElement to win.HTMLInputElement (where win is the defaultView passed
into isTabbable) so cross-realm inputs are detected correctly, and replace uses
of document.activeElement with container.ownerDocument.activeElement (or
win.document.activeElement) so activeElement is read from the same document as
the container; update the checks in the isTabbable function and any other
focus-related helpers that use global DOM constructors or document.activeElement
to use the passed-in win/container.ownerDocument to ensure correct behavior
across iframes.
In `@src/Popup/index.tsx`:
- Around line 255-266: The blur/refocus check uses the global
document.activeElement which breaks across documents (iframes/Shadow DOM);
update the logic in Popup's close branch (the block using root, target and
calling target.focus()—function/variables: focusPopupRootOrFirst, root, target)
to read active via root.ownerDocument.activeElement instead of
document.activeElement; apply the same change inside src/focusUtils.ts in
handlePopupTabTrap (replace document.activeElement with
root.ownerDocument.activeElement or the appropriate root/doc reference used
there) so cross-document focus containment checks (root.contains(active)) work
correctly.
🪄 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: a5c9b77a-5ef2-40d3-acca-4d3662621910
📒 Files selected for processing (6)
src/Popup/index.tsxsrc/UniqueProvider/index.tsxsrc/context.tssrc/focusUtils.tssrc/index.tsxtests/focus.test.tsx
src/focusUtils.ts
Outdated
| if ('disabled' in el && (el as HTMLButtonElement).disabled) { | ||
| return false; | ||
| } | ||
| if (el instanceof HTMLInputElement && el.type === 'hidden') { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 只读核验：定位跨 realm 敏感写法
rg -n "instanceof HTMLInputElement|document\.activeElement" src/focusUtils.ts src/Popup/index.tsxRepository: react-component/trigger
Length of output: 326
🏁 Script executed:
cat -n src/focusUtils.ts | head -80Repository: react-component/trigger
Length of output: 2681
🏁 Script executed:
cat -n src/Popup/index.tsx | sed -n '250,270p'Repository: react-component/trigger
Length of output: 762
🏁 Script executed:
# Check if there are any tests related to focusUtils
fd -e test -e spec | xargs grep -l "focusUtils\|isTabbable\|handlePopupTabTrap" 2>/dev/null | head -5Repository: react-component/trigger
Length of output: 49
🏁 Script executed:
# Check for any tests or documentation about iframe/cross-realm support
fd -type f \( -name "*.test.*" -o -name "*.spec.*" -o -name "README*" \) | xargs grep -l "iframe\|cross.realm\|cross.document" 2>/dev/nullRepository: react-component/trigger
Length of output: 239
🏁 Script executed:
# Check git history or issues directory for cross-realm discussions
find . -name "*.md" -o -name "*.txt" | xargs grep -i "iframe\|cross.realm\|cross.document\|cross.window" 2>/dev/null | head -20Repository: react-component/trigger
Length of output: 49
🏁 Script executed:
# Check if there are existing focus-related tests
fd -type f \( -name "*.test.*" -o -name "*.spec.*" \) | xargs grep -l "focus\|tabbable\|focusUtils" 2>/dev/nullRepository: react-component/trigger
Length of output: 239
🏁 Script executed:
# Find test files with correct syntax
fd "\.test\.|\.spec\." | head -20Repository: react-component/trigger
Length of output: 465
🏁 Script executed:
# Search for focus-related tests
rg -l "focus|tabbable|focusUtils" --type ts --type tsx --type js --type jsx 2>/dev/null | grep -i "test\|spec"Repository: react-component/trigger
Length of output: 49
🏁 Script executed:
# Check if container.contains works cross-document
rg -n "contains" src/focusUtils.tsRepository: react-component/trigger
Length of output: 117
🏁 Script executed:
# Look for any cross-document or shadow DOM concerns in the codebase
rg -i "ownerDocument|defaultView|cross.document|shadow" src/ | head -20Repository: react-component/trigger
Length of output: 1286
🏁 Script executed:
cat tests/focus.test.tsxRepository: react-component/trigger
Length of output: 5444
🏁 Script executed:
# Check what win parameter is used for in isTabbable
rg -A 5 "function isTabbable" src/focusUtils.tsRepository: react-component/trigger
Length of output: 275
跨 realm（iframe）下的 instanceof 判定与 activeElement 读取需修正。
Line 13 使用全局 HTMLInputElement 进行 instanceof 判定，但 isTabbable 已接收 win 参数（来自 container.ownerDocument.defaultView）；当容器在不同 document 中时判定失效。
Line 66 使用全局 document.activeElement 而应使用 container.ownerDocument.activeElement，虽然 container.contains() 提供了部分保护但不是完整解决方案。
建议修正：
修复方案
function isTabbable(el: HTMLElement, win: Window): boolean {
- if (el instanceof HTMLInputElement && el.type === 'hidden') {
+ if (el instanceof win.HTMLInputElement && el.type === 'hidden') {
return false;
}
export function handlePopupTabTrap(
e: React.KeyboardEvent,
container: HTMLElement,
): void {
- const active = document.activeElement as HTMLElement | null;
+ const active = container.ownerDocument.activeElement as HTMLElement | null;📝 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.
| if (el instanceof HTMLInputElement && el.type === 'hidden') { | |
| function isTabbable(el: HTMLElement, win: Window): boolean { | |
| if (el instanceof win.HTMLInputElement && el.type === 'hidden') { | |
| return false; | |
| } | |
| // ... rest of function | |
| } | |
| export function handlePopupTabTrap( | |
| e: React.KeyboardEvent, | |
| container: HTMLElement, | |
| ): void { | |
| const active = container.ownerDocument.activeElement as HTMLElement | null; | |
| // ... rest of function | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/focusUtils.ts` at line 13, The instanceof check in isTabbable should use
the frame's globals rather than the top-level ones: change references from the
global HTMLInputElement to win.HTMLInputElement (where win is the defaultView
passed into isTabbable) so cross-realm inputs are detected correctly, and
replace uses of document.activeElement with
container.ownerDocument.activeElement (or win.document.activeElement) so
activeElement is read from the same document as the container; update the checks
in the isTabbable function and any other focus-related helpers that use global
DOM constructors or document.activeElement to use the passed-in
win/container.ownerDocument to ensure correct behavior across iframes.
There was a problem hiding this comment.
This check seems unnecessary: hidden input are, well, hidden, and therefore do not receive focus from the browser.
There was a problem hiding this comment.
@KittyGiraudel, you're right — <input type="hidden"> elements are non-focusable per the HTML spec, and calling .focus() on them is a no-op in all browsers. That check is redundant and can be safely removed.
That also makes the cross-realm instanceof win.HTMLInputElement concern I raised for line 13 moot, since the fix is simply to delete the check entirely rather than correct it. The separate concern about using container.ownerDocument.activeElement instead of document.activeElement on line 66 is independent and still applies, but the hidden-input guard can go.
src/Popup/index.tsx
Outdated
| if (open && !wasOpen && root && isNodeVisible) { | ||
| focusPopupRootOrFirst(root); | ||
| } else if (!open && wasOpen && root) { | ||
| const active = document.activeElement as HTMLElement | null; | ||
| if ( | ||
| target && | ||
| active && | ||
| (root === active || root.contains(active)) | ||
| ) { | ||
| if (target.isConnected) { | ||
| target.focus(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 只读核验：定位当前实现中 activeElement 的 document 作用域
rg -n "activeElement" src/Popup/index.tsx src/focusUtils.tsRepository: react-component/trigger
Length of output: 241
🏁 Script executed:
# 查看src/Popup/index.tsx第255-270行的完整上下文
sed -n '255,270p' src/Popup/index.tsxRepository: react-component/trigger
Length of output: 506
🏁 Script executed:
# 查看src/focusUtils.ts中activeElement的完整上下文
sed -n '60,75p' src/focusUtils.tsRepository: react-component/trigger
Length of output: 407
🏁 Script executed:
# 检查root对象的声明和初始化，确认它是否总是可用
sed -n '1,260p' src/Popup/index.tsx | grep -n "root" | tail -20Repository: react-component/trigger
Length of output: 305
🏁 Script executed:
# 搜索与iframe或跨document相关的测试或文档
rg -i "iframe|cross.document|ownerDocument" --type ts --type tsxRepository: react-component/trigger
Length of output: 94
🏁 Script executed:
# 检查是否有其他地方也使用了document.activeElement且未使用ownerDocument
rg -n "document\.activeElement" --type ts --type tsxRepository: react-component/trigger
Length of output: 94
🏁 Script executed:
# 查看focusUtils.ts的完整容器相关上下文
sed -n '50,85p' src/focusUtils.tsRepository: react-component/trigger
Length of output: 834
🏁 Script executed:
# 查看src/Popup/index.tsx更多上下文，理解root的生命周期
sed -n '240,275p' src/Popup/index.tsxRepository: react-component/trigger
Length of output: 927
🏁 Script executed:
# 搜索ownerDocument的现有使用
rg "ownerDocument" --type ts --type javascriptRepository: react-component/trigger
Length of output: 101
🏁 Script executed:
# 搜索与焦点相关的测试文件
fd -e "test" -e "spec"Repository: react-component/trigger
Length of output: 49
🏁 Script executed:
# 检查是否有iframe相关的使用场景
rg -i "iframe" .Repository: react-component/trigger
Length of output: 49
🏁 Script executed:
# 验证HTMLElement是否有ownerDocument属性（应该有）
rg "ownerDocument" --type ts --type tsx -B 3 -A 3Repository: react-component/trigger
Length of output: 94
🏁 Script executed:
# 查看focusUtils.ts中handlePopupTabTrap的完整函数签名和使用
rg -B 5 -A 15 "function handlePopupTabTrap"Repository: react-component/trigger
Length of output: 857
🏁 Script executed:
# 检查root对象的类型定义
sed -n '166,170p' src/Popup/index.tsxRepository: react-component/trigger
Length of output: 235
🏁 Script executed:
# 搜索where handlePopupTabTrap被调用
rg "handlePopupTabTrap" --type ts --type tsxRepository: react-component/trigger
Length of output: 94
🏁 Script executed:
# 检查是否有任何关于focus管理或文档隔离的注释或文档
rg -i "focus|document" README.md CHANGELOG.md 2>/dev/null | head -30Repository: react-component/trigger
Length of output: 385
🏁 Script executed:
# 确认focusUtils.ts的同一问题，获取完整上下文
sed -n '55,75p' src/focusUtils.tsRepository: react-component/trigger
Length of output: 498
🏁 Script executed:
# 验证root.contains是否跨document兼容
rg -B 5 -A 5 "root.contains"Repository: react-component/trigger
Length of output: 582
跨 document 场景下回焦判定失效
Line 258 使用全局 document.activeElement，在 iframe、Shadow DOM 等跨 document 容器中，该元素属于主文档而非 popup 所在文档，导致 root.contains(active) 判定失败，回焦功能无法生效。
建议改为 root.ownerDocument.activeElement，同一问题也存在于 src/focusUtils.ts:66 的 handlePopupTabTrap 函数中。
🔧 修复建议
src/Popup/index.tsx
- const active = document.activeElement as HTMLElement | null;
+ const active = root.ownerDocument.activeElement as HTMLElement | null;src/focusUtils.ts
- const active = document.activeElement as HTMLElement | null;
+ const active = container.ownerDocument.activeElement as HTMLElement | null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Popup/index.tsx` around lines 255 - 266, The blur/refocus check uses the
global document.activeElement which breaks across documents (iframes/Shadow
DOM); update the logic in Popup's close branch (the block using root, target and
calling target.focus()—function/variables: focusPopupRootOrFirst, root, target)
to read active via root.ownerDocument.activeElement instead of
document.activeElement; apply the same change inside src/focusUtils.ts in
handlePopupTabTrap (replace document.activeElement with
root.ownerDocument.activeElement or the appropriate root/doc reference used
there) so cross-document focus containment checks (root.contains(active)) work
correctly.
![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 focus management and tab trapping for popups, adding a new focusPopup prop to the Trigger component and a dedicated focusUtils.ts utility file. The implementation automatically handles moving focus into the popup upon opening and restoring it to the trigger upon closing, with default activation for click, context menu, and focus triggers. The review feedback suggests several improvements to the accessibility utilities, including expanding the selector for tabbable elements, using the :disabled pseudo-class to account for disabled fieldsets, and refining visibility checks to detect elements hidden by their ancestors.
src/focusUtils.ts
Outdated
| import type * as React from 'react'; | ||
|
|
||
| const TABBABLE_SELECTOR = | ||
| 'a[href], button, input, select, textarea, [tabindex]:not([tabindex="-1"])'; |
There was a problem hiding this comment.
The TABBABLE_SELECTOR is missing some naturally tabbable elements such as summary, [contenteditable], and media elements with controls (audio[controls], video[controls]). Consider expanding the selector for better accessibility coverage.
const TABBABLE_SELECTOR =
'a[href], button, input, select, textarea, summary, [contenteditable], audio[controls], video[controls], [tabindex]:not([tabindex="-1"])';There was a problem hiding this comment.
Even that list still misses “tabbable” elements. See https://github.com/KittyGiraudel/focusable-selectors (which is still not exhaustive, but better than nothing).
src/focusUtils.ts
Outdated
| if (el.closest('[aria-hidden="true"]')) { | ||
| return false; | ||
| } | ||
| if ('disabled' in el && (el as HTMLButtonElement).disabled) { |
There was a problem hiding this comment.
Checking for the disabled property on the element itself does not account for elements disabled via a parent <fieldset disabled>. Using el.matches(':disabled') is a more robust way to check if an element is effectively disabled in the DOM.
| if ('disabled' in el && (el as HTMLButtonElement).disabled) { | |
| if (el.matches(':disabled')) { | |
| return false; | |
| } |
There was a problem hiding this comment.
Instead of doing that check in JavaScript, you can shift that to the selectors themselves in the TABBABLE_SELECTOR constant.
| return false; | ||
| } | ||
| const style = win.getComputedStyle(el); | ||
| if (style.display === 'none' || style.visibility === 'hidden') { |
There was a problem hiding this comment.
Checking display and visibility on the element itself via getComputedStyle does not detect if the element is hidden because one of its ancestors has display: none. A more reliable check for layout visibility is el.getClientRects().length > 0 or checking el.offsetParent === null (though the latter has edge cases for fixed elements).
| if (style.display === 'none' || style.visibility === 'hidden') { | |
| if (el.getClientRects().length === 0) { | |
| return false; | |
| } |
There was a problem hiding this comment.
The suggestion from Gemini makes sense. You want to check for the element’s offset dimensions and bounding rects instead of every possible way of manually hiding an element with CSS.
function isVisible(element) {
return Boolean(
element.offsetWidth ||
element.offsetHeight ||
element.getClientRects().length
)
}
|
@meet-student had a chance to review? |
|
@afc163 @meet-student please review |
src/focusUtils.ts
Outdated
| const ti = el.getAttribute('tabindex'); | ||
| if (ti !== null && Number(ti) < 0) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This check can be moved into the selectors themselves to avoid having to do it in JavaScript.
src/focusUtils.ts
Outdated
| } | ||
|
|
||
| /** Visible, tabbable descendants inside `container` (in DOM order). */ | ||
| export function getTabbableElements(container: HTMLElement): HTMLElement[] { |
There was a problem hiding this comment.
I think it’s worth keeping in mind, we do not actually need to list all the tabbable elements. We just need the 2 tabbable edges: the first one, and the last one — anything in between is completely unused. This is something I’ve solved for a11y-dialog (including accounting for Shadow DOM) if you’re interested:
| const active = document.activeElement as HTMLElement | null; | ||
| if ( | ||
| target && | ||
| active && | ||
| (root === active || root.contains(active)) | ||
| ) { |
There was a problem hiding this comment.
Why do we check whether we have an active element before moving the focus to the target (which I assume is the previously focused element prior opening the popover)? I think the code can be simplified into: target?.focus(). Unless I’m missing something.
|
@KittyGiraudel Thanks for your review, I just updated PR. Please review again |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/focusUtils.ts (1)
154-154:⚠️ Potential issue | 🟠 Major跨 document 场景下
activeElement读取对象不一致。Line 154 使用全局
document.activeElement，在 iframe/多 document 场景会导致焦点判定偏差，进而让 Tab trap 失效。建议改为container.ownerDocument.activeElement。🔧 建议修改
- const active = document.activeElement as HTMLElement | null; + const active = container.ownerDocument.activeElement as HTMLElement | null;#!/bin/bash # 只读核验：确认是否仍在使用全局 activeElement rg -n "document\.activeElement|ownerDocument\.activeElement" src/focusUtils.ts src/Popup/index.tsx -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/focusUtils.ts` at line 154, In focusUtils.ts the code reads the global document.activeElement into the variable active which breaks in iframe/multi-document contexts; update the logic in the function that defines const active = document.activeElement as HTMLElement | null to use container.ownerDocument.activeElement (falling back to container if ownerDocument is missing) so focus checks and the Tab-trap use the container's document; ensure you reference the same variable name (active) and preserve the existing null typing/guards where functions like the Tab-trap rely on it.
🤖 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/focusUtils.ts`:
- Line 154: In focusUtils.ts the code reads the global document.activeElement
into the variable active which breaks in iframe/multi-document contexts; update
the logic in the function that defines const active = document.activeElement as
HTMLElement | null to use container.ownerDocument.activeElement (falling back to
container if ownerDocument is missing) so focus checks and the Tab-trap use the
container's document; ensure you reference the same variable name (active) and
preserve the existing null typing/guards where functions like the Tab-trap rely
on it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7752ad1-f9c5-411c-a739-25d838ca5a74
📒 Files selected for processing (2)
src/Popup/index.tsxsrc/focusUtils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Popup/index.tsx
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #613 +/- ##
==========================================
- Coverage 97.26% 95.28% -1.99%
==========================================
Files 17 18 +1
Lines 952 1082 +130
Branches 274 330 +56
==========================================
+ Hits 926 1031 +105
- Misses 26 51 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
tests/focus.test.tsx
Outdated
| const popup = document.querySelector('.rc-trigger-popup')!; | ||
| expect(popup).toBeTruthy(); | ||
|
|
||
| const btnA = Array.from(document.querySelectorAll('button')).find( |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/focusUtils.test.tsx (2)
29-34: 建议统一在afterEach清理挂载节点，避免失败路径遗留 DOM。目前依赖每个用例手动
unmount/removeChild。当断言提前失败时，清理不会执行，可能影响后续用例。建议在mount内登记 disposer，并在afterEach统一回收。参考改法
describe('focusUtils', () => { let eleRect = { width: 100, height: 100 }; + const cleanups: Array<() => void> = []; beforeAll(() => { @@ beforeEach(() => { eleRect = { width: 100, height: 100 }; }); + + afterEach(() => { + while (cleanups.length) { + cleanups.pop()!(); + } + }); function mount(el: HTMLElement) { document.body.appendChild(el); - return () => { + const unmount = () => { document.body.removeChild(el); }; + cleanups.push(unmount); + return unmount; }Also applies to: 41-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/focusUtils.test.tsx` around lines 29 - 34, The test helper mount function currently appends elements directly and returns a manual disposer, which can be skipped if a test fails; modify mount (the function named mount in tests/focusUtils.test.tsx) to register its disposer in a shared array and add an afterEach hook that iterates this array to remove any remaining mounted nodes, clearing the array afterwards; ensure the returned disposer still removes the node and also removes itself from the shared registry so tests that call the disposer explicitly still work.
201-220: 建议补一条“正向 Tab 从 last/container 回到 first”的分支用例。
handlePopupTabTrap的 Shift+Tab 分支已覆盖，但正向 Tab 回环（active === last || active === container）是同等关键路径，建议补测以防回归。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/focusUtils.test.tsx` around lines 201 - 220, Add a new test that mirrors the existing Shift+Tab case but verifies the forward Tab wrap: create an element with two buttons (ids 'x' and 'y'), mount it and focus the last button ('y' or set document.activeElement to the container), create a React.KeyboardEvent-like object with key 'Tab' and shiftKey false, call handlePopupTabTrap(ev, el), assert ev.preventDefault was called and that focus moved to the first button ('x'), then unmount; reference handlePopupTabTrap, mount/unmount, and the button ids to locate where to add the test.
🤖 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/focusUtils.test.tsx`:
- Around line 12-23: 当前在 tests/focusUtils.test.tsx 的 beforeAll 中对原型使用
spyElementPrototypes 打桩（调用 spyElementPrototypes(HTMLElement, ...) 和
spyElementPrototypes(HTMLButtonElement, ...)）但没有保存或调用返回的恢复函数，建议修改为在 beforeAll
时接收 spyElementPrototypes 的返回值（例如 restore 或 mockRestore）并在 afterAll
钩子中调用以恢复原型；如果这是仓库全局约定，则统一两种解决方案：要么在每个涉及 spyElementPrototypes 的测试文件（如
focusUtils.test.tsx、align.test.tsx、ref.test.tsx、rect.test.tsx）都添加 afterAll 调用以
restore，对应的标识符为 spyElementPrototypes 返回的恢复函数，要么在测试文档中明确说明原型打桩应跨测试持久化并保持当前代码不变。
---
Nitpick comments:
In `@tests/focusUtils.test.tsx`:
- Around line 29-34: The test helper mount function currently appends elements
directly and returns a manual disposer, which can be skipped if a test fails;
modify mount (the function named mount in tests/focusUtils.test.tsx) to register
its disposer in a shared array and add an afterEach hook that iterates this
array to remove any remaining mounted nodes, clearing the array afterwards;
ensure the returned disposer still removes the node and also removes itself from
the shared registry so tests that call the disposer explicitly still work.
- Around line 201-220: Add a new test that mirrors the existing Shift+Tab case
but verifies the forward Tab wrap: create an element with two buttons (ids 'x'
and 'y'), mount it and focus the last button ('y' or set document.activeElement
to the container), create a React.KeyboardEvent-like object with key 'Tab' and
shiftKey false, call handlePopupTabTrap(ev, el), assert ev.preventDefault was
called and that focus moved to the first button ('x'), then unmount; reference
handlePopupTabTrap, mount/unmount, and the button ids to locate where to add the
test.
🪄 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: c6f96723-1645-4610-8089-2071ae91e885
📒 Files selected for processing (2)
tests/focus.test.tsxtests/focusUtils.test.tsx
✅ Files skipped from review due to trivial changes (1)
- tests/focus.test.tsx
| beforeAll(() => { | ||
| spyElementPrototypes(HTMLElement, { | ||
| offsetWidth: { get: () => eleRect.width }, | ||
| offsetHeight: { get: () => eleRect.height }, | ||
| offsetParent: { get: () => document.body }, | ||
| }); | ||
| spyElementPrototypes(HTMLButtonElement, { | ||
| offsetWidth: { get: () => eleRect.width }, | ||
| offsetHeight: { get: () => eleRect.height }, | ||
| offsetParent: { get: () => document.body }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) 查看当前文件是否有 restore/cleanup
rg -n -C2 "spyElementPrototypes|afterAll|afterEach|restore|reset" tests/focusUtils.test.tsx
# 2) 查看仓库内该工具的常见使用模式（是否接收返回值并在 afterAll/afterEach 调用）
rg -n -C3 "spyElementPrototypes\\(" --type ts --type tsxRepository: react-component/trigger
Length of output: 632
🏁 Script executed:
# View the complete test file to check for cleanup logic
cat tests/focusUtils.test.tsxRepository: react-component/trigger
Length of output: 7616
🏁 Script executed:
# Search for all uses of spyElementPrototypes in the repository
fd -e ts -e tsx | head -20 | xargs grep -l "spyElementPrototypes" 2>/dev/null || find . -name "*.test.ts" -o -name "*.test.tsx" | xargs grep -l "spyElementPrototypes" 2>/dev/null | head -10Repository: react-component/trigger
Length of output: 318
🏁 Script executed:
# Try to find and examine the spyElementPrototypes implementation
find . -type d -name "@rc-component" 2>/dev/null | head -5
find . -type f -name "*domHook*" 2>/dev/nullRepository: react-component/trigger
Length of output: 1131
🏁 Script executed:
# Check the domHook implementation
cat ./node_modules/.pnpm/rc-util@5.44.4_react-dom@18.3.1_react@19.2.5__react@19.2.5/node_modules/rc-util/lib/test/domHook.d.tsRepository: react-component/trigger
Length of output: 448
🏁 Script executed:
# Also check the JavaScript implementation
cat ./node_modules/.pnpm/rc-util@5.44.4_react-dom@18.3.1_react@19.2.5__react@19.2.5/node_modules/rc-util/lib/test/domHook.jsRepository: react-component/trigger
Length of output: 2671
🏁 Script executed:
# Search for other test files that use spyElementPrototypes to see the common pattern
rg "spyElementPrototypes" tests/ -A 5 -B 5 2>/dev/null | head -80Repository: react-component/trigger
Length of output: 3628
原型打桩未显式恢复，但这是仓库中的系统性做法。
第 12-23 行的 spyElementPrototypes 调用未捕获返回的 mockRestore() 函数，也没有在 afterAll 中调用恢复。虽然从 API 角度该方法 可以 恢复，但当前代码没有这样做。
需要注意的是，align.test.tsx、ref.test.tsx、rect.test.tsx 等多个测试文件都采用了相同的模式——在 beforeAll 中调用 spyElementPrototypes 而不恢复。这表明这可能是该仓库接受的做法，或是需要系统性修复的已知限制。
建议统一处理：要么在所有测试文件中添加 afterAll(() => { ...restore() }) 钩子，要么文档化说明原型打桩设计为跨测试持久化。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/focusUtils.test.tsx` around lines 12 - 23, 当前在
tests/focusUtils.test.tsx 的 beforeAll 中对原型使用 spyElementPrototypes 打桩（调用
spyElementPrototypes(HTMLElement, ...) 和 spyElementPrototypes(HTMLButtonElement,
...)）但没有保存或调用返回的恢复函数，建议修改为在 beforeAll 时接收 spyElementPrototypes 的返回值（例如 restore 或
mockRestore）并在 afterAll 钩子中调用以恢复原型；如果这是仓库全局约定，则统一两种解决方案：要么在每个涉及
spyElementPrototypes 的测试文件（如
focusUtils.test.tsx、align.test.tsx、ref.test.tsx、rect.test.tsx）都添加 afterAll 调用以
restore，对应的标识符为 spyElementPrototypes 返回的恢复函数，要么在测试文档中明确说明原型打桩应跨测试持久化并保持当前代码不变。
src/focusUtils.ts
Outdated
| const TABBABLE_SELECTOR = | ||
| 'a[href], button, input, select, textarea, [tabindex]:not([tabindex^="-"])'; |
There was a problem hiding this comment.
This still lacks a lot of selectors: details, audio, video and more. I recommend pulling a more comprehensive list from focusable-selectors.
src/focusUtils.ts
Outdated
| if ('disabled' in el && (el as HTMLButtonElement).disabled) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This is not solely specific to buttons: input, select, fieldset and textarea (and probably more) can all be disabled. This would be better moved into the tabbable selectors as :not(:disabled).
| const style = win.getComputedStyle(el); | ||
| if (style.display === 'none' || style.visibility === 'hidden') { |
There was a problem hiding this comment.
This is redundant with isNonVisibleForInteraction which already covers it: if an element has no dimensions, they’re hidden and non-focusable — regardless of their display or visibility properties.
There was a problem hiding this comment.
isNonVisibleForInteraction() catches elements that have no rendered box (offsetWidth/offsetHeight/getClientRects() all empty), which maps well to many display:none cases.
But visibility:hidden elements can still have layout dimensions and client rects, so they can slip past that check. The explicit style guard prevents those from being treated as tabbable.
So I'd keep this block, mainly for visibility:hidden correctness.
| if (!active || !container.contains(active)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I do not understand this: if the container doesn’t contain the active element, then the trap has failed, hasn’t it? Isn’t the whole idea to ensure the container keeps containing the active element while the trap is up?
There was a problem hiding this comment.
We should keep this guard.
container.contains(active) is intentional so we only cycle focus when the active element is currently inside the popup. If focus has already moved outside (click outside, programmatic focus shift, close timing, portal/shadow edge cases), forcing wrap would steal focus unexpectedly.
So this check is a defensive no-op when the trap is no longer applicable, not a trap failure we should “fix” inside this handler.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/focusUtils.ts (1)
148-149:⚠️ Potential issue | 🟠 Major请从
container.ownerDocument读取当前焦点。这里用全局
document.activeElement，在 iframe / 多 document 场景下会读错文档，container.contains(active)会直接失效，Tab trap 也就不会生效。应改为读取container.ownerDocument.activeElement。🔧 建议修改
- const active = document.activeElement as HTMLElement | null; + const active = container.ownerDocument.activeElement as HTMLElement | null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/focusUtils.ts` around lines 148 - 149, 把对当前焦点元素的读取改为从容器所属文档读取：在处理 getTabbableEdges(container) 后，不要使用全局 document.activeElement，而应使用 container.ownerDocument.activeElement；更新 active 变量的赋值（在使用 container.contains(active) 的上下文中）以确保在 iframe/多文档场景下判断正确并保持 Tab trap 生效，相关位置参考函数/变量 getTabbableEdges、first、last 和 active 的使用处进行替换。
🤖 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/focusUtils.ts`:
- Around line 83-93: The slot branch in findTabbableEl currently ignores
fallback content when (el as HTMLSlotElement).assignedElements() returns empty;
change it so when assigned.length === 0 you fall back to the slot's own child
elements (e.g. Array.from((el as HTMLSlotElement).children) or el.childNodes
filtered to elements), assign that to ordered (respecting the forward ? order :
reverse), and then iterate/recursively call findTabbableEl as you do for
assigned elements so default slot/fallback content is included in traversal.
---
Duplicate comments:
In `@src/focusUtils.ts`:
- Around line 148-149: 把对当前焦点元素的读取改为从容器所属文档读取：在处理 getTabbableEdges(container)
后，不要使用全局 document.activeElement，而应使用 container.ownerDocument.activeElement；更新
active 变量的赋值（在使用 container.contains(active) 的上下文中）以确保在 iframe/多文档场景下判断正确并保持 Tab
trap 生效，相关位置参考函数/变量 getTabbableEdges、first、last 和 active 的使用处进行替换。
🪄 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: 24656018-41bd-49d3-ba85-c64493ca3e39
📒 Files selected for processing (3)
package.jsonsrc/focusUtils.tstests/focusUtils.test.tsx
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/focusUtils.test.tsx
| } else if (el.localName === 'slot') { | ||
| const assigned = (el as HTMLSlotElement).assignedElements({ | ||
| flatten: true, | ||
| }) as HTMLElement[]; | ||
| const ordered = forward ? assigned : [...assigned].reverse(); | ||
| for (let i = 0; i < ordered.length; i += 1) { | ||
| const hit = findTabbableEl(ordered[i], forward, win); | ||
| if (hit) { | ||
| return hit; | ||
| } | ||
| } |
There was a problem hiding this comment.
插槽的 fallback 内容现在不会参与遍历。
当 <slot> 没有分配节点时，这里直接结束了分支，导致默认插槽内容里的可聚焦元素永远找不到；打开时首个聚焦和 Tab 环都可能失效。
🔧 建议修改
} else if (el.localName === 'slot') {
const assigned = (el as HTMLSlotElement).assignedElements({
flatten: true,
}) as HTMLElement[];
- const ordered = forward ? assigned : [...assigned].reverse();
+ const candidates =
+ assigned.length > 0
+ ? assigned
+ : (Array.from(el.children) as HTMLElement[]);
+ const ordered = forward ? candidates : [...candidates].reverse();
for (let i = 0; i < ordered.length; i += 1) {
const hit = findTabbableEl(ordered[i], forward, win);
if (hit) {
return hit;
}📝 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.
| } else if (el.localName === 'slot') { | |
| const assigned = (el as HTMLSlotElement).assignedElements({ | |
| flatten: true, | |
| }) as HTMLElement[]; | |
| const ordered = forward ? assigned : [...assigned].reverse(); | |
| for (let i = 0; i < ordered.length; i += 1) { | |
| const hit = findTabbableEl(ordered[i], forward, win); | |
| if (hit) { | |
| return hit; | |
| } | |
| } | |
| } else if (el.localName === 'slot') { | |
| const assigned = (el as HTMLSlotElement).assignedElements({ | |
| flatten: true, | |
| }) as HTMLElement[]; | |
| const candidates = | |
| assigned.length > 0 | |
| ? assigned | |
| : (Array.from(el.children) as HTMLElement[]); | |
| const ordered = forward ? candidates : [...candidates].reverse(); | |
| for (let i = 0; i < ordered.length; i += 1) { | |
| const hit = findTabbableEl(ordered[i], forward, win); | |
| if (hit) { | |
| return hit; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/focusUtils.ts` around lines 83 - 93, The slot branch in findTabbableEl
currently ignores fallback content when (el as
HTMLSlotElement).assignedElements() returns empty; change it so when
assigned.length === 0 you fall back to the slot's own child elements (e.g.
Array.from((el as HTMLSlotElement).children) or el.childNodes filtered to
elements), assign that to ordered (respecting the forward ? order : reverse),
and then iterate/recursively call findTabbableEl as you do for assigned elements
so default slot/fallback content is included in traversal.
3 participants
Summary
Implements focus behavior for floating popups as discussed for Ant Design Popover/Popconfirm-style usage (ant-design#57333): move focus into content on open, return it to the trigger on close, and keep Tab navigation inside the layer while open. Escape behavior is unchanged (existing Portal onEsc).
Default behavior
Summary by CodeRabbit