[PATCH v2 1/2] user32: Fix menu item rectangle calculation in GetMenuItemRect().

[Zhiyi Zhang]

This fixes the menu of BibleWorks 10 not showing up and some todo
in tests.

Sysmenu is in non-client area. And because MapWindowPoints()
should not be used with non-client area coordinates. Doing so
results in wrong coordinates for sysmenu item rectangles.

Signed-off-by: Zhiyi Zhang 
---
 dlls/user32/menu.c       | 10 +++++-
 dlls/user32/tests/menu.c | 67 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 71 insertions(+), 6 deletions(-)

diff --git a/dlls/user32/menu.c b/dlls/user32/menu.c
index ab1ae9bc1b..27e6430cc5 100644
--- a/dlls/user32/menu.c
+++ b/dlls/user32/menu.c
@@ -5306,6 +5306,7 @@ BOOL WINAPI GetMenuItemRect(HWND hwnd, HMENU hMenu, UINT uItem, RECT *rect)
 {
     POPUPMENU *menu;
     UINT pos;
+    RECT window_rect;

TRACE("(%p,%p,%d,%p)\n", hwnd, hMenu, uItem, rect);

@@ -5326,7 +5327,14 @@ BOOL WINAPI GetMenuItemRect(HWND hwnd, HMENU hMenu, UINT uItem, RECT *rect)
     OffsetRect(rect, menu->items_rect.left, menu->items_rect.top);
     release_menu_ptr(menu);

-    MapWindowPoints(hwnd, 0, (POINT *)rect, 2);
+    /* Popup menu item draws in the client area */
+    if (menu->wFlags & MF_POPUP) MapWindowPoints(hwnd, 0, (POINT *)rect, 2);
+    /* Sysmenu draws in the non-client area, can't use MapWindowPoints in non-client area */
+    else
+    {
+        GetWindowRect(hwnd, &window_rect);
+        OffsetRect(rect, window_rect.left, window_rect.top);
+    }
     return TRUE;
 }

diff --git a/dlls/user32/tests/menu.c b/dlls/user32/tests/menu.c
index 7f338ce9b1..54a5ea232a 100644
--- a/dlls/user32/tests/menu.c
+++ b/dlls/user32/tests/menu.c
@@ -341,6 +341,62 @@ static void test_getmenubarinfo(void)
     DestroyWindow(hwnd);
 }

+static void test_GetMenuItemRect(void)
+{
+    HWND hwnd;
+    HMENU hmenu;
+    HMENU popup_hmenu;
+    RECT window_rect;
+    RECT item_rect;
+    POINT client_top_left;
+    INT caption_height;
+    BOOL ret;
+
+    hwnd = CreateWindowW((LPCWSTR)MAKEINTATOM(atomMenuCheckClass), NULL, WS_OVERLAPPEDWINDOW, 0, 0, 100, 100, NULL,
+                         NULL, NULL, NULL);
+    ok(hwnd != NULL, "CreateWindow failed with error %d\n", GetLastError());
+    hmenu = CreateMenu();
+    ok(hmenu != NULL, "CreateMenu failed with error %d\n", GetLastError());
+    popup_hmenu = CreatePopupMenu();
+    ok(popup_hmenu != NULL, "CreatePopupMenu failed with error %d\n", GetLastError());
+    ret = AppendMenuA(popup_hmenu, MF_STRING, 0, "Popup");
+    ok(ret, "AppendMenu failed with error %d\n", GetLastError());
+    ret = AppendMenuA(hmenu, MF_STRING | MF_POPUP, (UINT_PTR)popup_hmenu, "Menu");
+    ok(ret, "AppendMenu failed with error %d\n", GetLastError());
+    ret = SetMenu(hwnd, hmenu);
+    ok(ret, "SetMenu failed with error %d\n", GetLastError());
+
+    /* Get the menu item rectangle of the displayed sysmenu item */
+    ret = GetMenuItemRect(hwnd, hmenu, 0, &item_rect);
+    ok(ret, "GetMenuItemRect failed with error %d\n", GetLastError());
+    GetWindowRect(hwnd, &window_rect);
+    /* Get the screen coordinate of the left top corner of the client rectangle */
+    client_top_left.x = 0;
+    client_top_left.y = 0;
+    MapWindowPoints(hwnd, 0, &client_top_left, 1);
+    caption_height = GetSystemMetrics(SM_CYFRAME) + GetSystemMetrics(SM_CYCAPTION);
+
+    ok(item_rect.left == client_top_left.x, "Expect item_rect.left %d == %d\n", item_rect.left, client_top_left.x);
+    ok(item_rect.right <= window_rect.right, "Expect item_rect.right %d <= %d\n", item_rect.right, window_rect.right);
+    /* A gap of 1 pixel is added deliberately in commit 75f9e64, so using equal operator would fail on Wine.
+     * Check that top and bottom are correct with 1 pixel margin tolerance */
+    ok(item_rect.top - (window_rect.top + caption_height) <= 1, "Expect item_rect.top %d - %d <= 1\n", item_rect.top,
+       window_rect.top + caption_height);
+    ok(item_rect.bottom - (client_top_left.y - 1) <= 1, "Expect item_rect.bottom %d - %d <= 1\n", item_rect.bottom,
+       client_top_left.y - 1);
+
+    /* Get the item rectangle of the not yet displayed popup menu item. */
+    ret = GetMenuItemRect(hwnd, popup_hmenu, 0, &item_rect);
+    ok(ret, "GetMenuItemRect failed with error %d\n", GetLastError());
+    ok(item_rect.left == client_top_left.x, "Expect item_rect.left %d == %d\n", item_rect.left, client_top_left.x);
+    ok(item_rect.right == client_top_left.x, "Expect item_rect.right %d == %d\n", item_rect.right, client_top_left.x);
+    ok(item_rect.top == client_top_left.y, "Expect item_rect.top %d == %d\n", item_rect.top, client_top_left.y);
+    ok(item_rect.bottom == client_top_left.y, "Expect item_rect.bottom %d == %d\n", item_rect.bottom,
+       client_top_left.y);
+
+    DestroyWindow(hwnd);
+}
+
 static void test_system_menu(void)
 {
     WCHAR testW[] = {'t','e','s','t',0};
@@ -2188,15 +2244,15 @@ static struct menu_mouse_tests_s {
     { INPUT_KEYBOARD, {{0}}, {VK_F10, 0}, TRUE, FALSE },
     { INPUT_KEYBOARD, {{0}}, {VK_F10, 0}, FALSE, FALSE },

-    { INPUT_MOUSE, {{1, 2}, {0}}, {0}, TRUE, TRUE }, /* test 20 */
+    { INPUT_MOUSE, {{1, 2}, {0}}, {0}, TRUE, FALSE }, /* test 20 */
     { INPUT_MOUSE, {{1, 1}, {0}}, {0}, FALSE, FALSE },
-    { INPUT_MOUSE, {{1, 0}, {0}}, {0}, TRUE, TRUE },
+    { INPUT_MOUSE, {{1, 0}, {0}}, {0}, TRUE, FALSE },
     { INPUT_MOUSE, {{1, 1}, {0}}, {0}, FALSE, FALSE },
-    { INPUT_MOUSE, {{1, 0}, {2, 2}, {0}}, {0}, TRUE, TRUE },
+    { INPUT_MOUSE, {{1, 0}, {2, 2}, {0}}, {0}, TRUE, FALSE },
     { INPUT_MOUSE, {{2, 1}, {0}}, {0}, FALSE, FALSE },
-    { INPUT_MOUSE, {{1, 0}, {2, 0}, {0}}, {0}, TRUE, TRUE },
+    { INPUT_MOUSE, {{1, 0}, {2, 0}, {0}}, {0}, TRUE, FALSE },
     { INPUT_MOUSE, {{3, 0}, {0}}, {0}, FALSE, FALSE },
-    { INPUT_MOUSE, {{1, 0}, {2, 0}, {0}}, {0}, TRUE, TRUE },
+    { INPUT_MOUSE, {{1, 0}, {2, 0}, {0}}, {0}, TRUE, FALSE },
     { INPUT_MOUSE, {{3, 1}, {0}}, {0}, TRUE, TRUE },
     { INPUT_MOUSE, {{1, 1}, {0}}, {0}, FALSE, FALSE },
     { -1 }
@@ -4199,6 +4255,7 @@ START_TEST(menu)
     test_subpopup_locked_by_menu();
     test_menu_ownerdraw();
     test_getmenubarinfo();
+    test_GetMenuItemRect();
     test_menu_bmp_and_string();
     test_menu_getmenuinfo();
     test_menu_setmenuinfo();

[Marvin]

Hi,

While running your changed tests on Windows, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?
Full results can be found at
https://testbot.winehq.org/JobDetails.pl?Key=39910

Your paranoid android.

=== w7pro64 (64 bit menu) ===
menu.c:794: Test failed: width of owner drawn menu item is wrong. Got 53 expected 24
menu.c:798: Test failed: Height of owner drawn menu item is wrong. Got 10 expected 19

[Zhiyi Zhang]

Random error, see https://test.winehq.org/data/acb879c9d2feae69e8b5b1ede28523a29aef1b89/win7_n....
Not related to this patch.

On Sat 7 14 21:38, Marvin wrote:
> Hi,
>
> While running your changed tests on Windows, I think I found new failures.
> Being a bot and all I'm not very good at pattern recognition, so I might be
> wrong, but could you please double-check?
> Full results can be found at
> https://testbot.winehq.org/JobDetails.pl?Key=39910
>
> Your paranoid android.
>
> === w7pro64 (64 bit menu) ===
> menu.c:794: Test failed: width of owner drawn menu item is wrong. Got 53 expected 24
> menu.c:798: Test failed: Height of owner drawn menu item is wrong. Got 10 expected 19

[Alexandre Julliard]

Zhiyi Zhang  writes:
> @@ -5326,7 +5327,14 @@ BOOL WINAPI GetMenuItemRect(HWND hwnd, HMENU hMenu, UINT uItem, RECT *rect)
>      OffsetRect(rect, menu->items_rect.left, menu->items_rect.top);
>      release_menu_ptr(menu);
> - MapWindowPoints(hwnd, 0, (POINT *)rect, 2);
> - /* Popup menu item draws in the client area */
> - if (menu->wFlags & MF_POPUP) MapWindowPoints(hwnd, 0, (POINT *)rect, 2);

You shouldn't access the menu pointer after it has been released.

[Zhiyi Zhang]

Thanks. I should have caught that. v3 sent.

On Mon 7 16 22:50, Alexandre Julliard wrote:
> Zhiyi Zhang  writes:
> > @@ -5326,7 +5327,14 @@ BOOL WINAPI GetMenuItemRect(HWND hwnd, HMENU hMenu, UINT uItem, RECT *rect)
> >      OffsetRect(rect, menu->items_rect.left, menu->items_rect.top);
> >      release_menu_ptr(menu);
> > - MapWindowPoints(hwnd, 0, (POINT *)rect, 2);
> > - /* Popup menu item draws in the client area */
> > - if (menu->wFlags & MF_POPUP) MapWindowPoints(hwnd, 0, (POINT *)rect, 2);
>
> You shouldn't access the menu pointer after it has been released.