feat: better a11y #509
feat: better a11y #509
Conversation
|
@aojunhao123 is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
Summary of ChangesHello @aojunhao123, 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 integrates Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough修改测试依赖与 TypeScript 类型；重构 Tooltip 的挂载与可见性管理（引入 Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Trigger
participant Tooltip
participant Popup
rect rgba(0,128,128,0.06)
User->>Trigger: hover / focus / click
Trigger->>Tooltip: onOpenChange(visible)
end
rect rgba(128,128,0,0.04)
Tooltip->>Tooltip: handleVisibleChange(visible)\n更新 mergedVisible
Tooltip->>Tooltip: 若 visible 或 forceRender -> popupMounted = true
Tooltip->>Trigger: 透传 forceRender 与 onOpenChange
end
rect rgba(0,64,128,0.06)
Tooltip->>Popup: 根据 popupMounted 挂载 Popup
Popup-->>Tooltip: 已挂载
Tooltip->>Trigger: 仅当 overlay 存在且 popupMounted 时设置 aria-describedby
end
alt 隐藏且 destroyOnHidden
Trigger->>Tooltip: onOpenChange(false)
Tooltip->>Tooltip: mergedVisible = false\n若 destroyOnHidden -> popupMounted = false
Tooltip->>Trigger: 移除 aria-describedby
else 隐藏但不销毁
Trigger->>Tooltip: onOpenChange(false)
Tooltip->>Tooltip: mergedVisible = false\npopupMounted 保留
Tooltip->>Trigger: 保持 aria-describedby
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🪛 markdownlint-cli2 (0.18.1)README.md14-14: Link and image reference definitions should be needed (MD053, link-image-reference-definitions) 15-15: Link and image reference definitions should be needed (MD053, link-image-reference-definitions) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
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 |
![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 correctly adds @testing-library/jest-dom as a dev dependency. However, the accompanying change to tsconfig.json will likely break your build by excluding necessary type definitions for Jest and Node.js. I've left a critical comment with a suggested fix on that file.
Additionally, please remember to import @testing-library/jest-dom in your test setup file (tests/setup.js) to make the custom matchers available at runtime. Without this step, your tests will fail.
| "declaration": true, | ||
| "skipLibCheck": true, | ||
| "esModuleInterop": true, | ||
| "types": ["@testing-library/jest-dom"], |
There was a problem hiding this comment.
By adding the types property, you override TypeScript's default type inclusion. This means that other ambient type definitions, like for Jest (jest) and Node.js (node), will no longer be automatically included, which will likely break your build. You need to explicitly list all required ambient types.1
| "types": ["@testing-library/jest-dom"], | |
| "types": ["jest", "node", "@testing-library/jest-dom"], |
Rules References
Footnotes
-
When using the
compilerOptions.typesproperty intsconfig.json, TypeScript will only include the type definitions for the packages explicitly listed in the array. This overrides the default behavior of automatically including all packages found innode_modules/@types. Therefore, you must list all necessary type definitions, such asjestandnode, to avoid compilation errors. ↩
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #509 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 37 62 +25
Branches 15 21 +6
=========================================
+ Hits 37 62 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
![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
🧹 Nitpick comments (1)
src/Tooltip.tsx (1)
63-63: 建议在数组元素之间添加空格数组字面量中逗号后缺少空格。建议改为
['hover', 'focus']以保持代码风格一致性。应用此 diff:
- trigger = ['hover','focus'], + trigger = ['hover', 'focus'],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Tooltip.tsx(5 hunks)tests/index.test.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: thinkasany
Repo: react-component/tooltip PR: 484
File: src/Popup.tsx:20-25
Timestamp: 2024-11-28T13:00:19.052Z
Learning: `overlayInnerClassName` 属性在 `Popup.tsx` 中是供内部使用的，已经通过 `classNames.inner` 在现有的测试用例中得到验证，无需单独添加测试。
📚 Learning: 2024-11-28T13:00:19.052Z
Learnt from: thinkasany
Repo: react-component/tooltip PR: 484
File: src/Popup.tsx:20-25
Timestamp: 2024-11-28T13:00:19.052Z
Learning: `overlayInnerClassName` 属性在 `Popup.tsx` 中是供内部使用的，已经通过 `classNames.inner` 在现有的测试用例中得到验证，无需单独添加测试。
Applied to files:
tests/index.test.tsxsrc/Tooltip.tsx
🔇 Additional comments (6)
src/Tooltip.tsx (3)
97-107: LGTM! popupMounted 初始状态逻辑合理初始状态正确处理了 forceRender、受控模式和 defaultVisible 三种场景，确保弹窗在应该挂载时被挂载。
167-167: LGTM! aria-describedby 应用逻辑正确只在 overlay 存在且弹窗已挂载时设置
aria-describedby,符合无障碍最佳实践。确保修复上述popupMounted状态管理的问题,以保证此逻辑正确工作。
194-194: LGTM! Trigger 属性配置正确
onOpenChange使用handleVisibleChange包装器确保popupMounted状态同步,forceRender的显式传递保证了渲染行为的一致性。Also applies to: 198-198
tests/index.test.tsx (3)
505-523: LGTM! 测试正确验证挂载后的 aria-describedby 行为测试现在正确等待弹窗挂载后再检查
aria-describedby属性,并验证了在默认destroyOnHidden={false}时属性会持久保留。这与新的popupMounted状态管理逻辑一致。
535-543: LGTM! 新增测试覆盖了立即挂载场景新增的两个测试用例正确验证了
defaultVisible和forceRender场景下aria-describedby的立即应用,确保初始挂载时的无障碍属性正确设置。Also applies to: 545-553
555-572: 测试覆盖了销毁场景,但缺少受控模式测试此测试正确验证了
destroyOnHidden={true}时aria-describedby的移除行为。但是,测试套件缺少对受控模式下destroyOnHidden={false}的测试用例,这正是src/Tooltip.tsx中 effect 逻辑存在问题的场景。建议添加以下测试用例以覆盖受控模式:
it('should keep aria-describedby in controlled mode when destroyOnHidden is false', async () => { const App = () => { const [visible, setVisible] = React.useState(false); return ( <> <button onClick={() => setVisible(!visible)}>Toggle</button> <Tooltip visible={visible} destroyOnHidden={false} overlay="tooltip content"> <button>Target</button> </Tooltip> </> ); }; const { container } = render(<App />); const target = container.querySelectorAll('button')[1]; const toggle = container.querySelectorAll('button')[0]; expect(target).not.toHaveAttribute('aria-describedby'); fireEvent.click(toggle); await waitFakeTimers(); expect(target).toHaveAttribute('aria-describedby'); fireEvent.click(toggle); await waitFakeTimers(); // Should keep aria-describedby because destroyOnHidden is false expect(target).toHaveAttribute('aria-describedby'); });
aojunhao123
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Tooltip.tsx (1)
97-107:popupMounted状态机整体合理，但需评估动态切换destroyOnHidden的边界场景这一段通过
popupMounted+updatePopupMounted+handleVisibleChange+useEffect将「是否挂载」与「是否可见」解耦，并统一考虑了forceRender、受控/非受控模式以及destroyOnHidden，可以修复之前在受控模式下destroyOnHidden={false}时aria-describedby被过早移除的问题，设计是合理的。不过有一个相对边缘的场景可以评估一下是否需要覆盖：
- 在非受控模式下（不传
visible），Tooltip 已经被展示过一次并关闭，此时popupMounted会保持为true（destroyOnHidden={false}时符合预期）。- 如果这时父组件把
destroyOnHidden从false改为true，Trigger很可能会在关闭状态下直接销毁 DOM，但当前逻辑中：
isControlled为false，useEffect分支不会再调用updatePopupMounted；popupMounted仍然保持为true，从而aria-describedby依旧指向已经被移除的节点。如果你们希望支持运行时切换
destroyOnHidden，建议进一步梳理这一场景下popupMounted与实际 DOM 的同步策略（例如基于 Trigger 的可见性回调或额外信号），否则可以在文档中说明该属性主要用于初始配置，不建议在运行时频繁切换。整体而言，这一块逻辑已经明显优于旧实现，上述只是边界行为上的一个建议。
Also applies to: 109-124, 126-140, 194-199
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Tooltip.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: thinkasany
Repo: react-component/tooltip PR: 484
File: src/Popup.tsx:20-25
Timestamp: 2024-11-28T13:00:19.052Z
Learning: `overlayInnerClassName` 属性在 `Popup.tsx` 中是供内部使用的，已经通过 `classNames.inner` 在现有的测试用例中得到验证，无需单独添加测试。
📚 Learning: 2024-11-28T13:00:19.052Z
Learnt from: thinkasany
Repo: react-component/tooltip PR: 484
File: src/Popup.tsx:20-25
Timestamp: 2024-11-28T13:00:19.052Z
Learning: `overlayInnerClassName` 属性在 `Popup.tsx` 中是供内部使用的，已经通过 `classNames.inner` 在现有的测试用例中得到验证，无需单独添加测试。
Applied to files:
src/Tooltip.tsx
🔇 Additional comments (3)
src/Tooltip.tsx (3)
7-7: React hooks 引入合理
useImperativeHandle、useRef、useEffect、useCallback都在组件中有实际使用，没有多余或遗漏，保持当前写法即可。
63-83: 默认触发改为['hover', 'focus']有助于键盘无障碍默认
trigger从单一hover扩展为['hover', 'focus']，让仅通过键盘focus的用户也能触发 Tooltip，属于向后兼容且更友好的改动；forceRender的解构与后续透传逻辑也一致，没有发现问题。
162-170: 仅在 Tooltip DOM 已挂载时设置aria-describedby
'aria-describedby': overlay && popupMounted ? mergedId : null确保只有在有overlay且实际已经挂载（popupMounted为真）时才给子元素添加aria-describedby，可以避免 ARIA 指向不存在的节点，是对无障碍行为的正确修复。
src/Tooltip.tsx
Outdated
| const Tooltip = React.forwardRef<TooltipRef, TooltipProps>((props, ref) => { | ||
| const { | ||
| trigger = ['hover'], | ||
| trigger = ['hover', 'focus'], |
There was a problem hiding this comment.
这是一个 break change，不能这么做。应该是如果没有可见 Tooltip 就不要添加 aria-describedby 属性。然后文档 FAQ 里补充一下说明如果需要键盘无障碍可以添加 focus trigger（并且 ConfigProvider 支持修改这个 tigger 默认值）。
它无障碍检测说有问题是因为现在还没有渲染，去掉就可以了。
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
98-114: 可选：增强辅助功能文档以反映自动 aria-describedby 处理。当前文档展示的是手动为 input 元素添加
aria-describedby的模式。根据本 PR 所做的改进，Tooltip 组件现已在 overlay 挂载时自动应用aria-describedby。考虑更新文档以说明：
- 在何时自动应用 aria-describedby（当 overlay 已挂载时）
- 开发者仍然可以手动控制（例如通过
idprop）这样可以帮助用户更好地理解新的行为。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(4 hunks)src/Tooltip.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Tooltip.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: thinkasany
Repo: react-component/tooltip PR: 484
File: src/Popup.tsx:20-25
Timestamp: 2024-11-28T13:00:19.052Z
Learning: `overlayInnerClassName` 属性在 `Popup.tsx` 中是供内部使用的，已经通过 `classNames.inner` 在现有的测试用例中得到验证，无需单独添加测试。
🪛 markdownlint-cli2 (0.18.1)
README.md
14-14: Link and image reference definitions should be needed
Unused link or image reference definition: "travis-image"
(MD053, link-image-reference-definitions)
15-15: Link and image reference definitions should be needed
Unused link or image reference definition: "travis-url"
(MD053, link-image-reference-definitions)
🔇 Additional comments (1)
README.md (1)
1-1: ✓ 包名更新一致且完整。从 rc-tooltip 到 @rc-component/tooltip 的包名替换已在整个文档中系统地应用，包括标题、徽章链接、导入语句和资源路径，确保文档的一致性和准确性。
Also applies to: 12-23, 39-39, 44-44, 50-50
README.md
Outdated
| [travis-image]: https://img.shields.io/travis/@rc-component/tooltip/master?style=flat-square | ||
| [travis-url]: https://travis-ci.com/@rc-component/tooltip |
There was a problem hiding this comment.
移除未使用的链接引用定义。
静态分析工具检测到 travis-image 和 travis-url 这两个链接引用定义在文档中未被使用。由于 CI 流程已从 Travis 迁移到 GitHub Actions（第 16-17 行），这些引用应该被移除以保持 markdown 的整洁。
应用以下修改移除未使用的引用：
-[travis-image]: https://img.shields.io/travis/@rc-component/tooltip/master?style=flat-square
-[travis-url]: https://travis-ci.com/@rc-component/tooltip
[github-actions-image]: https://github.com/@rc-component/tooltip/actions/workflows/react-component-ci.yml/badge.svg📝 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.
| [travis-image]: https://img.shields.io/travis/@rc-component/tooltip/master?style=flat-square | |
| [travis-url]: https://travis-ci.com/@rc-component/tooltip |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
14-14: Link and image reference definitions should be needed
Unused link or image reference definition: "travis-image"
(MD053, link-image-reference-definitions)
15-15: Link and image reference definitions should be needed
Unused link or image reference definition: "travis-url"
(MD053, link-image-reference-definitions)
🤖 Prompt for AI Agents
In README.md around lines 14 to 15, the link reference definitions
[travis-image] and [travis-url] are unused and should be removed; delete those
two reference lines so the README no longer contains obsolete Travis CI badge
references and ensure no other part of the file depends on them.
2 participants
Summary by CodeRabbit
新功能
测试
Chores
文档
✏️ Tip: You can customize this high-level summary in your review settings.