Re: [PATCH v3 1/2] oleacc: Check proper UI thread for window focus.

[Piotr Caban]

Hi Connor,

On 9/7/21 9:04 PM, Connor McAdams wrote:
> On Sat, Aug 28, 2021 at 03:32:59PM +0200, Piotr Caban wrote:
> > On 8/12/21 9:07 PM, Connor McAdams wrote:
> > > - info.cbSize = sizeof(info);
> > > - if(GetGUIThreadInfo(GetWindowThreadProcessId(This->hwnd, NULL), &info) &&
> > > -
> > >        info.hwndFocus == This->hwnd)
> >
> > It doesn't really matter but you can just call GetGUIThreadInfo(0, &info)
> > here. If This->hwnd thread is not foreground the hwnd comparison will always
> > fail.
>
> Sorry for replying late, I was out last week.
>
> Just tested out doing `GetGUIThreadInfo(0, &info)` and it doesn't work
> properly unless the window is visible, i.e ShowWindow(hwnd, TRUE) is
> called before GetGUIThreadInfo(0, &info).

It looks like a bug in wine. I've done some quick tests and 
GetForegroundWindow is also returning wrong window in invisible window test.

Here's the test I've tried:
GUITHREADINFO info;
HWND win, hwnd;

win = CreateWindow(L"button", L"button", WS_POPUP, 0, 0, 100, 100, 0, 0, 
0, 0);
hwnd = SetFocus(win);
ok(hwnd == win, "SetFocus\n");
hwnd = GetForegroundWindow();
ok(hwnd == win, "GetForegroundWindow\n");

memset(&info, 0, sizeof(info));
info.cbSize = sizeof(info);
GetGUIThreadInfo(0, &info);
ok(info.hwndFocus == win, "info.hwndFocus\n");

memset(&info, 0, sizeof(info));
info.cbSize = sizeof(info);
GetGUIThreadInfo(GetWindowThreadProcessId(win, NULL), &info);
ok(info.hwndFocus == win, "info.hwndFocus\n");

Anyway, while it's better to fix GetGUIThreadInfo, I guess it's OK to 
workaround it here. It would be good to add a comment though (especially 
if changing it to the call I have suggested doesn't cause any test 
failures).
> I also did some more testing on the weird `EVENT_SYSTEM_FOCUSABLE`
> behavior, and it seems like our current behavior matches Windows, in
> that any accessible object representing an HWND is always considered
> `focusable` so long as it isn't disabled, which contradicts the
> documentation.

I've done some tests and it looks like in some cases disabled windows 
may also be focusable. It's not related to this patch so you can leave 
it as is. I don't see any easy patterns in my limited testing.
> I can send a newer version of the patch with tests for these, if you
> think that's necessary. But, I think the current patches work
> functionally as is, unless there are some stylistic errors.

You will probably need to resend the patch since it's already fall of 
the list (after being marked as superseded).

Thanks,
Piotr