-
-
Notifications
You must be signed in to change notification settings - Fork 243
feat: manage focus for accessible click/context popups #613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,10 @@ import PopupContent from './PopupContent'; | |
| import useOffsetStyle from '../hooks/useOffsetStyle'; | ||
| import { useEvent } from '@rc-component/util'; | ||
| import type { PortalProps } from '@rc-component/portal'; | ||
| import { | ||
| focusPopupRootOrFirst, | ||
| handlePopupTabTrap, | ||
| } from '../focusUtils'; | ||
|
|
||
| export interface MobileConfig { | ||
| mask?: boolean; | ||
|
|
@@ -85,6 +89,12 @@ export interface PopupProps { | |
|
|
||
| // Mobile | ||
| mobile?: MobileConfig; | ||
|
|
||
| /** | ||
| * Move focus into the popup when it opens and return it to `target` when it closes. | ||
| * Tab cycles within the popup. Escape is handled by Portal `onEsc`. | ||
| */ | ||
| focusPopup?: boolean; | ||
| } | ||
|
|
||
| const Popup = React.forwardRef<HTMLDivElement, PopupProps>((props, ref) => { | ||
|
|
@@ -149,8 +159,13 @@ const Popup = React.forwardRef<HTMLDivElement, PopupProps>((props, ref) => { | |
| stretch, | ||
| targetWidth, | ||
| targetHeight, | ||
|
|
||
| focusPopup, | ||
| } = props; | ||
|
|
||
| const rootRef = React.useRef<HTMLDivElement>(null); | ||
| const prevOpenRef = React.useRef(false); | ||
|
|
||
| const popupContent = typeof popup === 'function' ? popup() : popup; | ||
|
|
||
| // We can not remove holder only when motion finished. | ||
|
|
@@ -208,12 +223,7 @@ const Popup = React.forwardRef<HTMLDivElement, PopupProps>((props, ref) => { | |
| offsetY, | ||
| ); | ||
|
|
||
| // ========================= Render ========================= | ||
| if (!show) { | ||
| return null; | ||
| } | ||
|
|
||
| // >>>>> Misc | ||
| // >>>>> Misc (computed before conditional return; hooks must run every render) | ||
| const miscStyle: React.CSSProperties = {}; | ||
| if (stretch) { | ||
| if (stretch.includes('height') && targetHeight) { | ||
|
|
@@ -232,6 +242,49 @@ const Popup = React.forwardRef<HTMLDivElement, PopupProps>((props, ref) => { | |
| miscStyle.pointerEvents = 'none'; | ||
| } | ||
|
|
||
| useLayoutEffect(() => { | ||
| if (!focusPopup) { | ||
| prevOpenRef.current = open; | ||
| return; | ||
| } | ||
|
|
||
| const root = rootRef.current; | ||
| const wasOpen = prevOpenRef.current; | ||
| prevOpenRef.current = open; | ||
|
|
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 使用全局 建议改为 🔧 修复建议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 |
||
| } | ||
| } | ||
| }, [open, focusPopup, isNodeVisible, target]); | ||
|
|
||
| const onPopupKeyDownCapture = useEvent( | ||
| (e: React.KeyboardEvent<HTMLDivElement>) => { | ||
| if (!focusPopup || !open) { | ||
| return; | ||
| } | ||
| const root = rootRef.current; | ||
| if (root) { | ||
| handlePopupTabTrap(e, root); | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| // ========================= Render ========================= | ||
| if (!show) { | ||
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <Portal | ||
| open={forceRender || isNodeVisible} | ||
|
|
@@ -276,7 +329,7 @@ const Popup = React.forwardRef<HTMLDivElement, PopupProps>((props, ref) => { | |
|
|
||
| return ( | ||
| <div | ||
| ref={composeRef(resizeObserverRef, ref, motionRef)} | ||
| ref={composeRef(resizeObserverRef, ref, motionRef, rootRef)} | ||
| className={cls} | ||
| style={ | ||
| { | ||
|
|
@@ -295,6 +348,7 @@ const Popup = React.forwardRef<HTMLDivElement, PopupProps>((props, ref) => { | |
| onPointerEnter={onPointerEnter} | ||
| onClick={onClick} | ||
| onPointerDownCapture={onPointerDownCapture} | ||
| onKeyDownCapture={onPopupKeyDownCapture} | ||
| > | ||
| {arrow && ( | ||
| <Arrow | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,91 @@ | ||||||||||||||||||||||||||||||||
| import type * as React from 'react'; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| const TABBABLE_SELECTOR = | ||||||||||||||||||||||||||||||||
| 'a[href], button, input, select, textarea, [tabindex]:not([tabindex="-1"])'; | ||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even that list still misses “tabbable” elements. See https://github.com/KittyGiraudel/focusable-selectors (which is still not exhaustive, but better than nothing). |
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| function isTabbable(el: HTMLElement, win: Window): boolean { | ||||||||||||||||||||||||||||||||
| if (el.closest('[aria-hidden="true"]')) { | ||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| if ('disabled' in el && (el as HTMLButtonElement).disabled) { | ||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Checking for the
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of doing that check in JavaScript, you can shift that to the selectors themselves in the |
||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||||||||||||||||||||||||||||
| if (el instanceof HTMLInputElement && el.type === 'hidden') { | ||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 使用全局 建议修正: 修复方案 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
Suggested change
🤖 Prompt for AI AgentsThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That also makes the cross-realm |
||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| const ti = el.getAttribute('tabindex'); | ||||||||||||||||||||||||||||||||
| if (ti !== null && Number(ti) < 0) { | ||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check can be moved into the selectors themselves to avoid having to do it in JavaScript. |
||||||||||||||||||||||||||||||||
| const style = win.getComputedStyle(el); | ||||||||||||||||||||||||||||||||
| if (style.display === 'none' || style.visibility === 'hidden') { | ||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Checking
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
)
}
Comment on lines
+45
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is redundant with
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| /** Visible, tabbable descendants inside `container` (in DOM order). */ | ||||||||||||||||||||||||||||||||
| export function getTabbableElements(container: HTMLElement): HTMLElement[] { | ||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 doc = container.ownerDocument; | ||||||||||||||||||||||||||||||||
| const win = doc.defaultView!; | ||||||||||||||||||||||||||||||||
| const nodeList = container.querySelectorAll<HTMLElement>(TABBABLE_SELECTOR); | ||||||||||||||||||||||||||||||||
| const list: HTMLElement[] = []; | ||||||||||||||||||||||||||||||||
| for (let i = 0; i < nodeList.length; i += 1) { | ||||||||||||||||||||||||||||||||
| const el = nodeList[i]; | ||||||||||||||||||||||||||||||||
| if (isTabbable(el, win)) { | ||||||||||||||||||||||||||||||||
| list.push(el); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| return list; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| export function focusPopupRootOrFirst( | ||||||||||||||||||||||||||||||||
| container: HTMLElement, | ||||||||||||||||||||||||||||||||
| ): HTMLElement | null { | ||||||||||||||||||||||||||||||||
| const tabbables = getTabbableElements(container); | ||||||||||||||||||||||||||||||||
| if (tabbables.length) { | ||||||||||||||||||||||||||||||||
| tabbables[0].focus(); | ||||||||||||||||||||||||||||||||
| return tabbables[0]; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| if (!container.hasAttribute('tabindex')) { | ||||||||||||||||||||||||||||||||
| container.setAttribute('tabindex', '-1'); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| container.focus(); | ||||||||||||||||||||||||||||||||
| return container; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| export function handlePopupTabTrap( | ||||||||||||||||||||||||||||||||
| e: React.KeyboardEvent, | ||||||||||||||||||||||||||||||||
| container: HTMLElement, | ||||||||||||||||||||||||||||||||
| ): void { | ||||||||||||||||||||||||||||||||
| if (e.key !== 'Tab' || e.defaultPrevented) { | ||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| const list = getTabbableElements(container); | ||||||||||||||||||||||||||||||||
| const active = document.activeElement as HTMLElement | null; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| if (!active || !container.contains(active)) { | ||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
Comment on lines
+151
to
+153
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should keep this guard. |
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| if (list.length === 0) { | ||||||||||||||||||||||||||||||||
| if (active === container) { | ||||||||||||||||||||||||||||||||
| e.preventDefault(); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| const first = list[0]; | ||||||||||||||||||||||||||||||||
| const last = list[list.length - 1]; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| if (!e.shiftKey) { | ||||||||||||||||||||||||||||||||
| if (active === last || active === container) { | ||||||||||||||||||||||||||||||||
| e.preventDefault(); | ||||||||||||||||||||||||||||||||
| first.focus(); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } else if (active === first || active === container) { | ||||||||||||||||||||||||||||||||
| e.preventDefault(); | ||||||||||||||||||||||||||||||||
| last.focus(); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.