fix: option position announcement for virtual mode#1226
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. |
功能概览该 PR 改进了 Select 组件虚拟列表模式下的屏幕阅读器无障碍通报。通过新增选项位置计数、更新 ARIA 属性（aria-setsize、aria-posinset）、重构隐藏项分组渲染，使虚拟化选项的位置序号与总数能正确通报给屏幕阅读器，同时支持分组选项的嵌套结构。 改动详情虚拟列表无障碍属性与分组渲染
🎯 3 (Moderate) | ⏱️ ~25 分钟 相关 PR
建议审查人
兔子之诗 🐰
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 improves accessibility in virtualized select lists by adding aria-setsize and aria-posinset attributes to options (skipping group headers) and nesting options inside role="group" wrappers to match native <select> announcements. Corresponding unit and snapshot tests have been added or updated. Feedback is provided regarding a performance issue in src/OptionList.tsx, where calculating option counts and positions on every render performs O(N) operations; it is recommended to precompute these values using React.useMemo to achieve O(1) lookups and prevent lag in large lists.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Skip group headers to match native <select> announcements with <optgroup> | ||
| const optionCount = memoFlattenOptions.filter((option) => !option.group).length; | ||
|
|
||
| const getOptionPosition = (index: number) => | ||
| memoFlattenOptions.slice(0, index + 1).filter((option) => !option.group).length; |
There was a problem hiding this comment.
Calculating optionCount and getOptionPosition on every render performs array filtering and slicing operations ($O(N)$) over the entire memoFlattenOptions list. Since getOptionPosition is called up to three times per render (for activeIndex - 1, activeIndex, and activeIndex + 1), and renders occur on every keyboard navigation step (as activeIndex changes), this can cause noticeable lag/performance degradation in large virtualized lists.
Since memoFlattenOptions is already memoized, we can precompute the option count and positions map in a single React.useMemo. This reduces the lookup in getOptionPosition to
// Skip group headers to match native <select> announcements with <optgroup>
const [optionCount, optionPositions] = React.useMemo(() => {
let count = 0;
const positions = new Array(memoFlattenOptions.length);
for (let i = 0; i < memoFlattenOptions.length; i += 1) {
if (!memoFlattenOptions[i].group) {
count += 1;
}
positions[i] = count;
}
return [count, positions];
}, [memoFlattenOptions]);
const getOptionPosition = (index: number) => optionPositions[index] || 0;
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1226 +/- ##
==========================================
- Coverage 99.44% 99.30% -0.15%
==========================================
Files 31 31
Lines 1266 1292 +26
Branches 442 449 +7
==========================================
+ Hits 1259 1283 +24
- Misses 7 9 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
tests/Accessibility.test.tsx (1)
266-303: ⚡ Quick win建议补一条“group wrapper 不应带
aria-posinset/aria-setsize”的显式断言。当前测试已验证
option上有位置属性，但还没直接锁定role="group"容器不应携带这些属性。补上后可更稳地覆盖“仅 option 有位置语义”的回归面。✅ 可追加断言示例
groupWrappers = getGroupWrappers(); expect(groupWrappers).toHaveLength(1); expect(groupWrappers[0]).toHaveAttribute('aria-label', 'Second group'); + groupWrappers.forEach((group) => { + expect(group).not.toHaveAttribute('aria-posinset'); + expect(group).not.toHaveAttribute('aria-setsize'); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Accessibility.test.tsx` around lines 266 - 303, Add explicit assertions to ensure the group wrapper elements returned by getGroupWrappers() do not carry positional attributes; after fetching groupWrappers (e.g., the variable groupWrappers used in the test) assert that groupWrappers[0] (and any group wrapper in the array) does not have aria-posinset and does not have aria-setsize, similar to how hiddenOptions (from getHiddenOptions()) are asserted for those attributes so the test guarantees only option elements expose position semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/OptionList.tsx`:
- Around line 304-306: The aria-label currently only sets when mergedLabel is a
string, which drops semantic labels for numeric titles; update the condition to
reuse the existing isTitleType check and coerce the title to string (e.g.,
aria-label={isTitleType(mergedLabel) ? String(mergedLabel) : null}) so numeric
labels are exposed to assistive tech; apply the same change to the other
occurrence that sets aria-label (the second block using
mergedLabel/getOptionPosition).
---
Nitpick comments:
In `@tests/Accessibility.test.tsx`:
- Around line 266-303: Add explicit assertions to ensure the group wrapper
elements returned by getGroupWrappers() do not carry positional attributes;
after fetching groupWrappers (e.g., the variable groupWrappers used in the test)
assert that groupWrappers[0] (and any group wrapper in the array) does not have
aria-posinset and does not have aria-setsize, similar to how hiddenOptions (from
getHiddenOptions()) are asserted for those attributes so the test guarantees
only option elements expose position semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0427256-7a22-48f6-b5ce-5248b088f57f
⛔ Files ignored due to path filters (3)
tests/__snapshots__/OptionList.test.tsx.snapis excluded by!**/*.snaptests/__snapshots__/Select.test.tsx.snapis excluded by!**/*.snaptests/__snapshots__/Tags.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/OptionList.tsxtests/Accessibility.test.tsx
| aria-label={typeof mergedLabel === 'string' ? mergedLabel : null} | ||
| aria-setsize={optionCount} | ||
| aria-posinset={getOptionPosition(index)} |
There was a problem hiding this comment.
建议将数字标签也映射到 aria-label，避免可读名称丢失。
当前仅在 label 为字符串时设置 aria-label。当标签是数字时（合法场景），隐藏无障碍节点会丢失该标签语义，读屏更可能退化为读 value。建议复用现有 isTitleType 逻辑统一转字符串。
💡 建议修改
- aria-label={typeof mergedLabel === 'string' ? mergedLabel : null}
+ aria-label={isTitleType(mergedLabel) ? String(mergedLabel) : null}
...
- aria-label={typeof groupLabel === 'string' ? groupLabel : null}
+ aria-label={isTitleType(groupLabel) ? String(groupLabel) : null}Also applies to: 361-362
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/OptionList.tsx` around lines 304 - 306, The aria-label currently only
sets when mergedLabel is a string, which drops semantic labels for numeric
titles; update the condition to reuse the existing isTitleType check and coerce
the title to string (e.g., aria-label={isTitleType(mergedLabel) ?
String(mergedLabel) : null}) so numeric labels are exposed to assistive tech;
apply the same change to the other occurrence that sets aria-label (the second
block using mergedLabel/getOptionPosition).
1 participant
Problem
With
virtualmode enabled, the hidden accessibility container only renders 3 options around the active one (activeIndex ± 1). Screen readers computed option positions from the DOM, so users always heard "1 of 3" / "2 of 3" / "3 of 3" regardless of the actual option position and list size.Fix
aria-posinset/aria-setsizeto the hidden options so screen readers announce the real position and total (e.g. "4 of 6").<select>+<optgroup>announcements.role="presentation"group headers in the hidden container withrole="group"+aria-labelwrappers nesting their options, so screen readers also announce the group name when the active option enters a new group (MDN example).Tests
role="option"elements only.Related issues
Fixes ant-design/ant-design#58346
Summary by CodeRabbit
发布说明