Handle IME composition correctly in commit title editor #9761
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@tdkn is attempting to deploy a commit to the GitButler Team on Vercel. A member of the Team first needs to authorize it. |
|
Thanks so much for tackling this! The PR looks great to me, but then again, my previous one also did ;). Let's wait for the professionals to arrive. Lastly, I find it quite shocking that the company that touts accessibility messes this up and either broke it recently, or still didn't fix something as simple as a boolean flag. Probably there is a whole story behind that if one would be digging into it. |
|
Great PR! I've also noticed this issue when using Chinese IME, though compared to Japanese IME (where Enter is commonly used to confirm input), Chinese IME tends to use number keys for candidate selection, so the impact is relatively smaller. Kapture.2025-08-10.at.19.09.45.mp4 |
|
Thanks so much for chiming in @nshcr! I was wondering if this fix would be universal and kind of assumed so, but as we all know, assumptions are errors (albeit convenient). Now I wonder how it's possible to implement this correctly? In theory, it doesn't matter if IME composition is ongoing, but it matters if something in that composition is selectable using the enter key. In Chinese, that's generally not the case. Would that mean we have to know which language is being typed to decide if Enter is special or not? |
|
@Byron In fact, the approach you used in your previous PR (#9615) with the agent mode was correct. |
|
@nshcr @Byron
Also, based on my testing, it seems that the following fix may also resolve the Chinese input issue that @nshcr encountered. diff --git a/apps/desktop/src/components/CommitMessageEditor.svelte b/apps/desktop/src/components/CommitMessageEditor.svelte
index b35f67569..165cfe25a 100644
--- a/apps/desktop/src/components/CommitMessageEditor.svelte
+++ b/apps/desktop/src/components/CommitMessageEditor.svelte
@@ -170,12 +170,14 @@
// Track IME composition state manually for WebKit compatibility
isTitleComposing = true;
}}
+ onkeyup={(e: KeyboardEvent) => {
+ isTitleComposing = e.isComposing;
+ }}
onkeydown={async (e: KeyboardEvent) => {
// Prevent focus movement when Enter is pressed during IME composition
// for Japanese/Chinese/Korean input confirmation.
// WebKit sets e.isComposing to false on Enter, so we use isTitleComposing as fallback
if (e.key === 'Enter' && (e.isComposing || isTitleComposing)) {
- isTitleComposing = false;
return;
}
// Submit commit with Ctrl/Cmd + Enter
diff --git a/apps/desktop/src/components/editor/MessageEditorInput.svelte b/apps/desktop/src/components/editor/MessageEditorInput.svelte
index e73064e57..23f6de824 100644
--- a/apps/desktop/src/components/editor/MessageEditorInput.svelte
+++ b/apps/desktop/src/components/editor/MessageEditorInput.svelte
@@ -10,6 +10,7 @@
oninput?: (e: Event) => void;
onchange?: (value: string) => void;
oncompositionstart?: CompositionEventHandler<HTMLTextAreaElement>;
+ onkeyup?: (e: KeyboardEvent) => void;
onkeydown: (e: KeyboardEvent) => void;
testId?: string;
}
@@ -22,6 +23,7 @@
oninput,
onchange,
oncompositionstart,
+ onkeyup,
onkeydown,
testId
}: Props = $props();
@@ -49,6 +51,7 @@
unstyled
onchange={(e) => onchange?.(e.currentTarget.value)}
{oncompositionstart}
+ {onkeyup}
{onkeydown}
/>
{#if isCharCount}CleanShot.2025-08-11.at.08.58.58.mp4To help better understand the differences in |
| if (e.key === 'Enter' || (e.key === 'Tab' && !e.shiftKey)) { | ||
| e.preventDefault(); | ||
| composer?.focus(); | ||
| } | ||
| // Cancel commit with Escape |
There was a problem hiding this comment.
Let's remove these two one-liner comments and keep ones that communicate something that isn't clear from the code itself?
| // Prevent focus movement when Enter is pressed during IME composition | ||
| // for Japanese/Chinese/Korean input confirmation. | ||
| // WebKit sets e.isComposing to false on Enter, so we use isTitleComposing as fallback | ||
| if (e.key === 'Enter' && (e.isComposing || isTitleComposing)) { |
There was a problem hiding this comment.
Do we need to check e.isComposing when we have isTitleComposing from the oncompositionstart event?
| @@ -53,7 +53,8 @@ | |||
| onchange, | |||
| onfocus, | |||
| onblur, | |||
| onkeydown | |||
| onkeydown, | |||
| ...restProps | |||
There was a problem hiding this comment.
We currently don't use restProps really anywhere in our code base, could we replace this with oncompositionstart for the sake of consistency?
|
@tdkn Sorry for my unexplained reply, here is my proposed solution (not fully tested, but I've tried it briefly with both Chinese and Japanese IMEs, and it seems to work well). Subject: [PATCH] diff
---
Index: apps/desktop/src/components/CommitMessageEditor.svelte
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/apps/desktop/src/components/CommitMessageEditor.svelte b/apps/desktop/src/components/CommitMessageEditor.svelte
--- a/apps/desktop/src/components/CommitMessageEditor.svelte (revision 8b4f2ae3dee056b5712c74c12cefcae14747aac8)
+++ b/apps/desktop/src/components/CommitMessageEditor.svelte (date 1754953196615)
@@ -68,6 +68,7 @@
let composer = $state<ReturnType<typeof MessageEditor>>();
let titleInput = $state<HTMLTextAreaElement>();
+ let isComposing = $state(false);
const suggestionsHandler = new CommitSuggestions(aiService, uiState);
const diffInputArgs = $derived<DiffInputContextArgs>(
@@ -165,14 +166,29 @@
onchange={(value) => {
onChange?.({ title: value });
}}
+ oninput={(e: InputEvent) => {
+ isComposing = e.isComposing;
+ }}
onkeydown={async (e: KeyboardEvent) => {
+ if ((e.key === 'Enter' || e.key === 'Escape') && isComposing) {
+ e.preventDefault();
+ isComposing = false;
+ return;
+ }
+ if (['0', '1', '2', '3', '4', '5', '6', '7', '8', '9'].includes(e.key)) {
+ e.preventDefault();
+ if (isComposing === true) {
+ isComposing = false;
+ }
+ return;
+ }
if (e.key === 'Enter' && (e.ctrlKey || e.metaKey)) {
e.preventDefault();
if (title.trim()) {
emitAction();
}
}
- if (e.key === 'Enter' || (e.key === 'Tab' && !e.shiftKey)) {
+ if ((e.key === 'Enter' || (e.key === 'Tab' && !e.shiftKey)) && !isComposing) {
e.preventDefault();
composer?.focus();
}The key points are:
To address this, I added two extra handlers in
This solution is quite similar to your PR, but with fewer code changes. The main differences are:
Of course, it's entirely up to you how you'd like to improve your PR. If you decide to incorporate some of my ideas, feel free to add me as a co-author in your commit — I'd be happy to see that :). EDIT: I've updated it with a more concise implementation, maybe helpful. + if (['Enter', 'Escape', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9'].includes(e.key) && isComposing) {
+ e.preventDefault();
+ isComposing = false;
+ return;
+ } |
|
Thank you all for reviewing the code. |
|
@tdkn I tested the playground you provided in different browsers and now fully understand what you mean. It's definitely an interesting issue. I dug around a bit and found a discussion about it.
An ancient bug still haunting modern developers — and once again, Apple is the one behind it🤔. My proposed solution doesn't rely on BTW, @Byron maybe this is the story you wanted. |
|
Thanks so much for digging into this @nshcr! And yes, I am also shocked by this Safari/WebKit bug, and very surprised by Apple here. Maybe this is the prime reason that most switch to Chrome - after all, it's much more compliant and thus usable. This issue makes handling this consistently difficult, particularly among the various components that seem to special-case the enter key. By the looks of it, we have to continue manually tracking it, while handling |
tdkn
force-pushed
the
fix-ime-composition-focus-shift
branch
from
48a2de1 to
abdc806
Compare
| oninput={(e: Event) => { | ||
| if (e instanceof InputEvent) { | ||
| isComposing = e.isComposing; | ||
| } | ||
| }} |
There was a problem hiding this comment.
I had to use instanceof to check whether it was an InputEvent
FYI: @nshcr
There was a problem hiding this comment.
@tdkn My bad, forget to add this change.
diff --git a/apps/desktop/src/components/editor/MessageEditorInput.svelte b/apps/desktop/src/components/editor/MessageEditorInput.svelte
--- a/apps/desktop/src/components/editor/MessageEditorInput.svelte (revision 8b4f2ae3dee056b5712c74c12cefcae14747aac8)
+++ b/apps/desktop/src/components/editor/MessageEditorInput.svelte (date 1754975641661)
@@ -6,7 +6,7 @@
value: string;
showCount?: boolean;
placeholder?: string;
- oninput?: (e: Event) => void;
+ oninput?: (e: InputEvent) => void;
onchange?: (value: string) => void;
onkeydown: (e: KeyboardEvent) => void;
testId?: string;BTW, ReviewCreation.svelte has the same logic as CommitMessageEditor.svelte, maybe it needs to be changed too?
gitbutler/apps/desktop/src/components/ReviewCreation.svelte
Lines 372 to 396 in d2b4f82
There was a problem hiding this comment.
Changing the argument type of MessageEditorInput.oninput also triggers the following errors. You will need to update the type definitions in the Textarea.svelte file.
When you trace from <MessageEditorInput /> through <Textarea /> to the native <textarea /> element, you’ll find that oninput is typed as Event & { currentTarget: EventTarget & HTMLTextAreaElement }. It isn’t an InputEvent. Therefore, you must cast the Event to an InputEvent. As noted in the official Svelte repo (sveltejs/svelte#9957), Svelte’s type definitions have some flaws 🤔
The Textarea component is used throughout the application, so modifying it feels risky. It would be great to have a way to minimize its impact.
There was a problem hiding this comment.
I missed ReviewCreation.svelte.
I’ll check it!
|
I've updated the code based on everyone's suggestions. |
| if (e.key === 'Enter' && (e.ctrlKey || e.metaKey)) { | ||
| e.preventDefault(); | ||
| if (title.trim()) { | ||
| emitAction(); | ||
| } | ||
| } | ||
| if (e.key === 'Enter' || (e.key === 'Tab' && !e.shiftKey)) { | ||
| if ((e.key === 'Enter' || (e.key === 'Tab' && !e.shiftKey)) && !isComposing) { |
There was a problem hiding this comment.
I was thinking about it later, && !isComposing check here seems unnecessary, in any state where this line is reached, isComposing should always be false.
The only exception is Tab, but when composing, the focus is on the IME, so the keydown event won't be triggered.
There was a problem hiding this comment.
Never mind, this check should stay, since pressing Tab during composition can move the focus.
|
It's a bit hard for me to understand when this PR is ready for review given all the work going in, so I put it back into draft state. |
Add composition state tracking to CommitMessageEditor and ReviewCreation components to prevent keyboard shortcuts from interfering with IME input. Track isComposing state and disable Enter/Tab/Escape navigation while users are typing with input methods like Asian keyboards. Co-authored-by: Byron <[email protected]> Co-authored-by: nshcr <[email protected]>
tdkn
force-pushed
the
fix-ime-composition-focus-shift
branch
from
abdc806 to
56cc986
Compare
There was a problem hiding this comment.
@nshcr
I applied the same updates to ReviewCreation.svelte as I did to CommitMessageEditor.svelte.
After confirming input with the number keys, pressing Enter moves the focus.
CleanShot.2025-08-12.at.23.10.37.mp4
|
I believe the fixes for the original issue are complete. I’ll switch this PR back to Ready for review. If similar issues are found elsewhere, it would be great to open new issues and address them in dedicated follow-up PRs. In my experience, trying to fix too many things in a single PR increases the risk of regressions. |
There was a problem hiding this comment.
That's awesome work, and it's great to have the fix finally.
I was wondering if it already makes sense to re-use as much of the logic to make it harder to get wrong in all the other input-components that will need it.
Maybe there is some solution already available in a maintained 'package' somewhere.
Or is it common for everyone on earth to fix this by hand over and over?
Besides that, it definitely looks good to me, but I will leave to to others to merge.
🧢 Changes
InputEvent.isComposingproperty☕️ Reasoning
Background
When typing in Japanese, Chinese, or Korean, users need to press the Enter key to confirm character conversion during IME composition. However, GitButler's commit editor was incorrectly interpreting this Enter key press as a signal to move focus to the next input field, disrupting the text input process.
Previous Attempt
PR #9615 attempted to fix this by checking the
isComposingproperty of keyboard events. However, this solution didn't work properly because WebKit browsers (Safari) incorrectly sete.isComposingtofalsewhen Enter is pressed during IME composition, as discovered in this comment. This led to the revert in #9752.Current Solution
This fix tracks the IME composition state using
InputEvent.isComposingfrom theoninputevent and prevents focus changes when:Additionally, it includes a workaround for certain keys (Enter, Escape, number keys) that may terminate IME composition, ensuring the composition state is properly reset.
Technical Details
oninputevent to trackisComposingstate fromInputEventImpact
This fix resolves a critical usability issue that has been preventing CJK (Chinese, Japanese, Korean) users from effectively using GitButler. The inability to properly input text in their native languages when writing commit messages was likely a dealbreaker that caused many potential users from these regions to abandon GitButler. With this fix, GitButler becomes truly accessible to millions of developers across East Asia, opening up a significant user base that was previously unable to adopt the tool.
🎫 Affected issues
Fixes: #9611