Skip to content
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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

aojunhao123
Copy link
Contributor

@aojunhao123 aojunhao123 commented Nov 23, 2024

🔥 PR Description

功能描述

为 Segmented 组件添加键盘导航和焦点管理功能,提升组件的可访问性和用户体验。

截图

segmented-keyboard

主要改动

  1. 添加键盘导航支持

    • 实现循环导航(到达边界时可回到开始/结束位置)
  2. 焦点状态管理

    • 新增焦点状态跟踪
    • 为当前选中项添加焦点样式
    • 点击时优化焦点显示逻辑

Summary by CodeRabbit

Summary by CodeRabbit

  • 新功能

    • 增强了分段选项组件的焦点和键盘导航功能。
    • 新增焦点状态样式,提升用户界面可视性。
    • 新增默认值和名称属性,以便于分段控件的配置。
  • 改进

    • 添加了键盘事件处理,支持使用箭头键在选项间导航。
    • 实现了焦点状态管理,确保用户交互更加流畅。
    • 增加了对键盘导航的测试,确保组件的可用性和交互性。
  • 依赖更新

    • 新增依赖 @testing-library/user-event,以支持测试中的用户交互。

Copy link

coderabbitai bot commented Nov 23, 2024

Walkthrough

此次更改主要涉及对.rc-segmented-item类的样式和InternalSegmentedOption组件的事件处理进行增强。在assets/index.less文件中,新增了聚焦状态的样式,去除了默认的轮廓,并添加了盒子阴影效果,以清晰指示聚焦状态。同时,src/index.tsx文件中增强了键盘导航和焦点管理,包括新增焦点和失去焦点的事件处理,以及对选项的键盘导航支持。

Changes

文件路径 更改摘要
assets/index.less 新增.rc-segmented-item类及其聚焦样式&:focus&-focused
src/index.tsx 修改InternalSegmentedOption组件,新增onFocusonBluronKeyDown属性,增强焦点和键盘事件处理;实现isFocused状态管理,改进键盘导航功能。
docs/demo/basic.tsx Segmented组件中新增defaultValuename属性,以增强配置。
package.json devDependencies中新增依赖@testing-library/user-event版本^14.5.2
tests/index.test.tsx Segmented组件新增键盘导航测试,包括焦点测试和循环导航测试。

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a 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实现提供了基本的焦点指示,但可以考虑以下优化建议:

  1. 增加对比度以提高可见性
  2. 考虑在暗色主题下的表现

建议调整样式如下:

  &-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

📥 Commits

Reviewing files that changed from the base of the PR and between f29b18c and c3e086c.

⛔ 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
Comment on lines 109 to 110
// Do not add focus style when clicking
onBlur();
Copy link

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
Comment on lines 206 to 218
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;
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

当所有选项均被禁用时,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.

Suggested change
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
Comment on lines 223 to 236
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;
}
Copy link

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.keyevent.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.

Copy link

@coderabbitai coderabbitai bot left a 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: 优化事件处理函数参数

handleFocushandleBlur 函数定义了事件参数但未使用,建议移除未使用的参数。

建议修改如下:

-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属性,但建议添加以下补充属性以提供更好的无障碍访问体验:

  1. 为选项添加 aria-checked 属性
  2. 添加 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

📥 Commits

Reviewing files that changed from the base of the PR and between c3e086c and 78bb227.

📒 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: 接口更改符合无障碍访问要求

新增的事件处理属性完善了键盘操作支持,类型定义准确。

Copy link

codecov bot commented Nov 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.37%. Comparing base (f29b18c) to head (6817d51).

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.
📢 Have feedback on the report? Share it here.

Copy link

@coderabbitai coderabbitai bot left a 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: 建议增强焦点管理的健壮性

当前的焦点管理实现较为基础,建议考虑以下增强:

  1. 添加 focusVisible 状态区分键盘和鼠标焦点
  2. 处理组件卸载时的焦点状态清理
+ 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

📥 Commits

Reviewing files that changed from the base of the PR and between 78bb227 and 429831f.

⛔ 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: 添加的属性增强了可访问性!

defaultValuename 属性的添加很好地提升了组件的可访问性:

  • 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.5jest@^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 用于模拟键盘交互测试是符合最佳实践的选择。

src/index.tsx Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a 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: 建议优化焦点状态管理

当前的焦点管理实现可以进一步改进:

  1. 考虑使用 useCallback 优化 handleFocushandleBlur 的性能
  2. 添加焦点状态的防抖处理,避免快速切换时的闪烁
  3. 考虑将焦点状态管理提取到独立的 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: 建议区分键盘焦点和鼠标焦点样式

当前的焦点样式实现可以优化:

  1. 区分键盘导航和鼠标点击的焦点样式
  2. 使用 :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

📥 Commits

Reviewing files that changed from the base of the PR and between 28be306 and fbd0d56.

📒 Files selected for processing (1)
  • src/index.tsx (5 hunks)
🔇 Additional comments (4)
src/index.tsx (4)

87-89: 事件处理器的类型定义正确且完整

新增的 onFocusonBluronKeyDown 事件处理器的类型定义符合 React 的标准事件类型。

Also applies to: 100-102


108-109: ⚠️ Potential issue

需要重新设计焦点处理逻辑

handleChange 中直接调用 onBlur() 会导致焦点闪烁问题。根据之前的反馈,应该:

  1. 移除在点击时强制失焦的逻辑
  2. 考虑使用 onKeyboardActive 来专门处理键盘焦点

203-226: ⚠️ Potential issue

键盘导航需要完善

当前的键盘导航实现存在以下问题:

  1. 缺少对禁用选项的跳过逻辑
  2. 未实现 Home/End 键支持
  3. 未处理 Enter/Space 键确认选择
  4. 缺少对 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

需要完善无障碍属性

建议添加以下无障碍属性以提升可访问性:

  1. 添加 aria-orientation 属性指示方向
  2. 添加 aria-live 区域通知选项变化
  3. 为选项添加更多的 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.

Copy link

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between fbd0d56 and f12a903.

📒 Files selected for processing (1)
  • src/index.tsx (5 hunks)
🔇 Additional comments (3)
src/index.tsx (3)

230-232: 建议完善无障碍属性

当前的无障碍支持可以进一步增强:

  1. 添加 aria-orientation 属性
  2. 为选项添加更多的 ARIA 属性
  3. 考虑添加实时区域通知
 <div
   role="radiogroup"
   aria-label="segmented control"
+  aria-orientation={vertical ? 'vertical' : 'horizontal'}
   tabIndex={disabled ? undefined : 0}
   {...divProps}
 >

108-109: ⚠️ Potential issue

建议重构焦点处理逻辑

handleChange 中直接调用 onBlur() 可能会导致不符合预期的焦点行为。建议:

  1. 创建一个新的状态来区分键盘和鼠标操作
  2. 只在键盘操作时处理焦点样式
+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

建议增强键盘导航功能

当前的键盘导航实现需要以下改进:

  1. 添加对 Home/End 键的支持
  2. 处理禁用选项的跳过逻辑
  3. 添加空格/回车键的确认操作
  4. 阻止默认的滚动行为
 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.

Copy link

@coderabbitai coderabbitai bot left a 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 键跳过
  • 带有禁用选项的键盘导航
  • 点击时的焦点样式

建议补充以下测试场景:

  1. 垂直方向的键盘导航(上下箭头键)
  2. 首尾项的 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

📥 Commits

Reviewing files that changed from the base of the PR and between f12a903 and 6817d51.

📒 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: ⚠️ Potential issue

键盘事件处理需要完善

当前的键盘事件处理缺少以下功能:

  1. 阻止默认行为
  2. Home/End 键支持
  3. 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

无障碍属性需要增强

当前的无障碍支持可以进一步完善:

  1. 添加方向指示
  2. 添加选项计数
  3. 完善 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 导入符合测试键盘交互的最佳实践。

src/index.tsx Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants