-
-
Notifications
You must be signed in to change notification settings - Fork 234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: improve TabPane accessibility and content handling #780
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Walkthrough该 PR 修改了 Changes
Sequence Diagram(s)sequenceDiagram
participant C as Component
participant T as TabPane
C->>T: 渲染组件(提供 active 状态和 children)
T->>T: 检查 hasContent(判断 children 是否存在)
alt active 且 hasContent 为 true
T->>T: 设置 tabIndex = 0
else
T->>T: 设置 tabIndex = -1
end
Possibly related PRs
Suggested reviewers
Poem
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
tests/accessibility.test.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-react". (The package "eslint-plugin-react" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following:
The plugin "eslint-plugin-react" was referenced from the config file in ".eslintrc.js » /node_modules/.pnpm/@umijs[email protected]_jest@29.7.0_@types[email protected][email protected]/node_modules/@umijs/fabric/dist/eslint.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #780 +/- ##
=======================================
Coverage 98.96% 98.96%
=======================================
Files 18 18
Lines 771 772 +1
Branches 227 224 -3
=======================================
+ Hits 763 764 +1
Misses 8 8 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/TabPanelList/TabPane.tsx (1)
32-32
: tabIndex 的处理逻辑改进了无障碍体验!当前实现通过检查
active
和hasContent
来控制面板的焦点行为,这种方式符合 ARIA 最佳实践。建议考虑为空面板添加
aria-empty
属性,以进一步提升无障碍体验。<div id={id && `${id}-panel-${tabKey}`} role="tabpanel" tabIndex={active && hasContent ? 0 : -1} + aria-empty={!hasContent} aria-labelledby={id && `${id}-tab-${tabKey}`} aria-hidden={!active}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/TabPanelList/TabPane.tsx
(1 hunks)
🔇 Additional comments (1)
src/TabPanelList/TabPane.tsx (1)
26-26
: 代码实现正确且简洁!使用
React.Children.count
来检查子元素是否存在的方法非常恰当。这种实现方式简单有效,为后续的 tabIndex 逻辑奠定了基础。
return ( | ||
<div | ||
id={id && `${id}-panel-${tabKey}`} | ||
role="tabpanel" | ||
tabIndex={active ? 0 : -1} | ||
tabIndex={active && hasContent ? 0 : -1} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
测试用例加一下
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/accessibility.test.tsx (1)
272-282
: 测试用例实现正确,建议增加更多场景测试测试用例很好地验证了空面板的焦点行为。为了更全面地测试该功能,建议考虑添加以下场景:
- 多个标签页混合测试(有内容和无内容的标签页)
- 动态切换标签页内容(从有到无,从无到有)
建议添加如下测试用例:
it('should handle focus correctly with mixed content tabs', async () => { const user = userEvent.setup(); const { getByRole } = render( <Tabs defaultActiveKey="1" items={[ { key: '1', label: 'Tab1', children: 'Content 1' }, { key: '2', label: 'Tab2', children: null }, { key: '3', label: 'Tab3', children: 'Content 3' } ]} />, ); // 验证有内容的面板可以获得焦点 const tabPanel1 = getByRole('tabpanel', { name: /Tab1/i }); await user.tab(); await user.tab(); expect(tabPanel1).toHaveFocus(); // 验证空面板不会获得焦点 const tabPanel2 = getByRole('tabpanel', { name: /Tab2/i }); await user.keyboard('{ArrowRight}'); await user.tab(); expect(tabPanel2).not.toHaveFocus(); }); it('should handle focus correctly when content changes dynamically', async () => { const user = userEvent.setup(); const Demo = () => { const [content, setContent] = React.useState('Initial content'); return ( <> <button onClick={() => setContent(null)}>Clear content</button> <button onClick={() => setContent('New content')}>Add content</button> <Tabs defaultActiveKey="1" items={[{ key: '1', label: 'Tab1', children: content }]} /> </> ); }; const { getByRole } = render(<Demo />); // 验证初始状态下面板可以获得焦点 const tabPanel = getByRole('tabpanel', { name: /Tab1/i }); await user.tab(); await user.tab(); expect(tabPanel).toHaveFocus(); // 清除内容后验证面板不会获得焦点 await user.click(getByRole('button', { name: /Clear content/i })); await user.tab(); await user.tab(); expect(tabPanel).not.toHaveFocus(); // 添加内容后验证面板可以获得焦点 await user.click(getByRole('button', { name: /Add content/i })); await user.tab(); await user.tab(); expect(tabPanel).toHaveFocus(); });
页签卡片复用了tabs组件,但是tabs children始终为空,需要特殊处理一下focus的情况
Summary by CodeRabbit