[PATCH v2 0/1] MR7701: winemac.drv: Only send key down events to the window's inputContext.
Sending key events through `-[NSTextInputContext handleEvent:]`, as we do in `macdrv_send_text_input_event`, results in doubled characters for certain macOS input sources. The Romaji source does this reliably, as do certain third-party sources. (The built-in dictation does it as well, but we already blacklist that - !5660 - because it remains "active" even when it's not.) I can't find this spelled out directly, but if you read between the lines, it seems clear that you're only supposed to send key *down* events to `-[NSTextInputContext handleEvent:]` and not key ups. Supporting evidence: 1. [The old documentation on the text system](https://developer.apple.com/library/archive/documentation/TextFonts/Conceptu...) only mentions `handleEvent:` being called in the context of `-keyDown:`. 2. [The guide to implementing a custom text view/NSTextInputClient](https://developer.apple.com/library/archive/documentation/TextFonts/Conceptu...) only mentions overriding `-keyDown:` and sending those events to `-handleEvent:`. 3. [iTerm only sends key down events to `-handleEvent:`](https://github.com/gnachman/iTerm2/blob/6134ea0a9d9d0fee5e7d7704fc98efec1fc7...). You have to unpack the logic a bit from there, but `-handleKeyDownEvent:...` is the only method that calls `-handleEventWithCocoa:inputContext:`, which is what (usually) calls `-handleEvent:` on the `inputContext`. 4. [The InputMethodKit method on the IMKServerInput protocol to handle NSEvents directly](https://developer.apple.com/documentation/objectivec/nsobject/1385363-handle...) only mentions getting key down events. So presumably input source authors sometimes do not check the type of the NSEvent they're receiving and process all key events equally, be they downs or ups, which results in the doubled input. This patch fixes the issues with Romaji and the problematic third-party method we've encountered. -- v2: winemac.drv: Only send key down events to the window's inputContext. https://gitlab.winehq.org/wine/wine/-/merge_requests/7701
From: Tim Clem <tclem(a)codeweavers.com> Rename and remove an argument to macdrv_send_text_input to reflect its purpose. Fixes doubled input with certain input sources. --- dlls/winemac.drv/cocoa_window.m | 10 +++++++--- dlls/winemac.drv/keyboard.c | 10 +++++++++- dlls/winemac.drv/macdrv_cocoa.h | 4 ++-- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/dlls/winemac.drv/cocoa_window.m b/dlls/winemac.drv/cocoa_window.m index 0d2862218ec..4ef9c305b61 100644 --- a/dlls/winemac.drv/cocoa_window.m +++ b/dlls/winemac.drv/cocoa_window.m @@ -3977,9 +3977,13 @@ uint32_t macdrv_window_background_color(void) } /*********************************************************************** - * macdrv_send_text_input_event + * macdrv_send_keydown_to_input_source + * + * Sends a key down event to the active window's inputContext so that it can be + * processed by input sources (AKA IMEs). This is only called when there is an + * active non-keyboard input source. */ -void macdrv_send_text_input_event(int pressed, unsigned int flags, int repeat, int keyc, void* himc, int* done) +void macdrv_send_keydown_to_input_source(unsigned int flags, int repeat, int keyc, void* himc, int* done) { OnMainThreadAsync(^{ BOOL ret; @@ -4004,7 +4008,7 @@ void macdrv_send_text_input_event(int pressed, unsigned int flags, int repeat, i // An NSEvent created with +keyEventWithType:... is internally marked // as synthetic and doesn't get sent through input methods. But one // created from a CGEvent doesn't have that problem. - c = CGEventCreateKeyboardEvent(NULL, keyc, pressed); + c = CGEventCreateKeyboardEvent(NULL, keyc, true); CGEventSetFlags(c, localFlags); CGEventSetIntegerValueField(c, kCGKeyboardEventAutorepeat, repeat); event = [NSEvent eventWithCGEvent:c]; diff --git a/dlls/winemac.drv/keyboard.c b/dlls/winemac.drv/keyboard.c index fe4f7d70a10..6f262a20170 100644 --- a/dlls/winemac.drv/keyboard.c +++ b/dlls/winemac.drv/keyboard.c @@ -1239,6 +1239,14 @@ UINT macdrv_ImeProcessKey(HIMC himc, UINT wparam, UINT lparam, const BYTE *key_s if (!macdrv_using_input_method()) return 0; + if (!pressed) + { + /* Only key down events should be sent to the Cocoa input context. Treat + key up events as handled, even though we don't really do anything + with them; all the action happens on the key down. */ + return 1; + } + switch (vkey) { case VK_SHIFT: @@ -1274,7 +1282,7 @@ UINT macdrv_ImeProcessKey(HIMC himc, UINT wparam, UINT lparam, const BYTE *key_s TRACE("flags 0x%08x keyc 0x%04x\n", flags, keyc); - macdrv_send_text_input_event(pressed, flags, repeat, keyc, himc, &done); + macdrv_send_keydown_to_input_source(flags, repeat, keyc, himc, &done); while (!done) NtUserMsgWaitForMultipleObjectsEx(0, NULL, INFINITE, QS_POSTMESSAGE | QS_SENDMESSAGE, 0); return done > 0; diff --git a/dlls/winemac.drv/macdrv_cocoa.h b/dlls/winemac.drv/macdrv_cocoa.h index 20d82b9bef8..917c8103d41 100644 --- a/dlls/winemac.drv/macdrv_cocoa.h +++ b/dlls/winemac.drv/macdrv_cocoa.h @@ -568,8 +568,8 @@ extern void macdrv_order_cocoa_window(macdrv_window w, macdrv_window prev, extern int macdrv_get_view_backing_size(macdrv_view v, int backing_size[2]); extern void macdrv_set_view_backing_size(macdrv_view v, const int backing_size[2]); extern uint32_t macdrv_window_background_color(void); -extern void macdrv_send_text_input_event(int pressed, unsigned int flags, int repeat, int keyc, - void* data, int* done); +extern void macdrv_send_keydown_to_input_source(unsigned int flags, int repeat, int keyc, + void* data, int* done); extern int macdrv_is_any_wine_window_visible(void); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7701
On Thu Apr 3 16:12:56 2025 +0000, Tim Clem wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/7701/diffs?diff_id=168419&start_sha=4470d6f6bbe1bfa6c5194eb1917ec011e9f5e993#ec9dfe2bf80e5cdb35cb0f13c9f06da8984c9a00_4020_4020) I misrepresented it.
The KeyUp event is handled by the ImeProcessKey, but it is not consumed. It does not need to be paired with a KeyDown event. 'Not consumed' means that the ImeProcessKey returns FALSE, and an unmodified WM_KEYUP message is passed to the WinProc. If macdrv_ImeProcessKey return ‘1’ for the KeyUp event, WM_KEYUP:wParam is replaced with VK_PROCESSKEY, which causes NtUserTranslateMessage to call ImmTranslateMessage unnecessarily. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7701#note_99882
On Thu Apr 3 16:15:35 2025 +0000, Tim Clem wrote:
Thank you for your investigation! The issue being related to composition would explain the behavior I've seen.
In addition to this issue, Korean input is currently broken in Wine macOS build. If this issue is fixed, I would like to try upstream. That's a separate problem than this? Could you file a bug with the details? This issue should be fixed first, but they are separate issues. I'll file a bug later.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7701#note_99883
On Thu Apr 3 17:23:33 2025 +0000, Byeongsik Jeon wrote:
I misrepresented it. The KeyUp event is handled by the ImeProcessKey, but it is not consumed. It does not need to be paired with a KeyDown event. 'Not consumed' means that the ImeProcessKey returns FALSE, and an unmodified WM_KEYUP message is passed to the WinProc. If macdrv_ImeProcessKey return ‘1’ for the KeyUp event, WM_KEYUP:wParam is replaced with VK_PROCESSKEY, which causes NtUserTranslateMessage to call ImmTranslateMessage unnecessarily. Ah, thanks, I see now - I didn't follow things all the way through imm32 & user32. I admit that's getting out of my zone of knowledge. So it's ok for the key down to get translated by ImmTranslateMessage, but the key up to come through as a raw WM_KEYUP? That works in my quick testing. I wonder if @rbernon has any thoughts as to whether there might be subtle repercussions.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7701#note_99886
On Thu Apr 3 17:34:35 2025 +0000, Tim Clem wrote:
Ah, thanks, I see now - I didn't follow things all the way through imm32 & user32. I admit that's getting out of my zone of knowledge. So it's ok for the key down to get translated by ImmTranslateMessage, but the key up to come through as a raw WM_KEYUP? That works in my quick testing. I wonder if @rbernon has any thoughts as to whether there might be subtle repercussions. Yes. This will match the native behavior.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7701#note_99890
participants (3)
-
Byeongsik Jeon (＠bsjeon) -
Tim Clem
-
Tim Clem (＠tclem)