[PATCH 0/2] MR5754: server: Don't send raw input events for unicode inputs.
The first commit adds some test infrastructure to be able check raw input keyboard events, along with a few tests relevant to the code paths in this MR. The second commit fixes raw input behavior with unicode inputs, and adds regression tests. Note that it was difficult to express the current Wine behavior with TODOs in the expected sequence, that's why I didn't introduce these regression tests independently. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5754
From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com> --- dlls/user32/tests/input.c | 179 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 174 insertions(+), 5 deletions(-) diff --git a/dlls/user32/tests/input.c b/dlls/user32/tests/input.c index c123f8fa439..d0cfd0fb3e7 100644 --- a/dlls/user32/tests/input.c +++ b/dlls/user32/tests/input.c @@ -111,6 +111,7 @@ enum user_function MSG_TEST_WIN = 1, LL_HOOK_KEYBD, LL_HOOK_MOUSE, + RAW_INPUT_KEYBOARD, }; struct user_call @@ -143,6 +144,12 @@ struct user_call UINT time; UINT_PTR extra; } ll_hook_ms; + struct + { + HWND hwnd; + BYTE code; + RAWKEYBOARD kbd; + } raw_input; }; BOOL todo; @@ -225,6 +232,15 @@ static int ok_call_( const char *file, int line, const struct user_call *expecte if (0 && (ret = expected->ll_hook_ms.time - received->ll_hook_ms.time)) goto done; if ((ret = (expected->ll_hook_ms.extra - received->ll_hook_ms.extra))) goto done; break; + case RAW_INPUT_KEYBOARD: + if ((ret = expected->raw_input.hwnd - received->raw_input.hwnd)) goto done; + if ((ret = expected->raw_input.code - received->raw_input.code)) goto done; + if ((ret = expected->raw_input.kbd.MakeCode - received->raw_input.kbd.MakeCode)) goto done; + if ((ret = expected->raw_input.kbd.Flags - received->raw_input.kbd.Flags)) goto done; + if ((ret = expected->raw_input.kbd.VKey - received->raw_input.kbd.VKey)) goto done; + if ((ret = expected->raw_input.kbd.Message - received->raw_input.kbd.Message)) goto done; + if ((ret = expected->raw_input.kbd.ExtraInformation - received->raw_input.kbd.ExtraInformation)) goto done; + break; } done: @@ -249,6 +265,13 @@ done: wine_dbgstr_point(&received->ll_hook_ms.point), received->ll_hook_ms.data, received->ll_hook_ms.flags, received->ll_hook_ms.time, received->ll_hook_ms.extra ); return ret; + case RAW_INPUT_KEYBOARD: + todo_wine_if( expected->todo || expected->todo_value ) + ok_(file, line)( !ret, "got WM_INPUT key hwnd %p, code %d, make_code %#x, flags %#x, vkey %s, message %s, extra %#lx\n", + received->raw_input.hwnd, received->raw_input.code, received->raw_input.kbd.MakeCode, + received->raw_input.kbd.Flags, debugstr_vk(received->raw_input.kbd.VKey), + debugstr_wm(received->raw_input.kbd.Message), received->raw_input.kbd.ExtraInformation ); + return ret; } switch (expected->func) @@ -270,6 +293,13 @@ done: wine_dbgstr_point(&received->ll_hook_ms.point), received->ll_hook_ms.data, received->ll_hook_ms.flags, received->ll_hook_ms.time, received->ll_hook_ms.extra ); return ret; + case RAW_INPUT_KEYBOARD: + todo_wine_if( expected->todo || expected->todo_value ) + ok_(file, line)( !ret, "got WM_INPUT key hwnd %p, code %d, make_code %#x, flags %#x, vkey %s, message %s, extra %#lx\n", + expected->raw_input.hwnd, expected->raw_input.code, expected->raw_input.kbd.MakeCode, + expected->raw_input.kbd.Flags, debugstr_vk(expected->raw_input.kbd.VKey), + debugstr_wm(expected->raw_input.kbd.Message), expected->raw_input.kbd.ExtraInformation ); + return ret; } return 0; @@ -342,16 +372,49 @@ static void append_ll_hook_ms( UINT msg, const MSLLHOOKSTRUCT *info ) } } +static void populate_raw_input_call( struct user_call *call, HWND hwnd, WPARAM wp, LPARAM lp ) +{ + RAWINPUT *raw; + BYTE data[sizeof(RAWINPUT)]; + UINT data_size = sizeof(data), ret; + HRAWINPUT hri = (HRAWINPUT)lp; + + ret = GetRawInputData( hri, RID_INPUT, data, &data_size, sizeof(RAWINPUTHEADER) ); + ok_ne( ret, (UINT)-1, UINT, "%u" ); + + raw = (RAWINPUT*)data; + if (raw->header.dwType != RIM_TYPEKEYBOARD) return; + + *call = (struct user_call) + { + .func = RAW_INPUT_KEYBOARD, + .raw_input = { .hwnd = hwnd, .code = GET_RAWINPUT_CODE_WPARAM(wp), .kbd = raw->data.keyboard } + }; + if (!append_message_hwnd) call->raw_input.hwnd = 0; +} + static void append_message( HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam ) { - if (!p_accept_message || p_accept_message( msg )) + ULONG index; + struct user_call call = {0}; + + if (p_accept_message && !p_accept_message( msg )) return; + + if (msg == WM_INPUT) populate_raw_input_call( &call, hwnd, wparam, lparam ); + + if (!call.func) { - struct user_call call = {.func = MSG_TEST_WIN, .message = {.hwnd = hwnd, .msg = msg, .wparam = wparam, .lparam = lparam}}; - ULONG index = InterlockedIncrement( ¤t_sequence_len ) - 1; - ok( index < ARRAY_SIZE(current_sequence), "got %lu calls\n", index ); + call = (struct user_call) + { + .func = MSG_TEST_WIN, + .message = {.hwnd = hwnd, .msg = msg, .wparam = wparam, .lparam = lparam} + }; if (!append_message_hwnd) call.message.hwnd = 0; - current_sequence[index] = call; } + + index = InterlockedIncrement( ¤t_sequence_len ) - 1; + ok( index < ARRAY_SIZE(current_sequence), "got %lu calls\n", index ); + current_sequence[index] = call; } static LRESULT CALLBACK append_message_wndproc( HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam ) @@ -1358,6 +1421,111 @@ static void test_keynames(void) } } +static BOOL accept_keyboard_messages_raw( UINT msg ) +{ + return is_keyboard_message( msg ) || msg == WM_INPUT; +} + +static void test_SendInput_raw_key_messages( WORD vkey, WORD wch, HKL hkl ) +{ +#define WIN_MSG(m, w, l, ...) {.func = MSG_TEST_WIN, .message = {.msg = m, .wparam = w, .lparam = l}, ## __VA_ARGS__} +#define RAW_KEY(s, f, v, m, ...) {.func = RAW_INPUT_KEYBOARD, .raw_input.kbd = {.MakeCode = s, .Flags = f, .VKey = v, .Message = m}, ## __VA_ARGS__} +#define KEY_MSG(m, s, v, ...) WIN_MSG( m, v, MAKELONG(1, (s) | (m == WM_KEYUP || m == WM_SYSKEYUP ? (KF_UP | KF_REPEAT) : 0)), ## __VA_ARGS__ ) + struct send_input_keyboard_test raw_legacy[] = + { + {.vkey = vkey, + .expect = {RAW_KEY(1, RI_KEY_MAKE, vkey, WM_KEYDOWN), KEY_MSG(WM_KEYDOWN, 1, vkey), WIN_MSG(WM_CHAR, wch, MAKELONG(1, 1)), {0}}}, + {.vkey = vkey, .flags = KEYEVENTF_KEYUP, + .expect = {RAW_KEY(2, RI_KEY_BREAK, vkey, WM_KEYUP), KEY_MSG(WM_KEYUP, 2, vkey), {0}}}, + {0}, + }; + struct send_input_keyboard_test raw_nolegacy[] = + { + {.vkey = vkey, .expect = {RAW_KEY(1, RI_KEY_MAKE, vkey, WM_KEYDOWN), {0}}}, + {.vkey = vkey, .flags = KEYEVENTF_KEYUP, .expect = {RAW_KEY(2, RI_KEY_BREAK, vkey, WM_KEYUP), {0}}}, + {0}, + }; + struct send_input_keyboard_test raw_vk_packet_legacy[] = + { + {.vkey = VK_PACKET, .expect_state = {[VK_PACKET] = 0x80}, + .expect = {RAW_KEY(1, RI_KEY_MAKE, VK_PACKET, WM_KEYDOWN), KEY_MSG(WM_KEYDOWN, 1, VK_PACKET), {0, .todo = TRUE}}}, + {.vkey = VK_PACKET, .flags = KEYEVENTF_KEYUP, + .expect = {RAW_KEY(2, RI_KEY_BREAK, VK_PACKET, WM_KEYUP), KEY_MSG(WM_KEYUP, 2, VK_PACKET), {0}}}, + {0}, + }; + struct send_input_keyboard_test raw_vk_packet_nolegacy[] = + { + {.vkey = VK_PACKET, .expect = {RAW_KEY(1, RI_KEY_MAKE, VK_PACKET, WM_KEYDOWN), {0}}}, + {.vkey = VK_PACKET, .flags = KEYEVENTF_KEYUP, .expect = {RAW_KEY(2, RI_KEY_BREAK, VK_PACKET, WM_KEYUP), {0}}}, + {0}, + }; +#undef WIN_MSG +#undef RAW_KEY +#undef KEY_MSG + RAWINPUTDEVICE rid = {.usUsagePage = 0x01, .usUsage = 0x06}; + int receive; + HWND hwnd; + + raw_legacy[0].expect_state[vkey] = 0x80; + + hwnd = CreateWindowW( L"static", NULL, WS_POPUP | WS_VISIBLE, 0, 0, 100, 100, NULL, NULL, NULL, NULL ); + ok_ne( NULL, hwnd, HWND, "%p" ); + wait_messages( 100, FALSE ); + + /* If we have had a spurious layout change, wch may be incorrect. */ + if (GetKeyboardLayout( 0 ) != hkl) + { + win_skip( "Spurious keyboard layout changed detected (expected: %p got: %p)\n", + hkl, GetKeyboardLayout( 0 ) ); + ok_ret( 1, DestroyWindow( hwnd ) ); + wait_messages( 100, FALSE ); + ok_seq( empty_sequence ); + return; + } + + p_accept_message = accept_keyboard_messages_raw; + + for (receive = 0; receive <= 1; receive++) + { + winetest_push_context( receive ? "receive" : "peek" ); + + if (receive) + { + /* test received messages */ + LONG_PTR old_proc = SetWindowLongPtrW( hwnd, GWLP_WNDPROC, (LONG_PTR)append_message_wndproc ); + ok_ne( 0, old_proc, LONG_PTR, "%#Ix" ); + } + + rid.dwFlags = 0; + ok_ret( 1, RegisterRawInputDevices( &rid, 1, sizeof(rid) ) ); + + /* get both WM_INPUT and legacy messages */ + check_send_input_keyboard_test( raw_legacy, !receive ); + check_send_input_keyboard_test( raw_vk_packet_legacy, !receive ); + + rid.dwFlags = RIDEV_REMOVE; + ok_ret( 1, RegisterRawInputDevices( &rid, 1, sizeof(rid) ) ); + + rid.dwFlags = RIDEV_NOLEGACY; + ok_ret( 1, RegisterRawInputDevices( &rid, 1, sizeof(rid) ) ); + + /* get only WM_INPUT messages */ + check_send_input_keyboard_test( raw_nolegacy, !receive ); + check_send_input_keyboard_test( raw_vk_packet_nolegacy, !receive ); + + rid.dwFlags = RIDEV_REMOVE; + ok_ret( 1, RegisterRawInputDevices( &rid, 1, sizeof(rid) ) ); + + winetest_pop_context(); + } + + ok_ret( 1, DestroyWindow( hwnd ) ); + wait_messages( 100, FALSE ); + ok_seq( empty_sequence ); + + p_accept_message = NULL; +} + static void test_GetMouseMovePointsEx( char **argv ) { #define BUFLIM 64 @@ -5873,6 +6041,7 @@ static void test_input_desktop( char **argv ) test_RegisterRawInputDevices(); test_GetRawInputData(); test_GetRawInputBuffer(); + test_SendInput_raw_key_messages( 'F', wch, hkl ); test_LoadKeyboardLayoutEx( hkl ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5754
From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com> --- dlls/user32/tests/input.c | 18 ++++++++++++++++++ server/queue.c | 9 +++++---- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/dlls/user32/tests/input.c b/dlls/user32/tests/input.c index d0cfd0fb3e7..3004c409918 100644 --- a/dlls/user32/tests/input.c +++ b/dlls/user32/tests/input.c @@ -1459,6 +1459,20 @@ static void test_SendInput_raw_key_messages( WORD vkey, WORD wch, HKL hkl ) {.vkey = VK_PACKET, .flags = KEYEVENTF_KEYUP, .expect = {RAW_KEY(2, RI_KEY_BREAK, VK_PACKET, WM_KEYUP), {0}}}, {0}, }; + struct send_input_keyboard_test raw_unicode_legacy[] = + { + {.scan = 0x3c0, .flags = KEYEVENTF_UNICODE, .expect_state = {[VK_PACKET] = 0x80}, + .expect = {KEY_MSG(WM_KEYDOWN, 0, VK_PACKET, .todo_value = TRUE), WIN_MSG(WM_CHAR, 0x3c0, 1), {0}}}, + {.scan = 0x3c0, .flags = KEYEVENTF_KEYUP | KEYEVENTF_UNICODE, + .expect = {KEY_MSG(WM_KEYUP, 0, VK_PACKET, .todo_value = TRUE), {0}}}, + {0}, + }; + struct send_input_keyboard_test raw_unicode_nolegacy[] = + { + {.scan = 0x3c0, .flags = KEYEVENTF_UNICODE}, + {.scan = 0x3c0, .flags = KEYEVENTF_KEYUP | KEYEVENTF_UNICODE}, + {0}, + }; #undef WIN_MSG #undef RAW_KEY #undef KEY_MSG @@ -1502,6 +1516,8 @@ static void test_SendInput_raw_key_messages( WORD vkey, WORD wch, HKL hkl ) /* get both WM_INPUT and legacy messages */ check_send_input_keyboard_test( raw_legacy, !receive ); check_send_input_keyboard_test( raw_vk_packet_legacy, !receive ); + /* no WM_INPUT message for unicode */ + check_send_input_keyboard_test( raw_unicode_legacy, !receive ); rid.dwFlags = RIDEV_REMOVE; ok_ret( 1, RegisterRawInputDevices( &rid, 1, sizeof(rid) ) ); @@ -1512,6 +1528,8 @@ static void test_SendInput_raw_key_messages( WORD vkey, WORD wch, HKL hkl ) /* get only WM_INPUT messages */ check_send_input_keyboard_test( raw_nolegacy, !receive ); check_send_input_keyboard_test( raw_vk_packet_nolegacy, !receive ); + /* no WM_INPUT message for unicode */ + check_send_input_keyboard_test( raw_unicode_nolegacy, !receive ); rid.dwFlags = RIDEV_REMOVE; ok_ret( 1, RegisterRawInputDevices( &rid, 1, sizeof(rid) ) ); diff --git a/server/queue.c b/server/queue.c index 6d4396234f2..2532b6ad87a 100644 --- a/server/queue.c +++ b/server/queue.c @@ -2099,11 +2099,12 @@ static int queue_keyboard_message( struct desktop *desktop, user_handle_t win, c unsigned int message_code, time; lparam_t lparam = input->kbd.scan << 16; unsigned int flags = 0; + BOOL unicode = (input->kbd.flags & KEYEVENTF_UNICODE) && !input->kbd.vkey; int wait; if (!(time = input->kbd.time)) time = get_tick_count(); - if (!(input->kbd.flags & KEYEVENTF_UNICODE)) + if (!unicode) { switch (vkey) { @@ -2188,7 +2189,7 @@ static int queue_keyboard_message( struct desktop *desktop, user_handle_t win, c } } - if ((foreground = get_foreground_thread( desktop, win ))) + if (!unicode && (foreground = get_foreground_thread( desktop, win ))) { struct rawinput_message raw_msg = {0}; raw_msg.foreground = foreground; @@ -2205,7 +2206,7 @@ static int queue_keyboard_message( struct desktop *desktop, user_handle_t win, c if ((device = current->process->rawinput_kbd) && (device->flags & RIDEV_NOLEGACY)) { - update_input_key_state( desktop, desktop->keystate, message_code, vkey ); + if (!unicode) update_input_key_state( desktop, desktop->keystate, message_code, vkey ); return 0; } @@ -2216,7 +2217,7 @@ static int queue_keyboard_message( struct desktop *desktop, user_handle_t win, c msg->msg = message_code; if (origin == IMO_INJECTED) msg_data->flags = LLKHF_INJECTED; - if (input->kbd.flags & KEYEVENTF_UNICODE && !vkey) + if (unicode) { vkey = hook_vkey = VK_PACKET; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5754
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=145936 Your paranoid android. === debian11b (64 bit WoW report) === wmp: media: Timeout
Rémi Bernon (@rbernon) commented about server/queue.c:
unsigned int message_code, time; lparam_t lparam = input->kbd.scan << 16; unsigned int flags = 0; + BOOL unicode = (input->kbd.flags & KEYEVENTF_UNICODE) && !input->kbd.vkey; int wait;
if (!(time = input->kbd.time)) time = get_tick_count();
- if (!(input->kbd.flags & KEYEVENTF_UNICODE)) + if (!unicode)
This will now be taken if `input->kbd.vkey` is set, regardless of the flag. I think you should keep the flag check here? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5754#note_71740
Rémi Bernon (@rbernon) commented about dlls/user32/tests/input.c:
{ - struct user_call call = {.func = MSG_TEST_WIN, .message = {.hwnd = hwnd, .msg = msg, .wparam = wparam, .lparam = lparam}}; - ULONG index = InterlockedIncrement( ¤t_sequence_len ) - 1; - ok( index < ARRAY_SIZE(current_sequence), "got %lu calls\n", index ); + call = (struct user_call) + { + .func = MSG_TEST_WIN, + .message = {.hwnd = hwnd, .msg = msg, .wparam = wparam, .lparam = lparam} + }; if (!append_message_hwnd) call.message.hwnd = 0; - current_sequence[index] = call; } + + index = InterlockedIncrement( ¤t_sequence_len ) - 1; + ok( index < ARRAY_SIZE(current_sequence), "got %lu calls\n", index ); + current_sequence[index] = call;
We don't use these struct literals anywhere in Wine, it's not too bad but I'm not sure I want to create a precedent here. As we only receive WM_INPUT when registering for it, what about doing something like: ``` static void append_rawinput_message( HWND hwnd, WPARAM wparam, HRAWINPUT handle ) { RAWINPUT rawinput; UINT size = sizeof(rawinput), ret; ret = GetRawInputData( handle, RID_INPUT, &rawinput, &size, sizeof(RAWINPUTHEADER) ); ok_ne( ret, (UINT)-1, UINT, "%u" ); if (rawinput.header.dwType == RIM_TYPEKEYBOARD) { struct user_call call = { .func = RAW_INPUT_KEYBOARD, .raw_input = {.hwnd = hwnd, .code = GET_RAWINPUT_CODE_WPARAM(wparam), .kbd = rawinput.data.keyboard} }; ULONG index = InterlockedIncrement( ¤t_sequence_len ) - 1; ok( index < ARRAY_SIZE(current_sequence), "got %lu calls\n", index ); if (!append_message_hwnd) call.message.hwnd = 0; current_sequence[index] = call; } } static void append_message( HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam ) { if (msg == WM_INPUT) append_rawinput_message( hwnd, wparam, (HRAWINPUT)lparam ); else if (!p_accept_message || p_accept_message( msg )) { struct user_call call = {.func = MSG_TEST_WIN, .message = {.hwnd = hwnd, .msg = msg, .wparam = wparam, .lparam = lparam}}; ULONG index = InterlockedIncrement( ¤t_sequence_len ) - 1; ok( index < ARRAY_SIZE(current_sequence), "got %lu calls\n", index ); if (!append_message_hwnd) call.message.hwnd = 0; current_sequence[index] = call; } } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5754#note_71741
Rémi Bernon (@rbernon) commented about dlls/user32/tests/input.c:
+ {.vkey = VK_PACKET, .expect_state = {[VK_PACKET] = 0x80}, + .expect = {RAW_KEY(1, RI_KEY_MAKE, VK_PACKET, WM_KEYDOWN), KEY_MSG(WM_KEYDOWN, 1, VK_PACKET), {0, .todo = TRUE}}}, + {.vkey = VK_PACKET, .flags = KEYEVENTF_KEYUP, + .expect = {RAW_KEY(2, RI_KEY_BREAK, VK_PACKET, WM_KEYUP), KEY_MSG(WM_KEYUP, 2, VK_PACKET), {0}}}, + {0}, + }; + struct send_input_keyboard_test raw_vk_packet_nolegacy[] = + { + {.vkey = VK_PACKET, .expect = {RAW_KEY(1, RI_KEY_MAKE, VK_PACKET, WM_KEYDOWN), {0}}}, + {.vkey = VK_PACKET, .flags = KEYEVENTF_KEYUP, .expect = {RAW_KEY(2, RI_KEY_BREAK, VK_PACKET, WM_KEYUP), {0}}}, + {0}, + }; +#undef WIN_MSG +#undef RAW_KEY +#undef KEY_MSG + RAWINPUTDEVICE rid = {.usUsagePage = 0x01, .usUsage = 0x06};
RAWINPUTDEVICE rid = {.usUsagePage = HID_USAGE_PAGE_GENERIC, .usUsage = HID_USAGE_GENERIC_KEYBOARD};
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5754#note_71742
Rémi Bernon (@rbernon) commented about server/queue.c:
if ((device = current->process->rawinput_kbd) && (device->flags & RIDEV_NOLEGACY)) { - update_input_key_state( desktop, desktop->keystate, message_code, vkey ); + if (!unicode) update_input_key_state( desktop, desktop->keystate, message_code, vkey );
```suggestion:-0+0 update_input_key_state( desktop, desktop->keystate, message_code, vkey ); ``` As far as I can see the VK_PACKET is still reflected in the *async* keystate[^1] even when RIDEV_NOLEGACY is used. However, for the raw_nolegacy test, 'F' vkey seems to be missing from the async key state somehow. Yet we don't check the async key state in the tests. This could be added but it would need more changes to the tests as it's not exactly the same as the thread keystate[^2], and it apparently uses a different vkey fixup method. In the meantime I would suggest to keep this like this. The reason why thread keystate isn't updated when RIDEV_NOLEGACY is used is simply because there's no keyboard messages. [^1]: The async keystate is the desktop keystate, and can only be queried for each vkey with `GetAsyncKeyState`. It reflects the immediate hardware state. [^2]: The therad keystate is the state of the keyboard, which we read with `GetKeyboardState` or `GetKeyState`, and is updated when keyboard window messages are received. It's often desync from the desktop keystate when input messages are being processed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5754#note_71743
participants (4)
-
Alexandros Frantzis -
Alexandros Frantzis (＠afrantzis)
-
Marvin -
Rémi Bernon