fix(spx-gui): fix input issue #2291
Conversation
Summary of ChangesHello @Overu, 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 resolves a critical input issue where characters could be lost when users were typing with an Input Method Editor (IME). By implementing a mechanism to track the IME's composition state, the system now intelligently defers input validation. This ensures a smoother and more reliable typing experience, preventing unintended component updates from interfering with text input during active composition. Highlights
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 aims to fix an issue with Input Method Editors (IMEs) by preventing validation from running during composition. The approach of using an isComposing flag is sound. However, I've found a critical issue in its implementation: the flag is initialized to true, which will disable input validation for all users not using an IME. My review includes a specific comment with a suggested fix for this issue.
| * Debounce is used to reduce validation frequency in normal scenarios. | ||
| * Effectively avoids unexpected updates and character loss during rapid IME input. | ||
| */ | ||
| let isComposing = true |
There was a problem hiding this comment.
The isComposing flag is initialized to true. This will prevent validation from running for users who are not using an Input Method Editor (IME), as the compositionend event that sets it to false will never fire for them. This effectively disables validation on input for non-IME users. The flag should be initialized to false.
let isComposing = false
![niupilot[bot]](https://avatars.githubusercontent.com/in/1894735?s=60&v=4){kind=small}
| * Debounce is used to reduce validation frequency in normal scenarios. | ||
| * Effectively avoids unexpected updates and character loss during rapid IME input. | ||
| */ | ||
| let isComposing = true |
There was a problem hiding this comment.
Critical Bug: Incorrect Initial State
| let isComposing = true | |
| let isComposing = false |
The isComposing flag is initialized to true, which blocks validation for all users until a composition event occurs. This breaks form validation for non-IME users. Should be false by default.
| * Debounce is used to reduce validation frequency in normal scenarios. | ||
| * Effectively avoids unexpected updates and character loss during rapid IME input. | ||
| */ | ||
| let isComposing = true |
There was a problem hiding this comment.
Suggestion: Use Reactive State
Consider using Vue's reactive state for better integration with the reactivity system:
| let isComposing = true | |
| const isComposing = ref(false) |
Then update the handlers and condition to use .value.
| function handleCompositionStart() { | ||
| isComposing = true | ||
| } | ||
| function handleCompositionEnd() { |
There was a problem hiding this comment.
Security Enhancement: Validate After Composition
Consider triggering validation when composition ends to ensure timely validation:
| function handleCompositionEnd() { | |
| function handleCompositionEnd() { | |
| isComposing = false | |
| // Trigger validation after composition ends | |
| if (props.path != null) { | |
| handleContentInput() | |
| } | |
| } |
Code Review SummaryOverall Assessment: Good solution for the IME input issue with excellent documentation. One critical bug needs fixing. Critical Issue: The Positives: Clean implementation, great documentation, maintains performance with debouncing, follows Vue.js best practices. The fix effectively addresses issue #2089 while maintaining code quality. Please address the initialization bug before merging. |
| } | ||
| function handleCompositionEnd() { | ||
| isComposing = false | ||
| handleContentInput() |
There was a problem hiding this comment.
本来 compositionend 事件触发时（或触发后），n-input 会调用 handleContentInput 吗？另外 handleContentChange 不能解决问题的具体原因是啥来着，是我们对它的定位理解有偏差吗？
There was a problem hiding this comment.
n-input在compositionend后会调用handleInput，然后handleInput内部会调用到handleContentInput，然后调用到我们的触发表单校验，这个时候如果用户随即开始输入，会发生输入进行中和组件更新，导致吞字发生。handleContentChange只有在失去焦点和submit的时候触发。
There was a problem hiding this comment.
n-input在compositionend后会调用handleInput，然后handleInput内部会调用到handleContentInput
那这里的 handleContentInput() 听起来跟 n-input 逻辑重复了，是需要的吗？
There was a problem hiding this comment.
仔细考虑了一下，是没有必要，因为n-input已经帮我们兼容了浏览器差异
| :label="label" | ||
| :path="path" | ||
| v-bind="nFormItemProps" | ||
| @compositionstart="handleCompositionStart" |
There was a problem hiding this comment.
另外我们真正关心的应该是具体的 input 的 compositionstart 和 compositionend 事件，这里可以在 NFormItem 上监听是单纯地因为事件走常规的 DOM 事件冒泡所以能在这里被监听到，还是 naive-ui 内部特别支持了在 NFormItem 上监听这俩事件？
There was a problem hiding this comment.
是常规的事件冒泡，naive-ui没有特别在NFormItem内部支持这两个事件。
|
This PR has been deployed to the preview environment. You can explore it using the preview URL. Warning Please note that deployments in the preview environment are temporary and will be automatically cleaned up after a certain period. Make sure to explore it before it is removed. For any questions, contact the XBuilder team. |
Fixed #2089