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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough该 PR 调整 Select 虚拟列表的无障碍序号通报。代码新增 ARIA 位置属性，按分组结构渲染隐藏选项，并补充虚拟列表与分组场景测试。 Changes虚拟列表无障碍属性与分组渲染
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1226 +/- ##
==========================================
+ Coverage 99.44% 99.46% +0.01%
==========================================
Files 31 31
Lines 1271 1301 +30
Branches 466 472 +6
==========================================
+ Hits 1264 1294 +30
Misses 7 7 ☔ 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
|
@afc163 @zombieJ @meet-student @QDyanbing Could you please review? |
|
@afc163 Could you please take a look? |
|
Thanks for the update. The current direction looks good to me: adding One small edge case before merging: Please also check the Codecov failures, or leave a note if they are expected/unrelated. The Vercel failure looks like the usual authorization issue and is not a code signal. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Accessibility.test.tsx (1)
307-318: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win显式开启
virtual，避免用例依赖默认值。这里验证的是虚拟模式下隐藏 a11y 容器的分段渲染，但
virtual没有显式传入；如果默认值以后调整，这个用例就不再稳定地覆盖这条路径了。Line 307-318 建议把场景条件写死。♻️ 建议修改
const { container } = render( <Select id="virtual-select" open + virtual options={[🤖 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 307 - 318, The Select accessibility test currently relies on the component’s default virtual behavior, which makes the scenario brittle if the default changes. Update the Select usage in the virtual a11y case to explicitly pass virtual so the test always exercises the hidden accessibility container path. Keep the change local to the test case around the Select and its grouped options setup.
🤖 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.
Nitpick comments:
In `@tests/Accessibility.test.tsx`:
- Around line 307-318: The Select accessibility test currently relies on the
component’s default virtual behavior, which makes the scenario brittle if the
default changes. Update the Select usage in the virtual a11y case to explicitly
pass virtual so the test always exercises the hidden accessibility container
path. Keep the change local to the test case around the Select and its grouped
options setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47d4a3a6-f7ca-445e-b0fb-bbcc10862375
📒 Files selected for processing (2)
src/OptionList.tsxtests/Accessibility.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/OptionList.tsx
|
@afc163 Done. Fixed value for labels and adjusted code coverage. |
yoyo837
left a comment
yoyo837
left a comment
There was a problem hiding this comment.
Thanks for the accessibility improvement. I found two follow-up suggestions around large grouped lists and non-text group labels. I am leaving these as comments rather than an approval/request-changes review.
| return; | ||
| } | ||
|
|
||
| const groupItem = item.groupOption ? getGroupItem(index) : null; |
There was a problem hiding this comment.
Could we precompute the owning group for each flattened item together with optionPositions? getGroupItem walks backward from the active index for each of up to three hidden items, so keyboard navigation near the end of a large single group can become O(N) per render and partially undermine virtualization. A memoized parallel array/map would make these lookups O(1).
| <div | ||
| key={group.key} | ||
| role="group" | ||
| aria-label={isTitleType(groupLabel) ? String(groupLabel) : null} |
There was a problem hiding this comment.
Could we preserve an accessible name when the group label is a React node? Option/group labels may be non-string React nodes; in that case this new role="group" wrapper is unnamed, so assistive technology cannot announce the group name. Consider using aria-labelledby with a hidden element that renders the label, or falling back to a string data.title when available, and add a regression test with a React-node group label.
There was a problem hiding this comment.
Thanks. I rechecked the latest head (8dc092c) and the React-node case is still unnamed unless the caller also supplies title. For example, with label={<span>React node group</span>} and no title, Testing Library reports the role="group" accessible name as empty, so getByRole('group', { name: 'React node group' }) fails. Since DefaultOptionType.label supports ReactNode, the data.title fallback does not cover the normal React-node-label case by itself. Could we keep this thread open and either expose a stable accessible-name source (or honor an explicit aria-label/aria-labelledby), or avoid emitting an unnamed group? A regression test for a React-node group label would catch this.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
0b2f977 to
8dc092c
Compare
|
@yoyo837 Fixed both suggestions. As to React-node group label I've added |
4 participants
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
发布说明
Bug Fixes
aria-setsize和aria-posinset信息。Tests