fix: improve pagination aria labels and semantics#690
Open
ZQDesigned wants to merge 9 commits into
Open
Conversation
|
@ZQDesigned 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为 Pager 与 Pagination 添加语义 role/aria-* 属性，pageLabel 从 Pagination 透传到 Pager，扩展键盘激活支持空格键，并补充对应无障碍与键盘交互测试验证。 Changes分页组件无障碍与交互增强
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves Pagination accessibility by adding ARIA attributes, keyboard activation support, and updating related tests/snapshots.
Changes:
- Add
aria-current,aria-label, androleattributes to pagination controls/items. - Update keyboard handling to activate controls via Space/Enter.
- Refresh tests and snapshots to assert/access the new accessibility attributes.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/index.test.tsx | Adds assertions for aria-current, roles, and labels on pagination items. |
| tests/snapshots/simple.test.tsx.snap | Updates snapshots to reflect new ARIA attributes and title placement. |
| tests/snapshots/demo.test.tsx.snap | Updates demo snapshots for new roles/labels and aria-current. |
| src/Pagination.tsx | Introduces ARIA/title behavior changes, keyboard handling updates, and locale fallbacks. |
| src/Pager.tsx | Adds aria-label, aria-current, and role to page items and accepts pageLabel. |
| src/Options.tsx | Adds non-empty-string fallback for aria-label values in page size / quick jumper. |
Comments suppressed due to low confidence (2)
src/Pager.tsx:1
role="button"(plus keyboard handlers) is being applied to the<li>whilepageris rendered fromitemRender(...)and is typically an interactive descendant (e.g. an<a>). This creates nested interactive controls and confusing semantics (multiple focus targets / conflicting roles). Prefer making the actual interactive element the focusable control and keep the<li>presentational (e.g., movetabIndex, key handlers, and ARIA attributes onto the rendered<a>/<button>, or render a<button>/<a>as the direct child and removerole/handlers from<li>).
/* eslint react/prop-types: 0 */
src/Pagination.tsx:1
- Similar to
Pager, addingrole="button"/tabIndex/keyboard handling to the<li>while it contains an actual<button>rendered bygetItemIcon(...)results in nested interactive elements and extra tab stops. This is an accessibility anti-pattern and can regress keyboard/screen reader behavior. Prefer moving click/key handling (and ARIA) onto the inner<button>(or render a single<button>as the clickable control) and keep the<li>as a non-interactive list container.
import { clsx } from 'clsx';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request improves the accessibility of the pagination component by adding appropriate ARIA roles, labels, and keyboard interaction support. The feedback highlights two critical areas of improvement: first, preventing potential runtime crashes in the getNonEmptyString helper when non-string locale values (such as React elements) are passed; second, resolving accessibility violations caused by nesting interactive elements (like <a> and <button>) inside parent elements that have been assigned role="button".
![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 (3)
src/Pagination.tsx (1)
264-269: 💤 Low value空格键支持的向后兼容性可以进一步增强
当前实现已正确支持现代浏览器的空格键激活（
event.key === ' '和event.key === 'Spacebar'），但在第 266-267 行的charCode和keyCode回退检查中仅检查了KeyCode.ENTER。对于不支持event.key的极老浏览器，空格键（keyCode 32）不会触发回调。虽然这些极老浏览器已非常罕见，且主要的无障碍改进在现代浏览器中已生效，但如果需要完整的向后兼容性，可以考虑在回退检查中也包含
KeyCode.SPACE。💡 可选的增强方案
function runIfEnter( event: React.KeyboardEvent<HTMLLIElement>, callback: (...args: any[]) => void, ...restParams: any[] ) { if ( event.key === 'Enter' || event.key === ' ' || event.key === 'Spacebar' || event.charCode === KeyCode.ENTER || - event.keyCode === KeyCode.ENTER + event.keyCode === KeyCode.ENTER || + event.keyCode === 32 // KeyCode.SPACE ) { event.preventDefault(); callback(...restParams); } }🤖 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/Pagination.tsx` around lines 264 - 269, The keyboard handler currently checks event.key for space but falls back to only KeyCode.ENTER for event.charCode/event.keyCode; update the fallback in the pagination keyboard handler (the conditional around event.key === ' ' / 'Spacebar' and event.charCode/event.keyCode) to also accept KeyCode.SPACE (numeric 32) so legacy browsers that only expose charCode/keyCode will treat the space key the same as Enter; modify the condition that references KeyCode.ENTER to include KeyCode.SPACE for both event.charCode and event.keyCode checks.tests/index.test.tsx (2)
100-110: ⚡ Quick win建议添加空格键激活的测试用例。
根据 PR 目标,"支持空格键激活以及回车键激活"是此次改进的一部分。当前测试用例以及现有的键盘支持测试(第 214-221 行和第 593-654 行)仅验证了回车键(Enter)的行为,缺少对空格键(Space)激活的测试覆盖。
🧪 建议添加空格键测试
可以在现有键盘支持测试部分(第 593 行开始的 "keyboard support" describe 块)中添加空格键测试:
it('should work with Space key for prev page', () => { const prevButton = $('li.rc-pagination-prev'); fireEvent.keyDown(prevButton, { key: ' ', keyCode: 32, which: 32 }); expect(onChange).toHaveBeenLastCalledWith(49, 10); }); it('should work with Space key for next page', () => { const nextButton = $('li.rc-pagination-next'); fireEvent.keyDown(nextButton, { key: ' ', keyCode: 32, which: 32 }); expect(onChange).toHaveBeenLastCalledWith(51, 10); }); it('should work with Space key for page items', () => { const pagers = wrapper.container.querySelectorAll('.rc-pagination-item'); fireEvent.keyDown(pagers[0], { key: ' ', keyCode: 32, which: 32 }); expect(onChange).toHaveBeenCalled(); });🤖 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/index.test.tsx` around lines 100 - 110, Add tests that assert Space key activation alongside Enter in the existing "keyboard support" describe block in tests/index.test.tsx: locate the block where fireEvent.keyDown and onChange are used (symbols: fireEvent, onChange, wrapper, $ helper) and add cases that fire keyDown with { key: ' ', keyCode: 32, which: 32 } on the prev button ('li.rc-pagination-prev'), next button ('li.rc-pagination-next'), and a page item ('.rc-pagination-item'), then assert onChange was called with the expected arguments (matching the existing Enter-key expectations) to ensure Space triggers the same navigation behavior.
105-107: ⚡ Quick win建议验证 aria-label 的值非空。
当前断言仅检查
aria-label属性是否存在,但即使值为空字符串断言也会通过。根据 PR 目标,已添加了安全回退机制以防止空标签。建议验证标签实际包含有意义的内容,以确保回退逻辑正常工作。💡 建议的增强断言
- expect(prevButton).toHaveAttribute('aria-label'); + expect(prevButton).toHaveAttribute('aria-label'); + expect(prevButton.getAttribute('aria-label')).toBeTruthy(); expect(nextButton).toHaveAttribute('role', 'button'); - expect(nextButton).toHaveAttribute('aria-label'); + expect(nextButton).toHaveAttribute('aria-label'); + expect(nextButton.getAttribute('aria-label')).toBeTruthy();🤖 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/index.test.tsx` around lines 105 - 107, The tests currently only assert the presence of the aria-label attribute on prevButton and nextButton; update those assertions to ensure the attribute value is non-empty so the fallback logic is exercised. Locate the assertions referencing prevButton and nextButton in the test (the lines using toHaveAttribute('aria-label')) and replace/augment them with checks that the aria-label value is a non-empty string (e.g., retrieve the attribute and assert it's truthy or not equal to an empty string) for both prevButton and nextButton to guarantee meaningful labels are present.
🤖 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/Options.tsx`:
- Around line 34-36: Extract the duplicated getNonEmptyString function into a
shared utility module and import it from both components: create a new exported
function getNonEmptyString in src/utils.ts (or your utilities module), remove
the local definitions of getNonEmptyString from Options.tsx and Pagination.tsx,
and replace them with an import of the shared getNonEmptyString; ensure the
function signature and behavior remain identical so calls in Options.tsx and
Pagination.tsx continue to work without further changes.
---
Nitpick comments:
In `@src/Pagination.tsx`:
- Around line 264-269: The keyboard handler currently checks event.key for space
but falls back to only KeyCode.ENTER for event.charCode/event.keyCode; update
the fallback in the pagination keyboard handler (the conditional around
event.key === ' ' / 'Spacebar' and event.charCode/event.keyCode) to also accept
KeyCode.SPACE (numeric 32) so legacy browsers that only expose charCode/keyCode
will treat the space key the same as Enter; modify the condition that references
KeyCode.ENTER to include KeyCode.SPACE for both event.charCode and event.keyCode
checks.
In `@tests/index.test.tsx`:
- Around line 100-110: Add tests that assert Space key activation alongside
Enter in the existing "keyboard support" describe block in tests/index.test.tsx:
locate the block where fireEvent.keyDown and onChange are used (symbols:
fireEvent, onChange, wrapper, $ helper) and add cases that fire keyDown with {
key: ' ', keyCode: 32, which: 32 } on the prev button ('li.rc-pagination-prev'),
next button ('li.rc-pagination-next'), and a page item ('.rc-pagination-item'),
then assert onChange was called with the expected arguments (matching the
existing Enter-key expectations) to ensure Space triggers the same navigation
behavior.
- Around line 105-107: The tests currently only assert the presence of the
aria-label attribute on prevButton and nextButton; update those assertions to
ensure the attribute value is non-empty so the fallback logic is exercised.
Locate the assertions referencing prevButton and nextButton in the test (the
lines using toHaveAttribute('aria-label')) and replace/augment them with checks
that the aria-label value is a non-empty string (e.g., retrieve the attribute
and assert it's truthy or not equal to an empty string) for both prevButton and
nextButton to guarantee meaningful labels are present.
🪄 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: d67d4cf3-cfe7-45f1-9ce5-f3df08803dd4
⛔ Files ignored due to path filters (2)
tests/__snapshots__/demo.test.tsx.snapis excluded by!**/*.snaptests/__snapshots__/simple.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
src/Options.tsxsrc/Pager.tsxsrc/Pagination.tsxtests/index.test.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Options.tsx (1)
122-137: ⚡ Quick win
SizeChangerRender的aria-label类型不安全（strictNullChecks 时可能触发 TS2322）
src/Options.tsx中'aria-label'期望为必填string，但getNonEmptyString(...)返回string | undefined（内部使用了?.trim()）。- 若启用
strictNullChecks，这会触发 TS2322；建议在此处兜底/做非空断言以保证传入类型恒为string。🛠️ 可选修复
- 'aria-label': getNonEmptyString(locale.page_size, 'Page Size'), + 'aria-label': getNonEmptyString(locale.page_size, 'Page Size') as string,🤖 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/Options.tsx` around lines 122 - 137, The 'aria-label' passed into the size changer render can be undefined (getNonEmptyString returns string | undefined) and will fail under strictNullChecks; update the call site inside the showSizeChanger && sizeChangerRender block to ensure a definitive string is provided to the 'aria-label' prop (e.g., use a fallback with nullish coalescing or a non-null assertion on getNonEmptyString(locale.page_size, 'Page Size')), so that the value passed to sizeChangerRender is always a string; modify the expression where 'aria-label' is set (referencing getNonEmptyString and the sizeChangerRender call) to produce a string unconditionally.
🤖 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/utils/getNonEmptyString.ts`:
- Around line 1-5: The predicate passed to Array.prototype.find in
getNonEmptyString currently returns unknown | undefined so the subsequent
?.trim() can be called on unknown and triggers TS18046; change the predicate to
a TypeScript type guard that narrows the element to string (for example make the
callback signature (v): v is string and return typeof v === 'string' && v.trim()
!== ''), so .find(...) will be typed as string | undefined and calling ?.trim()
is safe—update the predicate in getNonEmptyString accordingly.
---
Nitpick comments:
In `@src/Options.tsx`:
- Around line 122-137: The 'aria-label' passed into the size changer render can
be undefined (getNonEmptyString returns string | undefined) and will fail under
strictNullChecks; update the call site inside the showSizeChanger &&
sizeChangerRender block to ensure a definitive string is provided to the
'aria-label' prop (e.g., use a fallback with nullish coalescing or a non-null
assertion on getNonEmptyString(locale.page_size, 'Page Size')), so that the
value passed to sizeChangerRender is always a string; modify the expression
where 'aria-label' is set (referencing getNonEmptyString and the
sizeChangerRender call) to produce a string unconditionally.
🪄 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: 9098cfbe-8bab-4b1d-992e-7aa152e9dbaa
⛔ Files ignored due to path filters (2)
tests/__snapshots__/demo.test.tsx.snapis excluded by!**/*.snaptests/__snapshots__/simple.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
src/Options.tsxsrc/Pager.tsxsrc/Pagination.tsxsrc/utils/getNonEmptyString.tstests/index.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Pager.tsx
- tests/index.test.tsx
ZQDesigned
force-pushed
the
fix/a11y-aria-labels
branch
from
86fc95a to
0614c2f
Compare
ZQDesigned
force-pushed
the
fix/a11y-aria-labels
branch
from
0614c2f to
c559371
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Pagination.tsx`:
- Around line 410-413: The quick-jumper input in Pagination.tsx currently uses
locale.page directly for aria-label which can be missing or empty; update the
aria-label to fall back to a default 'Page' string (matching the Pager.tsx
fallback) so the input always has an accessible name—locate the input element
that uses value={internalInputVal} and change aria-label to use locale.page ||
'Page' (or a shared fallback constant) to ensure consistent accessibility.
- Around line 252-265: The runIfEnterOrSpace handler only checks event.key for
space but misses numeric fallbacks; update the function runIfEnterOrSpace to
include checks for event.charCode === KeyCode.SPACE (32) and event.keyCode ===
KeyCode.SPACE (32) alongside the existing ENTER checks so space activation works
in environments that only provide keyCode/charCode; then add/extend unit tests
for the Pagination component (covering previous/next and jump controls) to
simulate keyboard events with event.key = ' ' and 'Spacebar' and with
keyCode/charCode = 32 to assert the callbacks fire.
🪄 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: 80a6d822-a9f9-44d9-aab5-980e14f9b4b3
⛔ Files ignored due to path filters (2)
tests/__snapshots__/demo.test.tsx.snapis excluded by!**/*.snaptests/__snapshots__/simple.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
src/Pager.tsxsrc/Pagination.tsxtests/index.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/index.test.tsx
- src/Pager.tsx
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #690 +/- ##
==========================================
+ Coverage 99.03% 99.68% +0.65%
==========================================
Files 3 4 +1
Lines 312 321 +9
Branches 140 145 +5
==========================================
+ Hits 309 320 +11
+ Misses 3 1 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/index.test.tsx`:
- Around line 624-632: The Space-key tests in tests/index.test.tsx are
accidentally passing Enter key codes (keyCode/which: 13) which triggers the
Enter branch; update the four Space-related cases that fire keyDown on elements
like prevButton (selector 'li.rc-pagination-prev') and similar next/other
buttons so the synthetic event represents Space only — either remove
keyCode/which or set them to 32 (and keep key: ' ' or 'Spacebar' if kept);
ensure the fired events use the correct key/char codes and keep the existing
expect(onChange)... assertions unchanged.
🪄 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: 2c2c6c15-57d6-451f-ba3a-f6b8d13063a8
📒 Files selected for processing (1)
tests/index.test.tsx
这个需要覆盖上哈 |
Author
working on it |
这个场景的, 其他覆盖率没什么问题, 我看着 |
zombieJ
reviewed
Jun 2, 2026
zombieJ
reviewed
Jun 2, 2026
zombieJ
reviewed
Jun 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Improve accessibility semantics for pagination controls and quick-jumper inputs.
Changes
role="button"and explicitaria-labelto interactive paginationliitems (prev,next,jump-prev,jump-next, and page items)aria-current="page"on the active page itemitemRenderoutputMotivation
This follows the a11y request discussed in ant-design/ant-design#58072, where some pagination controls lacked effective labels/semantics in screen readers.
Verification
npm test -- tests/index.test.tsx tests/itemRender.test.tsx tests/options.test.tsx tests/simple.test.tsx tests/demo.test.tsx -unpm test -- tests/jumper.test.tsx tests/sizer.test.tsx tests/data-aria.test.tsxSummary by CodeRabbit
新功能
可访问性改进
测试