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新增测试库类型依赖；Tooltip 将 Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Trigger
participant Tooltip
participant Popup
User->>Trigger: hover / focus / click
Trigger->>Tooltip: onOpenChange(visible)
Tooltip->>Tooltip: 更新 mergedVisible / innerVisible
Tooltip->>Tooltip: 决定 popupMounted（基于 visible/forceRender/destroyOnHidden）
alt popupMounted 或 visible
Tooltip->>Popup: 挂载并渲染 overlay
Popup-->>Tooltip: overlay 已存在
Tooltip->>Trigger: 克隆子节点并注入 aria-describedby（仅在 overlay 存在且可见）
else overlay 不存在或不可见
Tooltip->>Trigger: 确保移除 aria-describedby
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
![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 36 -1
Branches 15 14 -1
=========================================
- Hits 37 36 -1 ☔ 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 指向不存在的节点，是对无障碍行为的正确修复。
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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Tooltip.tsx (1)
82-82: 移除 forceRender 属性传递以优化性能。根据维护者 zombieJ 的反馈："不需要这个，直接根据是否 visible 加就行了。节省性能。"
forceRender属性的传递是不必要的，应该基于可见性状态来渲染，这样可以节省性能开销。应用此 diff 移除 forceRender 相关代码：
styles, - forceRender, ...restPropspopupMotion={motion} defaultPopupVisible={defaultVisible} - forceRender={forceRender} autoDestroy={destroyOnHidden}基于过往学习，这符合维护者对性能优化的要求。
Also applies to: 165-165
📜 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)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/index.test.tsx
🧰 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 (4)
src/Tooltip.tsx (4)
7-7: LGTM！引入
useState用于管理内部可见性状态，这是支持受控/非受控模式所必需的。
93-107: 受控/非受控模式实现正确。可见性状态管理逻辑实现符合标准模式：
- 通过
'visible' in props判断受控模式innerVisible管理非受控状态mergedVisible作为统一的真实值来源handleVisibleChange正确处理状态更新和回调传播
134-134: 无障碍属性实现符合维护者反馈。
aria-describedby仅在overlay存在且mergedVisible为 true 时添加，这确保：
- 只有在有内容需要描述时才添加该属性
- 引用的 ID 在 DOM 中确实存在（避免引用已销毁的元素）
这与 zombieJ 的反馈一致："如果没有可见 Tooltip 就不要添加
aria-describedby属性"，并正确解决了链接问题中提到的无障碍问题。
161-161: LGTM！将
onOpenChange连接到handleVisibleChange确保了 Trigger 的可见性变化能正确同步到组件的内部状态，并触发用户提供的回调。
src/Tooltip.tsx
Outdated
| extraProps.popupVisible = props.visible; | ||
| } | ||
|
|
||
| const [innerVisible, setInnerVisible] = useState(() => defaultVisible || false); |
There was a problem hiding this comment.
还是有点浪费，多了一个 state update 就多了一个 re-render。我去给 trigger 的 renderProps 加一个 info 好了
src/Tooltip.tsx
Outdated
| extraProps.popupVisible = props.visible; | ||
| } | ||
|
|
||
| const handleVisibleChange = (nextVisible: boolean) => { |
src/Tooltip.tsx
Outdated
| {...extraProps} | ||
| > | ||
| {getChildren()} | ||
| {({ open }) => getChildren(open)} |
There was a problem hiding this comment.
这里不用额外的函数，因为本来就支持 renderProps 了
src/Tooltip.tsx
Outdated
| afterOpenChange={afterVisibleChange} | ||
| popupMotion={motion} | ||
| defaultPopupVisible={defaultVisible} | ||
| forceRender={forceRender} |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Tooltip.tsx(3 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)
25-25: LGTM - 显式 children 类型提升了 API 清晰度。将
children从TriggerProps的 Pick 中移除并作为显式必需 prop，配合 Line 117 的React.Children.only()强制单一子元素，使类型约束更加明确。
119-119: 条件逻辑正确实现了按可见性控制 aria-describedby。仅在
overlay存在且open为 true 时添加aria-describedby，符合 zombieJ 提出的性能优化建议和无障碍最佳实践。不可见时使用undefined可避免渲染空属性。
159-159: Render prop pattern is correctly implemented and fully supported by @rc-component/trigger v3.7.1.The code correctly passes
getChildrenas a render prop function to Trigger. The@rc-component/triggerv3.x library (including v3.7.1) supports passing the{ open }parameter to children render functions, sogetChildrenwill receive the current open state when invoked by Trigger. The pattern is compatible and follows the render prop convention properly.
| const getChildren: TriggerProps['children'] = ({ open }) => { | ||
| const child = React.Children.only(children); | ||
| const originalProps = child?.props || {}; | ||
| const childProps = { | ||
| ...originalProps, | ||
| 'aria-describedby': overlay ? mergedId : null, | ||
| const ariaProps: React.AriaAttributes = { | ||
| 'aria-describedby': overlay && open ? mergedId : undefined, | ||
| }; | ||
| return React.cloneElement<any>(children, childProps) as any; | ||
| return React.cloneElement(child, ariaProps); | ||
| }; |
There was a problem hiding this comment.
修复 aria-describedby 属性覆盖问题。
React.cloneElement(child, ariaProps) 会直接覆盖子元素已有的 aria-describedby 属性，破坏子元素自身的无障碍关联关系。根据 WAI-ARIA 规范，应使用空格分隔的 ID 列表合并多个描述源。
应用此 diff 以正确合并 aria-describedby：
const getChildren: TriggerProps['children'] = ({ open }) => {
const child = React.Children.only(children);
+ const existingDescribedBy = child.props['aria-describedby'];
+ const tooltipDescribedBy = overlay && open ? mergedId : undefined;
+ const mergedDescribedBy =
+ tooltipDescribedBy && existingDescribedBy
+ ? `${existingDescribedBy} ${tooltipDescribedBy}`
+ : tooltipDescribedBy || existingDescribedBy;
const ariaProps: React.AriaAttributes = {
- 'aria-describedby': overlay && open ? mergedId : undefined,
+ 'aria-describedby': mergedDescribedBy,
};
return React.cloneElement(child, ariaProps);
};📝 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.
| const getChildren: TriggerProps['children'] = ({ open }) => { | |
| const child = React.Children.only(children); | |
| const originalProps = child?.props || {}; | |
| const childProps = { | |
| ...originalProps, | |
| 'aria-describedby': overlay ? mergedId : null, | |
| const ariaProps: React.AriaAttributes = { | |
| 'aria-describedby': overlay && open ? mergedId : undefined, | |
| }; | |
| return React.cloneElement<any>(children, childProps) as any; | |
| return React.cloneElement(child, ariaProps); | |
| }; | |
| const getChildren: TriggerProps['children'] = ({ open }) => { | |
| const child = React.Children.only(children); | |
| const existingDescribedBy = child.props['aria-describedby']; | |
| const tooltipDescribedBy = overlay && open ? mergedId : undefined; | |
| const mergedDescribedBy = | |
| tooltipDescribedBy && existingDescribedBy | |
| ? `${existingDescribedBy} ${tooltipDescribedBy}` | |
| : tooltipDescribedBy || existingDescribedBy; | |
| const ariaProps: React.AriaAttributes = { | |
| 'aria-describedby': mergedDescribedBy, | |
| }; | |
| return React.cloneElement(child, ariaProps); | |
| }; |
🤖 Prompt for AI Agents
In src/Tooltip.tsx around lines 116 to 122, React.cloneElement(child, ariaProps)
currently overwrites any existing child aria-describedby; instead read the
child's existing aria-describedby (child.props &&
child.props['aria-describedby']), and if overlay && open and mergedId exist,
merge them by space-separating the existing value and mergedId (deduplicate if
present), then pass the combined string (or undefined if empty) into
cloneElement so existing aria-describedby values are preserved and multiple IDs
follow WAI-ARIA space-separated list rules.
2 participants
Summary by CodeRabbit
新功能
测试
Chores
文档
✏️ Tip: You can customize this high-level summary in your review settings.