fix: prevent aria props from being passed to wrapper div #1190
Conversation
|
@Pareder is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
Walkthrough该PR调整了Select组件中ARIA属性和role属性的分配方式。扩展BaseSelectProps以支持role属性，修改SelectInput的属性转发逻辑以避免直接在根元素上展开ARIA属性，并从上下文Props中移除role字段。同时添加测试验证ARIA属性正确传播到内部Input元素。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 |
Summary of ChangesHello @Pareder, 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 significantly enhances the accessibility of the 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. 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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1190 +/- ##
=======================================
Coverage 99.42% 99.42%
=======================================
Files 31 31
Lines 1216 1218 +2
Branches 433 433
=======================================
+ Hits 1209 1211 +2
Misses 7 7 ☔ 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 aims to improve accessibility by preventing ARIA attributes from being passed to the wrapper div and ensuring they are only applied to the input element. The changes are generally well-implemented, including updates to prop types, context, and the addition of tests to verify the new behavior. However, I've identified a potential issue where the role attribute might not be correctly filtered out from the wrapper div. My review includes a specific suggestion to address this.
|
|
||
| // ===================== Render ===================== | ||
| const domProps = omit(restProps, DEFAULT_OMIT_PROPS as any); | ||
| const ariaProps = pickAttrs(domProps, { aria: true }); |
There was a problem hiding this comment.
The pickAttrs utility with the { aria: true } option doesn't include the role attribute. This means role will not be added to ariaKeys and will not be omitted from the wrapper div, which contradicts the goal of this PR and the associated tests. To ensure role is also filtered out, you should include role: true in the pickAttrs options.
| const ariaProps = pickAttrs(domProps, { aria: true }); | |
| const ariaProps = pickAttrs(domProps, { aria: true, role: true }); |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/SelectInput/index.tsx (1)
220-229: 自定义根组件应该过滤 ARIA 属性以保持一致性在
RootComponent情况下（第 220-228 行），domProps被完整展开，包含了 ARIA 属性。然而，在主 div 元素上（第 234 行），代码明确使用omit(domProps, ariaKeys)来排除 ARIA 属性。这造成了不一致的行为：自定义根组件会接收 ARIA 属性，而默认 div 不会。根据第 211-212 行提取
ariaKeys的逻辑，应该在传递给RootComponent时也过滤掉 ARIA 属性，或者需要在代码中添加注释说明为什么要对自定义组件特殊处理。
🧹 Nitpick comments (1)
src/SelectInput/index.tsx (1)
211-212: 可选的代码清理建议
ariaProps变量被计算但未在代码中显式使用。虽然 ARIA 属性通过上下文正确传递到内部组件，但可以考虑添加注释说明为何计算此变量，或者如果不需要可以直接使用pickAttrs的返回值提取 keys：🔎 可选的重构建议
- const ariaProps = pickAttrs(domProps, { aria: true }); - const ariaKeys = Object.keys(ariaProps) as (keyof typeof domProps)[]; + // Extract ARIA attribute keys to omit from root element + const ariaKeys = Object.keys(pickAttrs(domProps, { aria: true })) as (keyof typeof domProps)[];
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/__snapshots__/Select.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
src/BaseSelect/index.tsxsrc/SelectInput/index.tsxsrc/hooks/useBaseProps.tstests/Accessibility.test.tsxtests/Select.test.tsx
💤 Files with no reviewable changes (1)
- src/hooks/useBaseProps.ts
🔇 Additional comments (3)
src/SelectInput/index.tsx (1)
14-14: LGTM!正确引入了
pickAttrs工具函数用于提取 ARIA 属性。tests/Select.test.tsx (1)
139-146: LGTM!移除了
as any类型断言是一个好的改进，说明类型系统现在正确支持role属性。这与BaseSelectProps扩展Pick<React.HTMLAttributes<HTMLElement>, 'role'>的更改保持一致。src/BaseSelect/index.tsx (1)
132-136: LGTM!通过扩展
Pick<React.HTMLAttributes<HTMLElement>, 'role'>正确地将role属性添加到公共 API。这使得组件可以通过标准 HTML 属性接收 role，与 ARIA 属性处理保持一致。
| it('should pass aria and role attributes to Input component', async () => { | ||
| const { container } = render( | ||
| <Select | ||
| role="group" | ||
| aria-required="true" | ||
| options={[ | ||
| { | ||
| value: '123', | ||
| }, | ||
| { | ||
| value: '1234', | ||
| }, | ||
| { | ||
| value: '12345', | ||
| }, | ||
| ]} | ||
| />, | ||
| ); | ||
|
|
||
| const wrapper = container.querySelector('.rc-select'); | ||
| expect(wrapper).not.toHaveAttribute('role'); | ||
| expect(wrapper).not.toHaveAttribute('aria-required'); | ||
|
|
||
| const input = container.querySelector('input'); | ||
| expect(input).toHaveAttribute('role', 'group'); | ||
| expect(input).toHaveAttribute('aria-required', 'true'); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
WAI-ARIA Select component appropriate role combobox listbox
💡 Result:
Short answer
- Use role="combobox" on the control when you have a single control that opens a popup for choosing a value (editable or select‑only). The popup element should use role="listbox" (or tree/grid/dialog if appropriate) and each item uses role="option". Manage aria-expanded, aria-controls, aria-activedescendant, and aria-autocomplete as required. [1][2]
When to use listbox without combobox
- If the UI is simply a persistent list of options (not a single control that opens/closes a popup), use role="listbox" for the list and role="option" for items — no combobox required. [3]
Key ARIA requirements (summary)
- combobox element: role="combobox", aria-expanded (true/false), aria-controls → id of popup, aria-haspopup defaults to "listbox". [1][2]
- popup: role="listbox" and children role="option"; when focus remains on the combobox use aria-activedescendant to indicate the highlighted option. [1][2]
- If editable/autocomplete, set aria-autocomplete on the combobox/textbox. [1]
References
- WAI-ARIA 1.2 (combobox role & structural rules). [1]
- WAI-ARIA Authoring Practices — Combobox pattern & examples. [2]
- MDN — listbox role guidance. [3]
🏁 Script executed:
# Find and examine the Select component implementation around line 265
find . -type f -name "*.tsx" -o -name "*.ts" | head -20Repository: react-component/select
Length of output: 716
🏁 Script executed:
# Search for the Select component implementation to understand default role
rg -t tsx -t ts "role.*combobox|role.*group" --context 3Repository: react-component/select
Length of output: 93
🏁 Script executed:
# Look for the test file to understand the full context
find . -path "*/tests/*Accessibility*" -type fRepository: react-component/select
Length of output: 96
🏁 Script executed:
# Examine the Accessibility test file around line 265
cat -n ./tests/Accessibility.test.tsx | sed -n '260,280p'Repository: react-component/select
Length of output: 936
🏁 Script executed:
# Search for Select component implementation and default role
rg "role.*=.*combobox|role.*=.*group" --context 2Repository: react-component/select
Length of output: 3100
🏁 Script executed:
# Look for the main Select component file
find . -type f -name "*.tsx" -o -name "*.ts" | grep -i select | head -10Repository: react-component/select
Length of output: 392
🏁 Script executed:
# Search for role attribute assignments in the codebase
rg "role\s*=" --context 2 | head -50Repository: react-component/select
Length of output: 2732
测试验证了属性传播机制，但使用的 ARIA role 不符合无障碍标准
新测试用例正确验证了属性在 Select 组件中的传播行为：包装器 div 不接收 role 或 aria-required，而内部 input 元素接收这些属性。
但根据 WAI-ARIA 规范，Select 组件的 input 元素应使用 role="combobox"，而非 role="group"。role="group" 通常用于分组相关元素，并不适合 Select 组件（见 line 265 的默认实现）。建议使用 role="combobox" 重写该测试，以遵循无障碍最佳实践。
🤖 Prompt for AI Agents
In tests/Accessibility.test.tsx around lines 217 to 243, the test uses
role="group" which is incorrect for a Select input; update the test to pass
role="combobox" into the Select props and change the assertions to expect the
internal input to have role "combobox" (while still asserting the wrapper does
not have role or aria-required), so the test aligns with the component's default
ARIA role and accessibility expectations.
1 participant
Do not pass aria attributes to wrapper div as they are already passes to correct
inputcomponent (https://github.com/react-component/select/blob/master/src/SelectInput/Content/index.tsx#L17). It improves the overall accessibility.Closes ant-design/ant-design#46030
Summary by CodeRabbit
发布说明
改进
测试
✏️ Tip: You can customize this high-level summary in your review settings.