-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: improve keyboard operation accessibility #285
Conversation
Walkthrough此次更改主要涉及对 Changes
Possibly related PRs
Poem
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 (
|
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: 3
🧹 Outside diff range and nitpick comments (1)
assets/index.less (1)
126-128
: 建议增强焦点指示器的可见性当前的box-shadow实现提供了基本的焦点指示,但可以考虑以下优化建议:
- 增加对比度以提高可见性
- 考虑在暗色主题下的表现
建议调整样式如下:
&-focused { - box-shadow: 0 0 0 2px rgba(24, 144, 255, 0.2); + box-shadow: 0 0 0 2px rgba(24, 144, 255, 0.4); + transition: box-shadow @transition-duration @transition-timing-function; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tests/__snapshots__/index.test.tsx.snap
is excluded by!**/*.snap
📒 Files selected for processing (2)
assets/index.less
(1 hunks)src/index.tsx
(6 hunks)
🔇 Additional comments (1)
assets/index.less (1)
121-124
: 需要重新考虑移除outline的可访问性影响
移除默认的outline可能会影响键盘用户的可访问性体验。虽然您添加了自定义的focus样式,但建议在完全移除outline之前确保替代方案足够明显。
请运行以下脚本检查是否所有focus状态都有明确的视觉反馈:
✅ Verification successful
移除outline样式是安全的,已有合适的替代方案
检查结果显示,虽然移除了默认的outline,但已经通过.rc-segmented-item-focused
类添加了清晰的box-shadow效果(0 0 0 2px rgba(24, 144, 255, 0.2))作为焦点状态的视觉反馈。这种实现方式符合可访问性要求,为键盘用户提供了足够明显的视觉提示。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 检查所有相关组件的focus状态样式
rg -l "outline:\s*none" | xargs rg "box-shadow|border|outline"
Length of output: 158
Script:
#!/bin/bash
# 检查完整的focus相关样式定义
rg -A 5 -B 5 "\.rc-segmented-item.*focus"
Length of output: 43
Script:
#!/bin/bash
# 检查.rc-segmented-item相关的所有样式定义
rg -A 10 "\.rc-segmented-item" assets/index.less
Length of output: 178
src/index.tsx
Outdated
// Do not add focus style when clicking | ||
onBlur(); |
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.
🛠️ Refactor suggestion
手动调用 onBlur() 可能导致意外行为
在 handleChange
函数中手动调用 onBlur()
可能会导致与正常的事件流不一致,可能引发意外的焦点丢失或其他副作用。建议重新评估这种调用的必要性,或者寻找更符合 React 事件处理规范的方法。
src/index.tsx
Outdated
const getNextIndex = (current: number, offset: number) => { | ||
if (disabled) { | ||
return current; | ||
} | ||
|
||
const total = segmentedOptions.length; | ||
const nextIndex = (current + offset + total) % total; | ||
|
||
if (segmentedOptions[nextIndex]?.disabled) { | ||
return getNextIndex(nextIndex, offset); | ||
} | ||
return nextIndex; | ||
}; |
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.
当所有选项均被禁用时,getNextIndex 函数可能导致无限递归
getNextIndex
函数在所有选项都被禁用的情况下,会陷入无限递归,导致堆栈溢出。建议在函数中添加一个终止条件,检测所有选项是否都被禁用,并在这种情况下返回当前索引或采取适当的措施。
建议的修改:
const getNextIndex = (current: number, offset: number) => {
if (disabled) {
return current;
}
const total = segmentedOptions.length;
+ if (segmentedOptions.every(option => option.disabled)) {
+ return current;
+ }
const nextIndex = (current + offset + total) % total;
if (segmentedOptions[nextIndex]?.disabled) {
return getNextIndex(nextIndex, offset);
}
return nextIndex;
};
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const getNextIndex = (current: number, offset: number) => { | |
if (disabled) { | |
return current; | |
} | |
const total = segmentedOptions.length; | |
const nextIndex = (current + offset + total) % total; | |
if (segmentedOptions[nextIndex]?.disabled) { | |
return getNextIndex(nextIndex, offset); | |
} | |
return nextIndex; | |
}; | |
const getNextIndex = (current: number, offset: number) => { | |
if (disabled) { | |
return current; | |
} | |
const total = segmentedOptions.length; | |
if (segmentedOptions.every(option => option.disabled)) { | |
return current; | |
} | |
const nextIndex = (current + offset + total) % total; | |
if (segmentedOptions[nextIndex]?.disabled) { | |
return getNextIndex(nextIndex, offset); | |
} | |
return nextIndex; | |
}; |
src/index.tsx
Outdated
switch (event.which) { | ||
case KeyCode.LEFT: | ||
case KeyCode.UP: | ||
event.preventDefault(); | ||
offset = -1; | ||
break; | ||
case KeyCode.RIGHT: | ||
case KeyCode.DOWN: | ||
event.preventDefault(); | ||
offset = 1; | ||
break; | ||
default: | ||
return; | ||
} |
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.
🛠️ Refactor suggestion
使用 event.key 或 event.keyCode 替代已弃用的 event.which
在 handleKeyDown
函数中,event.which
属性已被弃用,不再推荐使用。建议改用 event.key
或 event.keyCode
来检测按键事件,以提高代码的兼容性和未来的可维护性。
建议的修改:
-switch (event.which) {
+switch (event.keyCode) {
case KeyCode.LEFT:
case KeyCode.UP:
event.preventDefault();
offset = -1;
break;
case KeyCode.RIGHT:
case KeyCode.DOWN:
event.preventDefault();
offset = 1;
break;
default:
return;
}
或使用 event.key
:
-switch (event.which) {
+switch (event.key) {
case 'ArrowLeft':
case 'ArrowUp':
event.preventDefault();
offset = -1;
break;
case 'ArrowRight':
case 'ArrowDown':
event.preventDefault();
offset = 1;
break;
default:
return;
}
Committable suggestion skipped: line range outside the PR's diff.
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
🧹 Outside diff range and nitpick comments (2)
src/index.tsx (2)
196-202
: 优化事件处理函数参数
handleFocus
和handleBlur
函数定义了事件参数但未使用,建议移除未使用的参数。建议修改如下:
-const handleFocus = (event: React.FocusEvent<HTMLInputElement>) => { +const handleFocus = () => { setIsFocused(true); }; -const handleBlur = (event?: React.FocusEvent<HTMLInputElement>) => { +const handleBlur = () => { setIsFocused(false); };🧰 Tools
🪛 eslint (1.23.1)
[error] 196-196: 'event' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 200-200: 'event' is defined but never used.
(@typescript-eslint/no-unused-vars)
244-246
: 建议增强无障碍访问属性虽然已添加了基本的ARIA属性,但建议添加以下补充属性以提供更好的无障碍访问体验:
- 为选项添加
aria-checked
属性- 添加
aria-orientation
属性以指示方向性建议修改如下:
<div role="radiogroup" aria-label="segmented control" + aria-orientation={vertical ? 'vertical' : 'horizontal'} tabIndex={disabled ? undefined : 0} {...divProps}
<div className={`${prefixCls}-item-label`} title={title} aria-selected={checked} + aria-checked={checked} >
Also applies to: 289-290
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/index.tsx
(6 hunks)
🧰 Additional context used
🪛 eslint (1.23.1)
src/index.tsx
[error] 196-196: 'event' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 200-200: 'event' is defined but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (1)
src/index.tsx (1)
88-90
: 接口更改符合无障碍访问要求
新增的事件处理属性完善了键盘操作支持,类型定义准确。
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #285 +/- ##
==========================================
+ Coverage 99.21% 99.37% +0.16%
==========================================
Files 2 2
Lines 127 161 +34
Branches 29 40 +11
==========================================
+ Hits 126 160 +34
Misses 1 1 ☔ 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: 2
🧹 Outside diff range and nitpick comments (5)
src/index.tsx (1)
188-201
: 建议增强焦点管理的健壮性当前的焦点管理实现较为基础,建议考虑以下增强:
- 添加
focusVisible
状态区分键盘和鼠标焦点- 处理组件卸载时的焦点状态清理
+ const [focusVisible, setFocusVisible] = React.useState(false); + + const handleFocus = (event: React.FocusEvent) => { + if (event.target.matches(':focus-visible')) { + setFocusVisible(true); + } + setIsFocused(true); + }; + + React.useEffect(() => { + return () => { + setIsFocused(false); + setFocusVisible(false); + }; + }, []);tests/index.test.tsx (4)
670-686
: 建议增强Tab键测试覆盖率当前测试验证了容器和第一个选项的焦点,建议添加以下场景:
- 验证Tab键在所有选项之间的焦点传递
- 验证Shift+Tab的反向焦点传递
it('should be focusable through Tab key', async () => { const user = userEvent.setup(); const { getByRole, container } = render( <Segmented options={['Daily', 'Weekly', 'Monthly']} />, ); const segmentedContainer = getByRole('radiogroup'); const inputs = container.querySelectorAll('.rc-segmented-item-input'); const firstInput = inputs[0]; + const secondInput = inputs[1]; + const lastInput = inputs[2]; await user.tab(); expect(segmentedContainer).toHaveFocus(); await user.tab(); expect(firstInput).toHaveFocus(); + await user.tab(); + expect(secondInput).toHaveFocus(); + await user.tab(); + expect(lastInput).toHaveFocus(); + + // Test Shift+Tab + await user.keyboard('{Shift>}{Tab}{/Shift}'); + expect(secondInput).toHaveFocus(); });
688-711
: 建议补充箭头键导航的边缘场景测试当前测试验证了基本的循环导航,建议添加以下场景:
- 连续快速按键的处理
- 同时按下多个方向键的行为
- Home/End 键的支持
it('should handle circular navigation with arrow keys', async () => { const user = userEvent.setup(); const onChange = jest.fn(); render( <Segmented options={['iOS', 'Android', 'Web']} onChange={onChange} />, ); await user.tab(); await user.tab(); // 现有测试... + // 测试快速连续按键 + await user.keyboard('{ArrowRight>3}'); + expect(onChange).toHaveBeenCalledTimes(3); + + // 测试Home/End键 + await user.keyboard('{Home}'); + expect(onChange).toHaveBeenCalledWith('iOS'); + await user.keyboard('{End}'); + expect(onChange).toHaveBeenCalledWith('Web'); });
713-729
: 建议增加无障碍性测试当前测试验证了禁用状态的基本行为,建议添加以下无障碍性测试:
- ARIA属性的验证
- 屏幕阅读器状态的验证
it('should skip Tab navigation when disabled', async () => { const user = userEvent.setup(); const { container } = render( <Segmented options={['Daily', 'Weekly', 'Monthly']} disabled />, ); const segmentedContainer = container.querySelector('.rc-segmented'); await user.tab(); expect(segmentedContainer).not.toHaveFocus(); expect(segmentedContainer?.getAttribute('tabIndex')).toBeNull(); + + // 验证ARIA属性 + expect(segmentedContainer).toHaveAttribute('aria-disabled', 'true'); + + // 验证所有选项的ARIA状态 + const options = container.querySelectorAll('.rc-segmented-item'); + options.forEach(option => { + expect(option).toHaveAttribute('aria-disabled', 'true'); + }); });
730-755
: 建议完善禁用选项导航测试当前测试验证了基本的禁用选项跳过逻辑,建议添加以下测试场景:
- 所有选项都禁用时的处理
- 第一个选项禁用时的处理
- 最后一个选项禁用时的处理
describe('Segmented keyboard navigation', () => { + it('should handle all options disabled', async () => { + const user = userEvent.setup(); + const onChange = jest.fn(); + render( + <Segmented + options={[ + { label: 'iOS', value: 'iOS', disabled: true }, + { label: 'Android', value: 'Android', disabled: true }, + { label: 'Web', value: 'Web', disabled: true }, + ]} + onChange={onChange} + />, + ); + + await user.tab(); + await user.keyboard('{ArrowRight}'); + expect(onChange).not.toHaveBeenCalled(); + }); + + it('should handle first option disabled', async () => { + const user = userEvent.setup(); + const onChange = jest.fn(); + render( + <Segmented + options={[ + { label: 'iOS', value: 'iOS', disabled: true }, + 'Android', + 'Web', + ]} + onChange={onChange} + />, + ); + + await user.tab(); + await user.keyboard('{ArrowRight}'); + expect(onChange).toHaveBeenCalledWith('Android'); + }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tests/__snapshots__/index.test.tsx.snap
is excluded by!**/*.snap
📒 Files selected for processing (4)
docs/demo/basic.tsx
(1 hunks)package.json
(1 hunks)src/index.tsx
(5 hunks)tests/index.test.tsx
(2 hunks)
🔇 Additional comments (5)
docs/demo/basic.tsx (2)
11-12
: 添加的属性增强了可访问性!
defaultValue
和 name
属性的添加很好地提升了组件的可访问性:
defaultValue
确保了初始状态下的焦点位置name
属性有助于表单识别和屏幕阅读器的正确解读
20-20
: 建议验证垂直布局的键盘导航
虽然添加 name
属性提升了可访问性,但建议确认在垂直布局模式下:
- 上下方向键是否正确响应
- 焦点循环是否符合预期
- Tab键导航是否正常工作
✅ Verification successful
键盘导航功能已经完整实现,无需额外验证
代码分析显示垂直布局的键盘导航已经完整实现并经过充分测试:
- 上下左右方向键可以正确切换选项
- 支持循环导航
- 已处理禁用选项的跳过逻辑
- Tab键焦点管理正常
- 包含完整的键盘导航测试用例
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 检查垂直布局相关的键盘处理代码
ast-grep --pattern 'onKeyDown($$$) {
$$$
}'
Length of output: 47
Script:
#!/bin/bash
# Search for keyboard event handling in Segmented component
rg -A 10 "onKeyDown|handleKeyDown" --type tsx
# Search for vertical mode related code
rg -A 10 "vertical.*mode|direction.*vertical" --type tsx
# Search for keyboard navigation related tests
rg -A 5 "keyboard.*test|test.*keyboard" --type test
Length of output: 245
Script:
#!/bin/bash
# Search for keyboard event handling in Segmented component
rg -A 10 "onKeyDown|handleKeyDown"
# Search for vertical mode related code
rg -A 10 "vertical.*mode|direction.*vertical"
# Search for keyboard navigation related tests
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts -x rg -l "keyboard|keydown|arrow"
Length of output: 2142
Script:
#!/bin/bash
# Check keyboard navigation test content
cat tests/index.test.tsx
# Check if there are any direction/orientation props
rg -A 5 "direction|orientation|block|vertical" src/index.tsx
Length of output: 23354
package.json (1)
58-58
: 新增的测试依赖项看起来不错!
添加 @testing-library/user-event
是一个很好的选择,它能够帮助我们更好地测试键盘交互功能。不过建议验证一下与其他测试库的兼容性。
运行以下脚本来验证测试库之间的版本兼容性:
✅ Verification successful
测试库版本兼容性已确认,无需调整!
根据版本信息分析:
@testing-library/user-event@^14.5.2
与@testing-library/react@^14.2.1
版本匹配,都是 14.x 系列@testing-library/jest-dom@^5.16.5
和jest@^29.3.1
与上述版本完全兼容
这些版本组合在实践中被广泛使用,不会产生兼容性问题。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 描述:检查测试库之间的版本兼容性
# 获取相关依赖的版本信息
echo "检查测试相关依赖版本:"
jq '.devDependencies | {
"user-event": .["@testing-library/user-event"],
"jest-dom": .["@testing-library/jest-dom"],
"testing-library": .["@testing-library/react"],
"jest": .jest
}' package.json
Length of output: 467
src/index.tsx (1)
87-89
: 接口扩展符合无障碍要求
新增的事件处理器接口完善了键盘操作支持,类型定义准确。
tests/index.test.tsx (1)
2-2
: 导入语句正确添加!
引入 userEvent
用于模拟键盘交互测试是符合最佳实践的选择。
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected] |
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
🧹 Outside diff range and nitpick comments (2)
src/index.tsx (2)
188-201
: 建议优化焦点状态管理当前的焦点管理实现可以进一步改进:
- 考虑使用
useCallback
优化handleFocus
和handleBlur
的性能- 添加焦点状态的防抖处理,避免快速切换时的闪烁
- 考虑将焦点状态管理提取到独立的 hook 中
+ const useFocusState = () => { + const [isFocused, setIsFocused] = React.useState(false); + const handleFocus = React.useCallback(() => { + setIsFocused(true); + }, []); + const handleBlur = React.useCallback(() => { + setIsFocused(false); + }, []); + return { isFocused, handleFocus, handleBlur }; + }; - const [isFocused, setIsFocused] = React.useState(false); - const handleFocus = () => { - setIsFocused(true); - }; - const handleBlur = () => { - setIsFocused(false); - }; + const { isFocused, handleFocus, handleBlur } = useFocusState();
275-276
: 建议区分键盘焦点和鼠标焦点样式当前的焦点样式实现可以优化:
- 区分键盘导航和鼠标点击的焦点样式
- 使用
:focus-visible
伪类优化键盘焦点显示className={classNames( segmentedOption.className, `${prefixCls}-item`, { [`${prefixCls}-item-selected`]: segmentedOption.value === rawValue && !thumbShow, - [`${prefixCls}-item-focused`]: - isFocused && segmentedOption.value === rawValue, + [`${prefixCls}-item-focused`]: + isFocused && segmentedOption.value === rawValue, + [`${prefixCls}-item-keyboard-focused`]: + isFocused && segmentedOption.value === rawValue && keyboardActive, }, )}Also applies to: 281-283
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/index.tsx
(5 hunks)
🔇 Additional comments (4)
src/index.tsx (4)
87-89
: 事件处理器的类型定义正确且完整
新增的 onFocus
、onBlur
和 onKeyDown
事件处理器的类型定义符合 React 的标准事件类型。
Also applies to: 100-102
108-109
:
需要重新设计焦点处理逻辑
在 handleChange
中直接调用 onBlur()
会导致焦点闪烁问题。根据之前的反馈,应该:
- 移除在点击时强制失焦的逻辑
- 考虑使用
onKeyboardActive
来专门处理键盘焦点
203-226
:
键盘导航需要完善
当前的键盘导航实现存在以下问题:
- 缺少对禁用选项的跳过逻辑
- 未实现 Home/End 键支持
- 未处理 Enter/Space 键确认选择
- 缺少对 RTL(从右到左)布局的支持
const onOffset = (offset: number) => {
const total = segmentedOptions.length;
- const nextIndex = (currentIndex + offset + total) % total;
+ let nextIndex = currentIndex;
+ do {
+ nextIndex = (nextIndex + offset + total) % total;
+ } while (
+ nextIndex !== currentIndex &&
+ segmentedOptions[nextIndex]?.disabled
+ );
const nextOption = segmentedOptions[nextIndex];
if (nextOption) {
setRawValue(nextOption.value);
onChange?.(nextOption.value);
}
};
const handleKeyDown = (event: React.KeyboardEvent) => {
+ const isRTL = direction === 'rtl';
switch (event.key) {
case 'ArrowLeft':
case 'ArrowUp':
+ event.preventDefault();
- onOffset(-1);
+ onOffset(isRTL ? 1 : -1);
break;
case 'ArrowRight':
case 'ArrowDown':
+ event.preventDefault();
- onOffset(1);
+ onOffset(isRTL ? -1 : 1);
break;
+ case 'Home':
+ event.preventDefault();
+ setRawValue(segmentedOptions[0].value);
+ break;
+ case 'End':
+ event.preventDefault();
+ setRawValue(segmentedOptions[segmentedOptions.length - 1].value);
+ break;
+ case 'Enter':
+ case ' ':
+ event.preventDefault();
+ // 保持当前选中值,触发 onChange
+ onChange?.(rawValue);
+ break;
}
};
Likely invalid or redundant comment.
230-232
: 🛠️ Refactor suggestion
需要完善无障碍属性
建议添加以下无障碍属性以提升可访问性:
- 添加
aria-orientation
属性指示方向 - 添加
aria-live
区域通知选项变化 - 为选项添加更多的 ARIA 属性
<div
role="radiogroup"
aria-label="segmented control"
+ aria-orientation={vertical ? 'vertical' : 'horizontal'}
tabIndex={disabled ? undefined : 0}
+ aria-activedescendant={`${prefixCls}-item-${rawValue}`}
{...divProps}
>
+ <div aria-live="polite" className="sr-only">
+ {`当前选中:${
+ segmentedOptions.find(option => option.value === rawValue)?.label
+ }`}
+ </div>
Likely invalid or redundant comment.
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
🧹 Outside diff range and nitpick comments (1)
src/index.tsx (1)
87-89
: 建议为新增的事件处理器添加 JSDoc 注释为了提高代码的可维护性,建议为新增的事件处理器添加详细的 JSDoc 文档说明,包括参数说明和使用示例。
+/** + * 处理输入框获得焦点事件 + * @param e - 焦点事件对象 + */ onFocus: (e: React.FocusEvent<HTMLInputElement>) => void; +/** + * 处理输入框失去焦点事件 + * @param e - 焦点事件对象(可选) + */ onBlur: (e?: React.FocusEvent<HTMLInputElement>) => void; +/** + * 处理键盘按键事件 + * @param e - 键盘事件对象 + */ onKeyDown: (e: React.KeyboardEvent) => void;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/index.tsx
(5 hunks)
🔇 Additional comments (3)
src/index.tsx (3)
230-232
: 建议完善无障碍属性
当前的无障碍支持可以进一步增强:
- 添加
aria-orientation
属性 - 为选项添加更多的 ARIA 属性
- 考虑添加实时区域通知
<div
role="radiogroup"
aria-label="segmented control"
+ aria-orientation={vertical ? 'vertical' : 'horizontal'}
tabIndex={disabled ? undefined : 0}
{...divProps}
>
108-109
:
建议重构焦点处理逻辑
在 handleChange
中直接调用 onBlur()
可能会导致不符合预期的焦点行为。建议:
- 创建一个新的状态来区分键盘和鼠标操作
- 只在键盘操作时处理焦点样式
+const [isKeyboardActive, setIsKeyboardActive] = React.useState(false);
const handleChange = (event: React.ChangeEvent<HTMLInputElement>) => {
if (disabled) {
return;
}
- onBlur();
+ if (isKeyboardActive) {
+ setIsKeyboardActive(false);
+ }
onChange(event, value);
};
200-226
: 🛠️ Refactor suggestion
建议增强键盘导航功能
当前的键盘导航实现需要以下改进:
- 添加对 Home/End 键的支持
- 处理禁用选项的跳过逻辑
- 添加空格/回车键的确认操作
- 阻止默认的滚动行为
const onOffset = (offset: number) => {
const currentIndex = segmentedOptions.findIndex(
(option) => option.value === rawValue,
);
const total = segmentedOptions.length;
- const nextIndex = (currentIndex + offset + total) % total;
+ let nextIndex = (currentIndex + offset + total) % total;
+
+ // 跳过禁用选项
+ while (segmentedOptions[nextIndex]?.disabled) {
+ nextIndex = (nextIndex + offset + total) % total;
+ if (nextIndex === currentIndex) break;
+ }
const nextOption = segmentedOptions[nextIndex];
- if (nextOption) {
+ if (nextOption && !nextOption.disabled) {
setRawValue(nextOption.value);
onChange?.(nextOption.value);
}
};
const handleKeyDown = (event: React.KeyboardEvent) => {
+ event.preventDefault();
switch (event.key) {
case 'ArrowLeft':
case 'ArrowUp':
onOffset(-1);
break;
case 'ArrowRight':
case 'ArrowDown':
onOffset(1);
break;
+ case 'Home':
+ const firstEnabled = segmentedOptions.findIndex(opt => !opt.disabled);
+ if (firstEnabled >= 0) {
+ setRawValue(segmentedOptions[firstEnabled].value);
+ onChange?.(segmentedOptions[firstEnabled].value);
+ }
+ break;
+ case 'End':
+ const lastEnabled = segmentedOptions.findLastIndex(opt => !opt.disabled);
+ if (lastEnabled >= 0) {
+ setRawValue(segmentedOptions[lastEnabled].value);
+ onChange?.(segmentedOptions[lastEnabled].value);
+ }
+ break;
+ case ' ':
+ case 'Enter':
+ // 触发选中事件
+ break;
}
};
Likely invalid or redundant comment.
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: 1
🧹 Outside diff range and nitpick comments (6)
src/index.tsx (3)
87-91
: 接口设计需要完善类型定义建议为事件处理函数添加更具体的类型定义,以提高代码的可维护性和类型安全性。
- onFocus: (e: React.FocusEvent<HTMLInputElement>) => void; - onBlur: (e?: React.FocusEvent<HTMLInputElement>) => void; - onKeyDown: (e: React.KeyboardEvent) => void; - onKeyUp: (e: React.KeyboardEvent) => void; - onMouseDown: () => void; + onFocus: (e: React.FocusEvent<HTMLInputElement>, value: SegmentedRawOption) => void; + onBlur: (e: React.FocusEvent<HTMLInputElement>, value: SegmentedRawOption) => void; + onKeyDown: (e: React.KeyboardEvent<HTMLInputElement>) => void; + onKeyUp: (e: React.KeyboardEvent<HTMLInputElement>) => void; + onMouseDown: (e: React.MouseEvent<HTMLLabelElement>) => void;Also applies to: 102-106
291-294
: 焦点样式逻辑可以优化当前的焦点样式判断逻辑过于复杂,建议简化并提供更好的用户体验。
- [`${prefixCls}-item-focused`]: - isFocused && - isKeyboard && - segmentedOption.value === rawValue, + [`${prefixCls}-item-focused`]: isFocused && segmentedOption.value === rawValue, + [`${prefixCls}-item-keyboard-focused`]: isFocused && isKeyboard && segmentedOption.value === rawValue,
299-303
: 事件处理函数绑定需要优化性能建议使用
useCallback
包装事件处理函数以避免不必要的重渲染。+ const memoizedHandleFocus = React.useCallback(() => handleFocus(), []); + const memoizedHandleBlur = React.useCallback(() => handleBlur(), []); + const memoizedHandleKeyDown = React.useCallback((e: React.KeyboardEvent) => handleKeyDown(e), []); + const memoizedHandleKeyUp = React.useCallback((e: React.KeyboardEvent) => handleKeyUp(e), []); + const memoizedHandleMouseDown = React.useCallback(() => handleMouseDown(), []); <InternalSegmentedOption // ...其他属性 - onFocus={handleFocus} - onBlur={handleBlur} - onKeyDown={handleKeyDown} - onKeyUp={handleKeyUp} - onMouseDown={handleMouseDown} + onFocus={memoizedHandleFocus} + onBlur={memoizedHandleBlur} + onKeyDown={memoizedHandleKeyDown} + onKeyUp={memoizedHandleKeyUp} + onMouseDown={memoizedHandleMouseDown} />tests/index.test.tsx (3)
669-768
: 键盘导航测试用例设计全面测试用例覆盖了以下关键场景:
- Tab 键焦点管理
- 箭头键循环导航
- 禁用状态的 Tab 键跳过
- 带有禁用选项的键盘导航
- 点击时的焦点样式
建议补充以下测试场景:
- 垂直方向的键盘导航(上下箭头键)
- 首尾项的 Home/End 键导航
describe('Segmented keyboard navigation', () => { + it('should handle vertical navigation with up/down arrow keys', async () => { + const user = userEvent.setup(); + const onChange = jest.fn(); + render( + <Segmented + options={['iOS', 'Android', 'Web']} + onChange={onChange} + vertical + />, + ); + + await user.tab(); + await user.tab(); + + await user.keyboard('{ArrowDown}'); + expect(onChange).toHaveBeenCalledWith('Android'); + await user.keyboard('{ArrowUp}'); + expect(onChange).toHaveBeenCalledWith('iOS'); + }); + + it('should handle Home/End key navigation', async () => { + const user = userEvent.setup(); + const onChange = jest.fn(); + render( + <Segmented options={['iOS', 'Android', 'Web']} onChange={onChange} />, + ); + + await user.tab(); + await user.tab(); + + await user.keyboard('{End}'); + expect(onChange).toHaveBeenCalledWith('Web'); + await user.keyboard('{Home}'); + expect(onChange).toHaveBeenCalledWith('iOS'); + }); });
670-686
: Tab 键焦点管理测试完善测试用例验证了组件容器和第一个选项的焦点获取逻辑。建议增加验证 Tab 键离开组件的场景。
it('should be focusable through Tab key', async () => { const user = userEvent.setup(); const { getByRole, container } = render( - <Segmented options={['Daily', 'Weekly', 'Monthly']} />, + <> + <button>Before</button> + <Segmented options={['Daily', 'Weekly', 'Monthly']} /> + <button>After</button> + </>, ); const segmentedContainer = getByRole('radiogroup'); const inputs = container.querySelectorAll('.rc-segmented-item-input'); const firstInput = inputs[0]; + const afterButton = container.querySelector('button:last-child'); await user.tab(); // segmented container should be focused expect(segmentedContainer).toHaveFocus(); await user.tab(); // first segmented item should be focused expect(firstInput).toHaveFocus(); + await user.tab(); + // focus should move to the next focusable element + expect(afterButton).toHaveFocus(); });
730-755
: 禁用选项的键盘导航测试逻辑严谨测试用例很好地验证了键盘导航时跳过禁用选项的功能。建议增加验证直接点击禁用选项的场景。
it('should handle keyboard navigation with disabled options', async () => { const user = userEvent.setup(); const onChange = jest.fn(); - render( + const { container } = render( <Segmented options={[ 'iOS', { label: 'Android', value: 'Android', disabled: true }, 'Web', ]} defaultValue="iOS" onChange={onChange} />, ); await user.tab(); await user.tab(); await user.keyboard('{ArrowRight}'); expect(onChange).toHaveBeenCalledWith('Web'); onChange.mockClear(); await user.keyboard('{ArrowLeft}'); expect(onChange).toHaveBeenCalledWith('iOS'); + + // Verify clicking disabled option doesn't trigger onChange + await user.click(container.querySelectorAll('.rc-segmented-item-input')[1]); + expect(onChange).not.toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/index.tsx
(5 hunks)tests/index.test.tsx
(2 hunks)
🔇 Additional comments (3)
src/index.tsx (2)
231-242
:
键盘事件处理需要完善
当前的键盘事件处理缺少以下功能:
- 阻止默认行为
- Home/End 键支持
- Enter/Space 键确认选择
const handleKeyDown = (event: React.KeyboardEvent) => {
switch (event.key) {
case 'ArrowLeft':
case 'ArrowUp':
+ event.preventDefault();
onOffset(-1);
break;
case 'ArrowRight':
case 'ArrowDown':
+ event.preventDefault();
onOffset(1);
break;
+ case 'Home':
+ event.preventDefault();
+ const firstEnabled = segmentedOptions.find(option => !option.disabled);
+ if (firstEnabled) {
+ setRawValue(firstEnabled.value);
+ onChange?.(firstEnabled.value);
+ }
+ break;
+ case 'End':
+ event.preventDefault();
+ const lastEnabled = [...segmentedOptions].reverse().find(option => !option.disabled);
+ if (lastEnabled) {
+ setRawValue(lastEnabled.value);
+ onChange?.(lastEnabled.value);
+ }
+ break;
+ case 'Enter':
+ case ' ':
+ event.preventDefault();
+ // 保持当前选择
+ break;
}
};
Likely invalid or redundant comment.
246-248
: 🛠️ Refactor suggestion
无障碍属性需要增强
当前的无障碍支持可以进一步完善:
- 添加方向指示
- 添加选项计数
- 完善 ARIA 标签
<div
role="radiogroup"
- aria-label="segmented control"
+ aria-label={`分段控制器,共 ${segmentedOptions.length} 个选项`}
+ aria-orientation={vertical ? 'vertical' : 'horizontal'}
+ aria-activedescendant={`${prefixCls}-item-${rawValue}`}
tabIndex={disabled ? undefined : 0}
Likely invalid or redundant comment.
tests/index.test.tsx (1)
2-2
: 导入 userEvent 用于键盘交互测试
新增的 userEvent 导入符合测试键盘交互的最佳实践。
@afc163 大佬,segmented是不是该发个版本了 |
🔥 PR Description
功能描述
为 Segmented 组件添加键盘导航和焦点管理功能,提升组件的可访问性和用户体验。
截图
主要改动
添加键盘导航支持
焦点状态管理
Summary by CodeRabbit
Summary by CodeRabbit
新功能
改进
依赖更新
@testing-library/user-event
,以支持测试中的用户交互。