[PATCH v7 0/2] MR5761: comclt32: Allow ctrl-tab and tab hotkeys for propsheets

[Fabian Maurer (＠DarkShadow44)]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56103

--
  v7: comclt32: Allow hotkeys for propsheet navigation.
      comctl32/tests: Add test for propsheet hotkey navigation.

https://gitlab.winehq.org/wine/wine/-/merge_requests/5761

[PATCH v7 1/2] comctl32/tests: Add test for propsheet hotkey navigation.

[Fabian Maurer]

From: Fabian Maurer

---
 dlls/comctl32/tests/propsheet.c | 174 ++++++++++++++++++++++++++++++++
 1 file changed, 174 insertions(+)

diff --git a/dlls/comctl32/tests/propsheet.c b/dlls/comctl32/tests/propsheet.c
index 9bd1c08bd6a..bffae6f99bb 100644
--- a/dlls/comctl32/tests/propsheet.c
+++ b/dlls/comctl32/tests/propsheet.c
@@ -302,6 +302,8 @@ static void test_disableowner(void)
     DestroyWindow(parenthwnd);
 }

+HANDLE active_page_changed;
+
 static INT_PTR CALLBACK nav_page_proc(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam)
 {
     switch(msg){
@@ -312,6 +314,8 @@ static INT_PTR CALLBACK nav_page_proc(HWND hwnd, UINT msg, WPARAM wparam, LPARAM
             case PSN_SETACTIVE:
                 active_page = /* PropSheet_HwndToIndex(hdr->hwndFrom, hwnd); */
                     (int)SendMessageA(hdr->hwndFrom, PSM_HWNDTOINDEX, (WPARAM)hwnd, 0);
+                if (active_page_changed)
+                    SetEvent(active_page_changed);
                 return TRUE;
             case PSN_KILLACTIVE:
                 /* prevent navigation away from the fourth page */
@@ -1388,6 +1392,175 @@ static void test_invalid_hpropsheetpage(void)
     DestroyWindow(hdlg);
 }

+#define WM_DIALOG_LOADED (WM_USER + 0x99)
+
+static WNDPROC old_hotkeys_dialog_proc;
+static HWND hotkey_dialog;
+static HANDLE hotkey_dialog_loaded;
+
+static LRESULT CALLBACK new_hotkeys_dialog_proc(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam)
+{
+    switch(msg)
+    {
+    case WM_INITDIALOG:
+        hotkey_dialog = hwnd;
+        PostMessageW(hwnd, WM_DIALOG_LOADED, 0, 0);
+        break;
+
+    case WM_DIALOG_LOADED:
+        SetEvent(hotkey_dialog_loaded);
+        return TRUE;
+    }
+
+    return CallWindowProcW(old_hotkeys_dialog_proc, hwnd, msg, wparam, lparam);
+}
+
+
+static LRESULT CALLBACK hook_proc_hotkeys(int code, WPARAM wparam, LPARAM lparam)
+{
+    static BOOL done;
+    if (code == HCBT_CREATEWND)
+    {
+        CBT_CREATEWNDW *c = (CBT_CREATEWNDW *)lparam;
+
+        /* The first dialog created will be the parent dialog */
+        if (!done && c->lpcs->lpszClass == (LPWSTR)WC_DIALOG)
+        {
+            old_hotkeys_dialog_proc = (WNDPROC)SetWindowLongPtrW((HWND)wparam, GWLP_WNDPROC, (LONG_PTR)new_hotkeys_dialog_proc);
+            done = TRUE;
+        }
+    }
+
+    return CallNextHookEx(NULL, code, wparam, lparam);
+}
+
+static DWORD WINAPI hotkey_dialog_thread(void *arg)
+{
+    HPROPSHEETPAGE hpsp[2];
+    PROPSHEETPAGEA psp[2];
+    PROPSHEETHEADERA psh;
+    HHOOK hook;
+    int i;
+
+    /* set up a hook proc in order to subclass the main dialog early on */
+    hook = SetWindowsHookExW(WH_CBT, hook_proc_hotkeys, NULL, GetCurrentThreadId());
+
+    /* create the property sheet pages */
+    memset(psp, 0, sizeof(PROPSHEETPAGEA) * ARRAY_SIZE(psp));
+
+    for (i = 0; i < ARRAY_SIZE(psp); i++)
+    {
+        psp[i].dwSize = sizeof(PROPSHEETPAGEA);
+        psp[i].hInstance = GetModuleHandleA(NULL);
+        psp[i].pszTemplate = (LPCSTR)MAKEINTRESOURCE(IDD_PROP_PAGE_RADIO);
+        psp[i].pfnDlgProc = nav_page_proc;
+        hpsp[i] = pCreatePropertySheetPageA(&psp[i]);
+    }
+
+    /* set up the property sheet dialog */
+    memset(&psh, 0, sizeof(psh));
+    psh.dwSize = PROPSHEETHEADERA_V1_SIZE;
+    psh.dwFlags = PSH_WIZARD;
+    psh.pszCaption = "A Wizard";
+    psh.nPages = ARRAY_SIZE(psp);
+    psh.hwndParent = GetDesktopWindow();
+    psh.phpage = hpsp;
+    pPropertySheetA(&psh);
+
+    UnhookWindowsHookEx(hook);
+
+    return 0;
+}
+
+static void test_hotkey_navigation(void)
+{
+    HWND control;
+    HANDLE thread;
+    int i;
+    INPUT inputs[6] = {0};
+
+    for (i = 0; i < ARRAY_SIZE(inputs); i++)
+    {
+        inputs[i].type = INPUT_KEYBOARD;
+    }
+
+    active_page_changed = CreateEventW(NULL, FALSE, FALSE, NULL);
+    ok(active_page_changed != NULL, "CreateEvent failed, error %lu\n", GetLastError());
+    hotkey_dialog_loaded = CreateEventW(NULL, FALSE, FALSE, NULL);
+    ok(hotkey_dialog_loaded != NULL, "CreateEvent failed, error %lu\n", GetLastError());
+
+    thread = CreateThread(NULL, 0, hotkey_dialog_thread, NULL, 0, NULL);
+
+    WaitForSingleObject(hotkey_dialog_loaded, INFINITE);
+    ResetEvent(active_page_changed);
+    SetFocus(hotkey_dialog);
+
+    control = (HWND)SendMessageA(hotkey_dialog, PSM_GETCURRENTPAGEHWND, 0, 0);
+    active_page = (LONG)SendMessageA(hotkey_dialog, PSM_HWNDTOINDEX, (WPARAM)control, 0);
+    ok(active_page == 0, "Got %ld\n", active_page);
+
+    inputs[0].ki.wVk = VK_CONTROL;
+    inputs[0].ki.dwFlags = 0;
+    inputs[1].ki.wVk = VK_NEXT;
+    inputs[1].ki.dwFlags = 0;
+    inputs[2].ki.wVk = VK_NEXT;
+    inputs[2].ki.dwFlags = KEYEVENTF_KEYUP;
+    inputs[3].ki.wVk = VK_CONTROL;
+    inputs[3].ki.dwFlags = KEYEVENTF_KEYUP;
+    SendInput(4, inputs, sizeof(INPUT));
+    WaitForSingleObject(active_page_changed, 1000);
+    todo_wine
+    ok(active_page == 1, "Got %ld\n", active_page);
+
+    inputs[0].ki.wVk = VK_CONTROL;
+    inputs[0].ki.dwFlags = 0;
+    inputs[1].ki.wVk = VK_PRIOR;
+    inputs[1].ki.dwFlags = 0;
+    inputs[2].ki.wVk = VK_PRIOR;
+    inputs[2].ki.dwFlags = KEYEVENTF_KEYUP;
+    inputs[3].ki.wVk = VK_CONTROL;
+    inputs[3].ki.dwFlags = KEYEVENTF_KEYUP;
+    SendInput(4, inputs, sizeof(INPUT));
+    WaitForSingleObject(active_page_changed, 1000);
+    ok(active_page == 0, "Got %ld\n", active_page);
+
+    inputs[0].ki.wVk = VK_CONTROL;
+    inputs[0].ki.dwFlags = 0;
+    inputs[1].ki.wVk = VK_TAB;
+    inputs[1].ki.dwFlags = 0;
+    inputs[2].ki.wVk = VK_TAB;
+    inputs[2].ki.dwFlags = KEYEVENTF_KEYUP;
+    inputs[3].ki.wVk = VK_CONTROL;
+    inputs[3].ki.dwFlags = KEYEVENTF_KEYUP;
+    SendInput(4, inputs, sizeof(INPUT));
+    WaitForSingleObject(active_page_changed, 1000);
+    todo_wine
+    ok(active_page == 1, "Got %ld\n", active_page);
+
+    inputs[0].ki.wVk = VK_CONTROL;
+    inputs[0].ki.dwFlags = 0;
+    inputs[1].ki.wVk = VK_SHIFT;
+    inputs[1].ki.dwFlags = 0;
+    inputs[2].ki.wVk = VK_TAB;
+    inputs[2].ki.dwFlags = 0;
+    inputs[3].ki.wVk = VK_TAB;
+    inputs[3].ki.dwFlags = KEYEVENTF_KEYUP;
+    inputs[4].ki.wVk = VK_SHIFT;
+    inputs[4].ki.dwFlags = KEYEVENTF_KEYUP;
+    inputs[5].ki.wVk = VK_CONTROL;
+    inputs[5].ki.dwFlags = KEYEVENTF_KEYUP;
+    SendInput(6, inputs, sizeof(INPUT));
+    WaitForSingleObject(active_page_changed, 1000);
+    ok(active_page == 0, "Got %ld\n", active_page);
+
+    PostMessageA(hotkey_dialog, WM_CLOSE, 0, 0);
+    WaitForSingleObject(thread, INFINITE);
+    CloseHandle(thread);
+    CloseHandle(hotkey_dialog_loaded);
+    CloseHandle(active_page_changed);
+    active_page_changed = NULL;
+}
+
 static void init_comctl32_functions(void)
 {
     HMODULE hComCtl32 = LoadLibraryA("comctl32.dll");
@@ -1442,6 +1615,7 @@ START_TEST(propsheet)
     test_CreatePropertySheetPage();
     test_page_dialog_texture();
     test_invalid_hpropsheetpage();
+    test_hotkey_navigation();

if (!load_v6_module(&ctx_cookie, &ctx))
         return;

[PATCH v7 2/2] comclt32: Allow hotkeys for propsheet navigation.

[Fabian Maurer]

From: Fabian Maurer

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56103
---
 dlls/comctl32/propsheet.c       | 108 ++++++++++++++++----------------
 dlls/comctl32/tests/propsheet.c |   2 -
 2 files changed, 54 insertions(+), 56 deletions(-)

diff --git a/dlls/comctl32/propsheet.c b/dlls/comctl32/propsheet.c
index b530a1a8094..fe8a4c68333 100644
--- a/dlls/comctl32/propsheet.c
+++ b/dlls/comctl32/propsheet.c
@@ -2884,6 +2884,59 @@ static void PROPSHEET_CleanUp(HWND hwndDlg)
   GlobalFree(psInfo);
 }

+/******************************************************************************
+ *            PROPSHEET_IsDialogMessage
+ */
+static BOOL PROPSHEET_IsDialogMessage(HWND hwnd, LPMSG lpMsg)
+{
+   PropSheetInfo* psInfo = GetPropW(hwnd, PropSheetInfoStr);
+
+   TRACE("\n");
+   if (!psInfo || (hwnd != lpMsg->hwnd && !IsChild(hwnd, lpMsg->hwnd)))
+      return FALSE;
+
+   if (lpMsg->message == WM_KEYDOWN && (GetKeyState(VK_CONTROL) & 0x8000))
+   {
+      int new_page = 0;
+      INT dlgCode = SendMessageW(lpMsg->hwnd, WM_GETDLGCODE, 0, (LPARAM)lpMsg);
+
+      if (!(dlgCode & DLGC_WANTMESSAGE))
+      {
+         switch (lpMsg->wParam)
+         {
+            case VK_TAB:
+               if (GetKeyState(VK_SHIFT) & 0x8000)
+                   new_page = -1;
+                else
+                   new_page = 1;
+               break;
+
+            case VK_NEXT:   new_page = 1;  break;
+            case VK_PRIOR:  new_page = -1; break;
+         }
+      }
+
+      if (new_page)
+      {
+         if (PROPSHEET_CanSetCurSel(hwnd) != FALSE)
+         {
+            new_page += psInfo->active_page;
+
+            if (new_page < 0)
+               new_page = psInfo->nPages - 1;
+            else if (new_page >= psInfo->nPages)
+               new_page = 0;
+
+            PROPSHEET_SetCurSel(hwnd, new_page, 1, 0);
+         }
+
+         return TRUE;
+      }
+   }
+
+   return IsDialogMessageW(hwnd, lpMsg);
+}
+
 static INT do_loop(const PropSheetInfo *psInfo)
 {
     MSG msg = { 0 };
@@ -2896,7 +2949,7 @@ static INT do_loop(const PropSheetInfo *psInfo)
         if(ret == -1)
             break;

-        if(!IsDialogMessageW(hwnd, &msg))
+        if(!PROPSHEET_IsDialogMessage(hwnd, &msg))
         {
             TranslateMessage(&msg);
             DispatchMessageW(&msg);
@@ -3234,59 +3287,6 @@ BOOL WINAPI DestroyPropertySheetPage(HPROPSHEETPAGE hpsp)
     return TRUE;
 }

-/******************************************************************************
- *            PROPSHEET_IsDialogMessage
- */
-static BOOL PROPSHEET_IsDialogMessage(HWND hwnd, LPMSG lpMsg)
-{
-   PropSheetInfo* psInfo = GetPropW(hwnd, PropSheetInfoStr);
-
-   TRACE("\n");
-   if (!psInfo || (hwnd != lpMsg->hwnd && !IsChild(hwnd, lpMsg->hwnd)))
-      return FALSE;
-
-   if (lpMsg->message == WM_KEYDOWN && (GetKeyState(VK_CONTROL) & 0x8000))
-   {
-      int new_page = 0;
-      INT dlgCode = SendMessageW(lpMsg->hwnd, WM_GETDLGCODE, 0, (LPARAM)lpMsg);
-
-      if (!(dlgCode & DLGC_WANTMESSAGE))
-      {
-         switch (lpMsg->wParam)
-         {
-            case VK_TAB:
-               if (GetKeyState(VK_SHIFT) & 0x8000)
-                   new_page = -1;
-                else
-                   new_page = 1;
-               break;
-
-            case VK_NEXT:   new_page = 1;  break;
-            case VK_PRIOR:  new_page = -1; break;
-         }
-      }
-
-      if (new_page)
-      {
-         if (PROPSHEET_CanSetCurSel(hwnd) != FALSE)
-         {
-            new_page += psInfo->active_page;
-
-            if (new_page < 0)
-               new_page = psInfo->nPages - 1;
-            else if (new_page >= psInfo->nPages)
-               new_page = 0;
-
-            PROPSHEET_SetCurSel(hwnd, new_page, 1, 0);
-         }
-
-         return TRUE;
-      }
-   }
-
-   return IsDialogMessageW(hwnd, lpMsg);
-}
-
 /******************************************************************************
  *            PROPSHEET_DoCommand
  */
diff --git a/dlls/comctl32/tests/propsheet.c b/dlls/comctl32/tests/propsheet.c
index bffae6f99bb..4ccab5685ec 100644
--- a/dlls/comctl32/tests/propsheet.c
+++ b/dlls/comctl32/tests/propsheet.c
@@ -1509,7 +1509,6 @@ static void test_hotkey_navigation(void)
     inputs[3].ki.dwFlags = KEYEVENTF_KEYUP;
     SendInput(4, inputs, sizeof(INPUT));
     WaitForSingleObject(active_page_changed, 1000);
-    todo_wine
     ok(active_page == 1, "Got %ld\n", active_page);

inputs[0].ki.wVk = VK_CONTROL;
@@ -1534,7 +1533,6 @@ static void test_hotkey_navigation(void)
     inputs[3].ki.dwFlags = KEYEVENTF_KEYUP;
     SendInput(4, inputs, sizeof(INPUT));
     WaitForSingleObject(active_page_changed, 1000);
-    todo_wine
     ok(active_page == 1, "Got %ld\n", active_page);

inputs[0].ki.wVk = VK_CONTROL;

[PATCH v7 2/2] comclt32: Allow hotkeys for propsheet navigation.

[Marvin]

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=146588

Your paranoid android.

=== debian11b (64 bit WoW report) ===

kernel32:
comm.c:1574: Test failed: AbortWaitCts hComPortEvent failed
comm.c:1586: Test failed: Unexpected time 1001, expected around 500

[Fabian Maurer (＠DarkShadow44)]

On Wed Jun 26 01:11:50 2024 +0000, Zhiyi Zhang wrote:
> If you run it on TestBots multiple times ( >=5 ) and can't reproduce the
> issue anymore, let's consider it fixed. If it still appears in future
> runs, let's add a flaky.

FWIW, I updated the timeout to be 1 second. The [Test](https://testbot.winehq.org/JobDetails.pl?Key=146588) for that didn't have issues.

I also reran the test a few times:
[Retry 1](https://testbot.winehq.org/JobDetails.pl?Key=146582)
[Retry 2](https://testbot.winehq.org/JobDetails.pl?Key=146583)
[Retry 3](https://testbot.winehq.org/JobDetails.pl?Key=146584) (failed once)
[Retry 4](https://testbot.winehq.org/JobDetails.pl?Key=146585)
[Retry 5](https://testbot.winehq.org/JobDetails.pl?Key=146586)
[Retry 6](https://testbot.winehq.org/JobDetails.pl?Key=146587)

The failure is very rare. Should I just mark it flaky, or retry a bunch of times with SetForegroundWindow?

[Zhiyi Zhang (＠zhiyi)]

On Wed Jun 26 20:50:52 2024 +0000, Fabian Maurer wrote:
> FWIW, I updated the timeout to be 1 second. The
> [Test](https://testbot.winehq.org/JobDetails.pl?Key=146588) for that
> didn't have issues.
> I also reran the test a few times:
> [Retry 1](https://testbot.winehq.org/JobDetails.pl?Key=146582)
> [Retry 2](https://testbot.winehq.org/JobDetails.pl?Key=146583)
> [Retry 3](https://testbot.winehq.org/JobDetails.pl?Key=146584) (failed once)
> [Retry 4](https://testbot.winehq.org/JobDetails.pl?Key=146585)
> [Retry 5](https://testbot.winehq.org/JobDetails.pl?Key=146586)
> [Retry 6](https://testbot.winehq.org/JobDetails.pl?Key=146587)
> The failure is very rare. Should I just mark it flaky, or retry a bunch
> of times with SetForegroundWindow?

Try SetForegroundWindow() first. If it still happens then mark it as flaky.