feat: add ignored element support for focus lock #727
Conversation
Add ability to mark elements as ignored during focus lock, allowing temporary focus on elements outside the locked area. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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 enhancement to the focus lock mechanism by providing a way to temporarily bypass the focus restriction for designated elements. This feature addresses scenarios where certain interactive components, such as tooltips or modals, need to receive focus even when a primary focus lock is active, thereby improving the flexibility and user experience of applications utilizing focus management. 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. Changelog
Activity
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
|
|
Caution Review failedThe pull request is closed. Walkthrough在焦点锁定逻辑中新增按锁（id）跟踪的“被忽略元素”映射与检测：引入 ignoreElement API 以在当前锁定上下文中标记元素为被忽略；syncFocus 在检测到被忽略元素时提前返回；在注销/teardown 时清理映射；useLockFocus 返回元组形式以暴露 ignoreElement 回调。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
庆祝诗
🚥 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #727 +/- ##
==========================================
+ Coverage 85.98% 86.19% +0.21%
==========================================
Files 38 38
Lines 1020 1043 +23
Branches 380 370 -10
==========================================
+ Hits 877 899 +22
- Misses 141 142 +1
Partials 2 2 ☔ 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}
There was a problem hiding this comment.
Code Review
This pull request introduces support for ignoring specific elements within the focus lock mechanism, which is a valuable addition. The implementation is generally sound, introducing an ignoredElementMap to track elements that should be exempt from the focus trap and exposing a function via the useLockFocus hook to manage this. My review includes one suggestion to enhance type safety when dealing with document.activeElement, which will make the new logic more robust.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
![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
🤖 Fix all issues with AI agents
In `@src/Dom/focus.ts`:
- Around line 210-217: 当前实现中如果目标元素先获得焦点，syncFocus（触发于 focusin）会在 ignoreElement
被调用前将焦点拉回锁定区域，导致 ignoreElement 时序问题；请在 ignoreElement 的
JSDoc（或模块顶层使用文档）明确说明必须在触发焦点转移（例如在调用 element.focus() 之前）先调用
ignoreElement(ele)，并在文档中引用相关符号 ignoreElement、syncFocus、ignoredElementMap 和
getElement，以便调用方按正确顺序使用这些 API；如需额外保障，可在 ignoreElement 或 syncFocus
中加入运行时断言或警告（可选），但主要要求是更新注释/文档以声明调用时序约束。
🧹 Nitpick comments (3)
src/Dom/focus.ts (3)
105-105: 值类型HTMLElement | null中的| null是多余的。
ignoreElement（第 212 行）在ele为真时才会调用set，因此 map 中永远不会存储null值。建议将类型简化为Map<HTMLElement, HTMLElement>，使意图更清晰。♻️ 建议修改
-const ignoredElementMap = new Map<HTMLElement, HTMLElement | null>(); +const ignoredElementMap = new Map<HTMLElement, HTMLElement>();
210-217:ignoreElement每次渲染都会生成新的引用，建议使用useCallback包裹。当前
ignoreElement在每次渲染时都会创建一个新的函数引用。如果调用方将其作为 prop 传递或放入useEffect的依赖数组中，会导致不必要的重新渲染或 effect 重新执行。♻️ 建议使用 useCallback
+import { useCallback, useEffect } from 'react'; - const ignoreElement = (ele: HTMLElement) => { - const element = getElement(); - if (element && ele) { - // Set the ignored element for current lock element - // Only one element can be ignored at a time for this lock - ignoredElementMap.set(element, ele); - } - }; + const ignoreElement = useCallback((ele: HTMLElement) => { + const element = getElement(); + if (element && ele) { + ignoredElementMap.set(element, ele); + } + }, [getElement]);
195-219: 缺少取消忽略（un-ignore）的 API。一旦调用
ignoreElement(ele)，该元素将一直处于被忽略状态，直到焦点锁释放。如果被忽略的元素（如弹出层）关闭但仍保留在 DOM 中（如通过display: none以外的方式隐藏），焦点锁对该元素的豁免会持续生效，可能导致焦点意外逸出锁定区域。建议考虑以下方案之一：
- 让
ignoreElement返回一个清除函数，用于取消忽略状态。- 提供
ignoreElement(null)的语义来主动清除当前被忽略的元素。♻️ 方案 2 示例
- const ignoreElement = (ele: HTMLElement) => { - const element = getElement(); - if (element && ele) { - // Set the ignored element for current lock element - // Only one element can be ignored at a time for this lock - ignoredElementMap.set(element, ele); - } - }; + const ignoreElement = (ele: HTMLElement | null) => { + const element = getElement(); + if (element) { + if (ele) { + ignoredElementMap.set(element, ele); + } else { + ignoredElementMap.delete(element); + } + } + };
Add a simple test to verify that ignoreElement allows focus on ignored elements outside the locked area. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use useId from React hooks as a stable key instead of element reference for the ignoredElementMap. This prevents issues where element reference changes (e.g., during component re-renders) cause the ignore functionality to break. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Change from elementToIdMap to idToElementMap for clearer logic: stable ID as key maps to element. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Simplify the isIgnoredElement function for better readability. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Dom/focus.ts`:
- Around line 235-242: The ignoreElement function is recreated on every render
which causes unstable references; wrap it in React's useCallback to memoize it
(e.g. const ignoreElement = useCallback((ele: HTMLElement) => { if (ele) {
ignoredElementMap.set(id, ele); } }, [id])) and ensure useCallback is imported;
this will keep the returned [ignoreElement] stable for consumers and effects
while still using the stable id from useId.
🧹 Nitpick comments (3)
src/Dom/focus.ts (3)
106-109: 每个锁 ID 只能存储一个被忽略元素，多次调用会静默覆盖。
ignoredElementMap是Map<string, HTMLElement | null>结构，每个锁 ID 只能关联一个被忽略的元素。如果消费者对同一个锁调用多次ignoreElement(eleA)再ignoreElement(eleB)，eleA会被静默覆盖。如果业务场景需要同时忽略多个不相关的元素（如多个 popup），当前设计将无法满足。如果确认只需要支持单个元素，建议在 JSDoc 中注明；否则可考虑改用
Map<string, Set<HTMLElement>>。另外，
HTMLElement | null中的| null是多余的——ignoreElement中if (ele)已过滤掉null，且Map.get()对不存在的 key 返回undefined。♻️ 建议简化类型
-const ignoredElementMap = new Map<string, HTMLElement | null>(); +const ignoredElementMap = new Map<string, HTMLElement>();
115-135: 反向查找抵消了idToElementMap重构的优势。
isIgnoredElement遍历idToElementMap的所有条目，通过元素引用比较找到对应的lockId。这本质上是一次 element → id 的反向查找，而 commit message 提到重构的目的正是"避免依赖元素引用"。建议将
focusElements从HTMLElement[]改为存储{ element, id }的结构，或者直接维护一个elementToIdMap反向映射，这样isIgnoredElement可以 O(1) 查找，逻辑也更清晰。♻️ 方案示例：使用带 id 的栈结构
-let focusElements: HTMLElement[] = []; +let focusElements: { element: HTMLElement; id: string }[] = []; function getLastElement() { - return focusElements[focusElements.length - 1]; + return focusElements[focusElements.length - 1]?.element; +} + +function getLastLockId() { + return focusElements[focusElements.length - 1]?.id; } function isIgnoredElement(element: Element | null): boolean { if (!element) return false; - const lastElement = getLastElement(); - if (!lastElement) return false; - - // Find the ID that maps to the last element - let lockId: string | undefined; - for (const [id, ele] of idToElementMap.entries()) { - if (ele === lastElement) { - lockId = id; - break; - } - } - - if (!lockId) return false; + const lockId = getLastLockId(); + if (!lockId) return false; const ignoredEle = ignoredElementMap.get(lockId); return ( !!ignoredEle && (ignoredEle === element || ignoredEle.contains(element)) ); }
226-233:getElement未纳入useEffect依赖数组。如果
getElement返回的元素在lock和id未变的情况下发生变化，useEffect不会重新执行，焦点锁定仍作用于旧元素。这可能是延续既有模式（将
getElement视为稳定的 ref 回调），但如果有动态切换容器元素的场景，需注意此限制。
| const ignoreElement = (ele: HTMLElement) => { | ||
| if (ele) { | ||
| // Set the ignored element using stable ID | ||
| ignoredElementMap.set(id, ele); | ||
| } | ||
| }; | ||
|
|
||
| return [ignoreElement]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
ignoreElement 未使用 useCallback 包裹，每次渲染生成新引用。
ignoreElement 在每次渲染时都会创建新的函数实例。如果消费者将其传递给子组件的 props 或用于 useEffect 的依赖数组，会导致不必要的重渲染或副作用重新执行。
由于 id（来自 useId）是稳定的，用 useCallback 包裹后引用也将保持稳定。
♻️ 建议使用 useCallback
+import { useEffect, useCallback } from 'react';
-import { useEffect } from 'react';- const ignoreElement = (ele: HTMLElement) => {
- if (ele) {
- // Set the ignored element using stable ID
- ignoredElementMap.set(id, ele);
- }
- };
+ const ignoreElement = useCallback((ele: HTMLElement) => {
+ if (ele) {
+ ignoredElementMap.set(id, ele);
+ }
+ }, [id]);🤖 Prompt for AI Agents
In `@src/Dom/focus.ts` around lines 235 - 242, The ignoreElement function is
recreated on every render which causes unstable references; wrap it in React's
useCallback to memoize it (e.g. const ignoreElement = useCallback((ele:
HTMLElement) => { if (ele) { ignoredElementMap.set(id, ele); } }, [id])) and
ensure useCallback is imported; this will keep the returned [ignoreElement]
stable for consumers and effects while still using the stable id from useId.
1 participant
Add ability to mark elements as ignored during focus lock, allowing temporary focus on elements outside the locked area.
ref ant-design/ant-design#56782
Summary by CodeRabbit
新功能
变更
测试