feat: add util function for common keyboard event handling #715
Conversation
|
@aojunhao123 is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough在 Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 分钟 诗句
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
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 |
Summary of ChangesHello @aojunhao123, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a dedicated utility function to streamline keyboard event management. Its primary purpose is to provide a robust mechanism for applications to easily identify and ignore keyboard events that occur within specific interactive UI components or during text composition, thereby enhancing user experience by preventing global shortcuts or listeners from interfering with standard input operations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new utility function, shouldIgnoreKeyboardEvent, to improve keyboard event handling by ignoring events on specific interactive elements. The implementation is correct and addresses the intended use case. I have one suggestion to refactor a conditional block for better conciseness and readability.
| if ( | ||
| e.isComposing || | ||
| tagName === 'INPUT' || | ||
| tagName === 'TEXTAREA' || | ||
| tagName === 'SELECT' || | ||
| target.isContentEditable | ||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
This conditional block can be simplified into a single return statement. This improves conciseness without sacrificing readability.
return (
e.isComposing ||
tagName === 'INPUT' ||
tagName === 'TEXTAREA' ||
tagName === 'SELECT' ||
target.isContentEditable
);![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/KeyCode.ts (2)
521-523: 建议添加 JSDoc 文档注释为了与文件中其他函数保持一致，并提高 API 的可发现性，建议为此函数添加 JSDoc 注释，说明其用途、参数和返回值。
🔎 建议的文档注释
+ /** + * Check if a keyboard event should be ignored. + * Returns true for events on input elements, editable content, or during IME composition. + * @param e - The keyboard event to check + * @returns true if the event should be ignored, false otherwise + */ shouldIgnoreKeyboardEvent: function shouldIgnoreKeyboardEvent( e: KeyboardEvent, ) {
531-542: 可以简化返回逻辑最后的
return false语句可以省略，直接在条件判断后返回 false，使代码更简洁。🔎 建议的简化
const tagName = target.tagName; - if ( + return ( e.isComposing || tagName === 'INPUT' || tagName === 'TEXTAREA' || tagName === 'SELECT' || target.isContentEditable - ) { - return true; - } - - return false; + ); },
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/KeyCode.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test / react component workflow
🔇 Additional comments (1)
src/KeyCode.ts (1)
531-539: 确认是否应该区分不同的 INPUT 类型的键盘事件处理当前
shouldIgnoreKeyboardEvent函数对所有INPUT元素返回true，但部分 INPUT 类型（如type="button"、type="submit"、type="checkbox"、type="radio"等）通常需要处理键盘事件。建议确认该函数的使用场景和设计意图后，决定是否需要区分不同的 INPUT 类型。
aojunhao123
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #715 +/- ##
==========================================
- Coverage 91.58% 85.92% -5.67%
==========================================
Files 37 38 +1
Lines 939 1016 +77
Branches 314 380 +66
==========================================
+ Hits 860 873 +13
- Misses 77 141 +64
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/KeyCode.ts (1)
528-528: 可选：为target添加显式类型标注。虽然当前代码通过
instanceof检查确保了类型安全，但显式标注类型可以提高代码可读性。🔎 建议的类型标注
- const target = e.target; + const target: EventTarget | null = e.target;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/KeyCode.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test / react component workflow
🔇 Additional comments (3)
src/KeyCode.ts (3)
531-533: 类型守卫实现正确。对
HTMLElement的实例检查确保了后续访问tagName和isContentEditable的类型安全，逻辑合理。
535-544: 可编辑元素检测逻辑正确。该逻辑正确识别了 INPUT、TEXTAREA、SELECT 标签和 contentEditable 元素，配合
checkEditable选项提供了灵活的控制。注意tagName在 HTML 中返回大写字符串，当前的比较方式是正确的。
546-548: IME 组合状态检测实现正确。通过检查
e.isComposing属性处理 IME 输入场景，与 PR 描述中的使用场景（IME 期间不应触发组件内部键盘事件）一致，逻辑准确。
src/KeyCode.ts
Outdated
| shouldIgnoreKeyboardEvent: function shouldIgnoreKeyboardEvent( | ||
| e: KeyboardEvent, | ||
| options?: { | ||
| checkEditable?: boolean; | ||
| checkComposing?: boolean; | ||
| }, | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
补充 JSDoc 文档说明。
此函数作为新增的公共 API，应与文件中其他函数保持一致，添加 JSDoc 注释。建议说明函数用途、参数含义和返回值（例如：返回 true 表示应忽略该键盘事件）。
🔎 建议的 JSDoc 文档
+ /**
+ * Determines whether a keyboard event should be ignored based on the event target and composition state.
+ * @param e - The keyboard event to check
+ * @param options - Optional configuration
+ * @param options.checkEditable - Whether to ignore events on editable elements (default: true)
+ * @param options.checkComposing - Whether to ignore events during IME composition (default: true)
+ * @returns true if the event should be ignored, false otherwise
+ */
shouldIgnoreKeyboardEvent: function shouldIgnoreKeyboardEvent(
e: KeyboardEvent,
options?: {
checkEditable?: boolean;
checkComposing?: boolean;
},
) {📝 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.
| shouldIgnoreKeyboardEvent: function shouldIgnoreKeyboardEvent( | |
| e: KeyboardEvent, | |
| options?: { | |
| checkEditable?: boolean; | |
| checkComposing?: boolean; | |
| }, | |
| ) { | |
| /** | |
| * Determines whether a keyboard event should be ignored based on the event target and composition state. | |
| * @param e - The keyboard event to check | |
| * @param options - Optional configuration | |
| * @param options.checkEditable - Whether to ignore events on editable elements (default: true) | |
| * @param options.checkComposing - Whether to ignore events during IME composition (default: true) | |
| * @returns true if the event should be ignored, false otherwise | |
| */ | |
| shouldIgnoreKeyboardEvent: function shouldIgnoreKeyboardEvent( | |
| e: KeyboardEvent, | |
| options?: { | |
| checkEditable?: boolean; | |
| checkComposing?: boolean; | |
| }, | |
| ) { |
🤖 Prompt for AI Agents
In src/KeyCode.ts around lines 521 to 527, the function
shouldIgnoreKeyboardEvent is missing JSDoc; add a JSDoc block immediately above
the function following the same style as other functions in this file that
explains the function purpose (determine whether a KeyboardEvent should be
ignored), describes parameters (e: KeyboardEvent; options object with optional
boolean checkEditable and checkComposing and what each flag does), and states
the return value (boolean — true means the event should be ignored). Include
@param annotations for each parameter and @returns describing the boolean
result.
src/KeyCode.ts
Outdated
| e: KeyboardEvent, | ||
| options?: { | ||
| checkEditable?: boolean; | ||
| checkComposing?: boolean; |
There was a problem hiding this comment.
editable 应该是默认的，所以不需要这个参数。额外 compsing 检查就行了，不需要 check 前缀。
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/KeyCode.test.ts (2)
22-54: 建议添加 TEXTAREA 和 SELECT 元素的测试覆盖。当前测试覆盖了基本场景，但根据 AI 摘要，
isEditableTarget方法支持 INPUT/TEXTAREA/SELECT 元素。建议添加针对这些元素类型的显式测试用例以确保完整的覆盖。🔎 建议添加的测试用例
+ it('check textarea target', () => { + const textarea = document.createElement('textarea'); + document.body.appendChild(textarea); + + const result = testIsEditableTarget(window, textarea, { bubbles: true }); + + document.body.removeChild(textarea); + + expect(result).toBe(true); + }); + + it('check select target', () => { + const select = document.createElement('select'); + document.body.appendChild(select); + + const result = testIsEditableTarget(window, select, { bubbles: true }); + + document.body.removeChild(select); + + expect(result).toBe(true); + });
35-44: 可选：考虑添加边缘情况的测试覆盖。为了提高测试的健壮性，可以考虑添加以下边缘情况的测试：
- 禁用状态的输入框（
disabled属性）- 只读状态的输入框（
readonly属性）- 不同类型的 input 元素（如
type="button"可能需要不同的处理）这些测试可以帮助确保函数在各种实际使用场景中都能正确工作。
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/KeyCode.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test / react component workflow
🔇 Additional comments (1)
tests/KeyCode.test.ts (1)
4-20: 辅助函数设计良好！这个测试辅助函数结构清晰，正确处理了事件监听、分发和清理。分离监听目标和分发目标的设计很好地支持了事件冒泡的测试场景。
tests/KeyCode.test.ts
Outdated
| }; | ||
|
|
||
| listenTarget.addEventListener('keydown', handler); | ||
| dispatchTarget.dispatchEvent(new KeyboardEvent('keydown', init)); |
There was a problem hiding this comment.
testinglib 有创建 event 的函数，直接写一个 element 再 createEvent 就行了，测试不用太散，直接写一个 lement 数组，遍历测试就行：
const eleList = [
{
element: 'div',
isContentEditable: true,
expect: true,
},
...
];另外 IME composing 这个不用测，因为你代码里也没写
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.