fix(Safari): Event execution sequence #901
Conversation
📝 WalkthroughWalkthrough本次更改主要针对 Changes
Sequence Diagram(s)sequenceDiagram
participant User as 用户
participant Input as 输入框
participant ForwardSender as ForwardSender组件
User->>Input: 输入中文（使用IME）
Input->>ForwardSender: onCompositionStart
ForwardSender->>ForwardSender: 标记isCompositionRef为true
User->>Input: 结束组合输入（onCompositionEnd）
Input->>ForwardSender: onCompositionEnd
ForwardSender->>ForwardSender: 根据条件设置isCompositionEndRef
User->>Input: 按下回车（onKeyDown/onKeyPress）
Input->>ForwardSender: onKeyDown，记录pendingKeyDownRef
Input->>ForwardSender: onKeyPress
ForwardSender->>ForwardSender: 判断是否允许提交
alt 允许提交
ForwardSender->>ForwardSender: 执行提交逻辑
else 禁止提交
ForwardSender->>ForwardSender: 不提交
end
Assessment against linked issues
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
|
![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)
components/sender/index.tsx (2)
104-104: 考虑更稳健的浏览器检测方法虽然用户代理检测在这种特定的浏览器 bug 修复场景中是可以理解的，但建议考虑以下改进：
- 添加注释说明此检测方法的局限性和使用原因
- 考虑使用特性检测作为备选方案
- 验证正则表达式是否能正确匹配所有 Safari 版本
+// Safari 浏览器检测：用于修复组合事件执行顺序差异 (issue #872) +// 注意：用户代理检测可能在未来版本中失效，需要定期验证 const isSafari = /^((?!chrome|android).)*safari/i.test(navigator.userAgent);
250-252: 建议添加注释说明 Safari 特殊处理逻辑代码逻辑正确，但建议添加注释说明为什么需要在 Safari 中强制重置组合状态，以便后续维护。
+ // Safari 修复：由于事件执行顺序差异，需要在按键事件中重置组合状态 if (isSafari) { isCompositionRef.current = false; }另外，考虑是否需要添加其他清理机制来处理可能的边缘情况，例如在组件卸载时重置状态。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/sender/index.tsx(3 hunks)
🔇 Additional comments (1)
components/sender/index.tsx (1)
224-226: Safari 组合事件处理逻辑合理针对 Safari 浏览器事件执行顺序差异的修复是合理的。通过在组合结束时不重置状态，确保后续按键事件能正确判断组合状态。
Bundle ReportBundle size has no change ✅ |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #901 +/- ##
==========================================
- Coverage 92.53% 92.00% -0.54%
==========================================
Files 69 69
Lines 1554 1575 +21
Branches 401 425 +24
==========================================
+ Hits 1438 1449 +11
- Misses 116 126 +10 ☔ 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 (2)
.dumi/hooks/useIsSafari.tsx (2)
5-6: 考虑命名约定和 SSR 兼容性由于此函数不使用任何 React hooks（如 useState、useEffect），建议重命名为
isSafari以遵循标准命名约定。另外，需要考虑服务端渲染环境下navigator可能不存在的情况。-const useIsSafari = (): boolean => { - const ua = navigator.userAgent; +const useIsSafari = (): boolean => { + // Handle SSR environment + if (typeof window === 'undefined' || typeof navigator === 'undefined') { + return false; + } + const ua = navigator.userAgent;
8-19: 优化性能并增强检测逻辑当前的检测逻辑准确且全面，但存在性能优化空间。建议使用 useMemo 缓存检测结果，避免重复计算。同时，可以考虑添加更多的 Safari 特征检测。
+import { useMemo } from 'react'; + const useIsSafari = (): boolean => { - const ua = navigator.userAgent; - - const isSafari = - // 1. Check if the User-Agent contains "Safari" - /Safari/.test(ua) && - // 2. Exclude browsers that use WebKit but are not Safari (e.g., Chrome, Edge, etc.) - !/Chrome|CriOS|Chromium|Edg/.test(ua) && - // 3. Additional Safari-specific checks: - - // a) Check for the `window.safari` object (Safari's proprietary API) - (typeof (window as any).safari !== 'undefined' || - // b) Check for Safari-specific CSS support (`-webkit-touch-callout` is Safari-only) - CSS.supports('-webkit-touch-callout', 'none')); - return isSafari; + return useMemo(() => { + // Handle SSR environment + if (typeof window === 'undefined' || typeof navigator === 'undefined') { + return false; + } + + const ua = navigator.userAgent; + + const isSafari = + // 1. Check if the User-Agent contains "Safari" + /Safari/.test(ua) && + // 2. Exclude browsers that use WebKit but are not Safari (e.g., Chrome, Edge, etc.) + !/Chrome|CriOS|Chromium|Edg/.test(ua) && + // 3. Additional Safari-specific checks: + + // a) Check for the `window.safari` object (Safari's proprietary API) + (typeof (window as any).safari !== 'undefined' || + // b) Check for Safari-specific CSS support (`-webkit-touch-callout` is Safari-only) + CSS.supports('-webkit-touch-callout', 'none')); + return isSafari; + }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.dumi/hooks/useIsSafari.tsx(1 hunks)
🔇 Additional comments (2)
.dumi/hooks/useIsSafari.tsx (2)
1-4: 文档清晰准确JSDoc 注释准确描述了函数功能和检测策略，解释了多重检测方法的使用原因。
20-22: 导出方式符合预期默认导出方式合适，便于在其他组件中导入使用。
.dumi/hooks/useIsSafari.tsx
Outdated
| // 1. Check if the User-Agent contains "Safari" | ||
| /Safari/.test(ua) && | ||
| // 2. Exclude browsers that use WebKit but are not Safari (e.g., Chrome, Edge, etc.) | ||
| !/Chrome|CriOS|Chromium|Edg/.test(ua) && |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/sender/index.tsx (2)
234-247: 事件顺序处理逻辑实现正确代码正确处理了 Chrome 和 Safari 的不同事件执行顺序，通过
isCompositionRef和isCompositionEndRef标志位区分两种场景。注释清晰地说明了两种事件顺序。可以考虑将注释翻译为中文以保持一致性：
- // Parameter isCompositionRef.current is the event execution: onInternalCompositionStart -> onInternalKeyPress -> onInternalCompositionEnd - // Parameter isCompositionEndRef.current is the event execution: onInternalCompositionStart -> onInternalCompositionEnd -> onInternalKeyPress + // isCompositionRef.current 对应的事件执行顺序：onInternalCompositionStart -> onInternalKeyPress -> onInternalCompositionEnd (Chrome) + // isCompositionEndRef.current 对应的事件执行顺序：onInternalCompositionStart -> onInternalCompositionEnd -> onInternalKeyPress (Safari)
216-217: 考虑添加 ref 清理逻辑
pendingKeyDownRef存储了键盘事件对象，虽然在组合输入结束时会清理（第 242 行），但建议在组件卸载时也进行清理，以避免潜在的内存问题。建议在组件中添加清理逻辑：
+ React.useEffect(() => { + return () => { + // 组件卸载时清理 ref + pendingKeyDownRef.current = null; + }; + }, []);Also applies to: 275-283
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/sender/index.tsx(3 hunks)
🔇 Additional comments (5)
components/sender/index.tsx (5)
216-217: 新增的 ref 声明看起来合理新增的
isCompositionEndRef和pendingKeyDownRef用于跟踪组合输入结束状态和待处理的键盘事件，这对于处理 Safari 和 Chrome 之间的事件顺序差异是必要的。
249-273: 提取的键盘事件处理逻辑合理将原有的键盘按键处理逻辑提取到
handleActualKeyPress函数中，并通过isCanSubmit参数控制是否允许提交，这很好地解决了组合输入时回车键不应直接提交的问题。
386-386: 正确连接了 keydown 事件处理器将
onInternalKeyDown添加到InputTextArea组件是必要的，用于捕获和记录键盘事件。
275-283:❓ Verification inconclusive
新增的 keydown 处理器实现合理
记录 keydown 事件用于后续的 CapsLock 判断是必要的。过滤 'Unidentified' 键可以避免记录无效事件。
验证 'Unidentified' 键的处理是否覆盖了所有需要的场景：
🏁 Script executed:
#!/bin/bash # 搜索代码库中其他处理 'Unidentified' 键的地方，了解这种处理方式是否一致 rg -A 3 "Unidentified" --type tsx --type tsLength of output: 71
🏁 Script executed:
#!/bin/bash # 在所有 .ts 和 .tsx 文件中搜索 'Unidentified' rg -A 3 "Unidentified" -g '*.ts' -g '*.tsx'Length of output: 273
新增的 keydown 处理器实现合理，但需确认过滤覆盖面
我在所有
.ts/.tsx文件中全局搜索，仅在components/sender/index.tsx发现对e.key !== 'Unidentified'的过滤逻辑，未见其他文件中有类似处理。请人工确认在所有目标浏览器和输入场景（如组合输入、非英文键盘、移动设备等）下，KeyboardEvent.key为"Unidentified"的情况是否都应被忽略，以避免遗漏合法按键事件或误过滤。
229-232:❓ Verification inconclusive
CapsLock 处理逻辑需要澄清
注释 "After processing the input Chinese, use CapsLock" 不够清晰。为什么 CapsLock 键会重置
isCompositionEndRef？这种特殊处理的具体场景是什么？建议添加更详细的注释说明这个边缘情况。建议改进注释：
- // After processing the input Chinese, use CapsLock + // 在某些输入法中，CapsLock 键可能会触发特殊的组合输入行为 + // 重置 isCompositionEndRef 以避免影响后续输入
🏁 Script executed:
#!/bin/bash # Locate the file path for components/sender/index.tsx filepath=$(fd --type f 'index.tsx' | grep 'components/sender/index.tsx' || true) echo "Found file: $filepath" if [[ -n "$filepath" ]]; then echo "---- Lines 200-260 of $filepath ----" sed -n '200,260p' "$filepath" else echo "Error: components/sender/index.tsx not found" fiLength of output: 2408
🏁 Script executed:
#!/bin/bash # Search for pendingKeyDownRef usage across the repository rg "pendingKeyDownRef" -nLength of output: 378
🏁 Script executed:
#!/bin/bash # Display lines 260-340 for context on pendingKeyDownRef assignment sed -n '260,340p' components/sender/index.tsxLength of output: 2208
请补充 CapsLock 特殊处理的使用场景和原因
当前注释
// After processing the input Chinese, use CapsLock过于简略，建议改为：
const onInternalCompositionEnd = () => { if (isCompositionRef.current) { isCompositionEndRef.current = true; isCompositionRef.current = false; } - // After processing the input Chinese, use CapsLock + // 在部分输入法切换场景中，按下 CapsLock 会触发 compositionEnd 事件， + // 但并非真实文本输入结束，需清除 isCompositionEndRef 以免误判后续按键 if (pendingKeyDownRef.current?.key === 'CapsLock') { isCompositionEndRef.current = false; } };请说明具体是在何种输入法或操作下会出现此问题，方便后续维护和阅读。
|
太复杂了。。。 |
|
我建议把原来的 isCompositionRef 逻辑删掉。 |
The current implementation logic is
Distinguish between As for the judgment of |
中文版模板 / Chinese template
🤔 This is a ...
🔗 Related Issues
fix #872
the execution order of Chrome is
onInternalCompositionStart-onInternalKeyPress-onInternalCompositionEndSafari is
onInternalCompositionStart-onInternalCompositionEnd-onInternalKeyPressso,
isCompositionRef.currentis not work in SafariAfter recognizing that the browser is Safari, I adjusted
isCompositionRef.currentto ensure consistency with the execution order of Chrome💡 Background and Solution
📝 Change Log
Summary by CodeRabbit