feat: manage focus for accessible click/context popups#613
feat: manage focus for accessible click/context popups#613claytonlin1110 wants to merge 2 commits intoreact-component:masterfrom
Conversation
Walkthrough在 Trigger/Popup 链路中新增可选的焦点管理功能 Changes
Sequence Diagram(s)sequenceDiagram
participant Trigger
participant UniqueProvider
participant Popup
participant Document
Trigger->>UniqueProvider: open request (with focusPopup)
UniqueProvider->>Popup: render with focusPopup prop
Popup->>Popup: mount rootRef, track prevOpen
Popup->>Document: useLayoutEffect on open -> focusPopupRootOrFirst(root)
Note right of Popup: onKeyDownCapture -> handlePopupTabTrap
Popup->>Document: on close -> if active inside popup && target connected -> focus target
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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.10% -2.17%
==========================================
Files 17 18 +1
Lines 952 1082 +130
Branches 274 326 +52
==========================================
+ Hits 926 1029 +103
- Misses 26 53 +27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 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