[PATCH v4 0/6] winex11.drv: Fix several cursor and focus issues
Resending these here after iterating a bit in background for further discussion. I squashed a fix from Zhiyi Zhang <zzhang(a)codeweavers.com> in PATCH 3/6. Rémi Bernon (6): winex11.drv: Do not react to keyboard grab focus events winex11.drv: Only grab or warp the cursor when keyboard isn't grabbed winex11.drv: Retry last ClipCursor when grab is released winex11.drv: Do not set clipping_cursor when clip window map state changes winex11.drv: Explicitly call XUngrabPointer when clipping is released winex11.drv: Only call XWarpPointer if we can get exclusive pointer grab dlls/winex11.drv/event.c | 47 +++++++++++++++++++++++++++++++++--- dlls/winex11.drv/mouse.c | 50 +++++++++++++++++++++++++++++++++++++++ dlls/winex11.drv/x11drv.h | 2 ++ 3 files changed, 96 insertions(+), 3 deletions(-) -- 2.23.0.rc1
Several window managers are sending FocusOut with NotifyGrab mode then FocusOut with NotifyWhileGrabbed mode when a window focus is lost, as a consequence of grabbing the keyboard input before changing window focus. This is the case during alt-tab, but keyboard can also be grabbed when bringing activity view or clicking on the title bar. In this cases NotifyWhileGrabbed events aren't sent until the window really loses foreground. In the same manner, when focus is restored, they usually send FocusIn with NotifyWhileGrabbed mode followed by FocusIn with NotifyUngrab mode when the keyboard grab is released. When bringing activity view back and forth, or clicking on the title bar, only NotifyUngrab event will be sent. In order to be consistent across WM and to help simplifying focus handling, just ignore focus events related to keyboard grabs. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/event.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c index edeae7caff0..ff4cbb2157c 100644 --- a/dlls/winex11.drv/event.c +++ b/dlls/winex11.drv/event.c @@ -772,6 +772,19 @@ static BOOL X11DRV_FocusIn( HWND hwnd, XEvent *xev ) if (event->detail == NotifyPointer) return FALSE; if (hwnd == GetDesktopWindow()) return FALSE; + switch (event->mode) + { + case NotifyGrab: + WARN( "unexpected FocusIn event with NotifyGrab mode\n" ); + break; + case NotifyWhileGrabbed: + break; + case NotifyNormal: + break; + case NotifyUngrab: + return TRUE; /* ignore wm specific NotifyUngrab / NotifyGrab events w.r.t focus */ + } + if ((xic = X11DRV_get_ic( hwnd ))) XSetICFocus( xic ); if (use_take_focus) { @@ -855,6 +868,20 @@ static BOOL X11DRV_FocusOut( HWND hwnd, XEvent *xev ) return TRUE; } if (!hwnd) return FALSE; + + switch (event->mode) + { + case NotifyUngrab: + WARN( "unexpected FocusOut event with NotifyUngrab mode\n" ); + break; + case NotifyNormal: + break; + case NotifyWhileGrabbed: + break; + case NotifyGrab: + return TRUE; /* ignore wm specific NotifyUngrab / NotifyGrab events w.r.t focus */ + } + focus_out( event->display, hwnd ); return TRUE; } -- 2.23.0.rc1
Signed-off-by: Huw Davies <huw(a)codeweavers.com>
When the window manager has taken a keyboard grab, it may be going to move the window itself, so the application should not move the cursor at the same time. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/event.c | 9 +++++++++ dlls/winex11.drv/mouse.c | 12 ++++++++++++ dlls/winex11.drv/x11drv.h | 1 + 3 files changed, 22 insertions(+) diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c index ff4cbb2157c..447aa5000ca 100644 --- a/dlls/winex11.drv/event.c +++ b/dlls/winex11.drv/event.c @@ -155,6 +155,9 @@ static const char * event_names[MAX_EVENT_HANDLERS] = "SelectionNotify", "ColormapNotify", "ClientMessage", "MappingNotify", "GenericEvent" }; +/* is someone else grabbing the keyboard, for example the WM, when manipulating the window */ +BOOL keyboard_grabbed = FALSE; + int xinput2_opcode = 0; /* return the name of an X event */ @@ -778,10 +781,13 @@ static BOOL X11DRV_FocusIn( HWND hwnd, XEvent *xev ) WARN( "unexpected FocusIn event with NotifyGrab mode\n" ); break; case NotifyWhileGrabbed: + keyboard_grabbed = TRUE; break; case NotifyNormal: + keyboard_grabbed = FALSE; break; case NotifyUngrab: + keyboard_grabbed = FALSE; return TRUE; /* ignore wm specific NotifyUngrab / NotifyGrab events w.r.t focus */ } @@ -875,10 +881,13 @@ static BOOL X11DRV_FocusOut( HWND hwnd, XEvent *xev ) WARN( "unexpected FocusOut event with NotifyUngrab mode\n" ); break; case NotifyNormal: + keyboard_grabbed = FALSE; break; case NotifyWhileGrabbed: + keyboard_grabbed = TRUE; break; case NotifyGrab: + keyboard_grabbed = TRUE; return TRUE; /* ignore wm specific NotifyUngrab / NotifyGrab events w.r.t focus */ } diff --git a/dlls/winex11.drv/mouse.c b/dlls/winex11.drv/mouse.c index f737a306a56..a268cd9b341 100644 --- a/dlls/winex11.drv/mouse.c +++ b/dlls/winex11.drv/mouse.c @@ -392,6 +392,12 @@ static BOOL grab_clipping_window( const RECT *clip ) GetModuleHandleW(0), NULL ))) return TRUE; + if (keyboard_grabbed) + { + WARN( "refusing to clip to %s\n", wine_dbgstr_rect(clip) ); + return FALSE; + } + /* enable XInput2 unless we are already clipping */ if (!data->clip_hwnd) enable_xinput2(); @@ -1430,6 +1436,12 @@ BOOL CDECL X11DRV_SetCursorPos( INT x, INT y ) struct x11drv_thread_data *data = x11drv_init_thread_data(); POINT pos = virtual_screen_to_root( x, y ); + if (keyboard_grabbed) + { + WARN( "refusing to warp to %u, %u\n", pos.x, pos.y ); + return FALSE; + } + XWarpPointer( data->display, root_window, root_window, 0, 0, 0, 0, pos.x, pos.y ); data->warp_serial = NextRequest( data->display ); XNoOp( data->display ); diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index 0d3695bdcf8..8418caea0c0 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -382,6 +382,7 @@ extern Colormap default_colormap DECLSPEC_HIDDEN; extern XPixmapFormatValues **pixmap_formats DECLSPEC_HIDDEN; extern Window root_window DECLSPEC_HIDDEN; extern BOOL clipping_cursor DECLSPEC_HIDDEN; +extern BOOL keyboard_grabbed DECLSPEC_HIDDEN; extern unsigned int screen_bpp DECLSPEC_HIDDEN; extern BOOL use_xkb DECLSPEC_HIDDEN; extern BOOL usexrandr DECLSPEC_HIDDEN; -- 2.23.0.rc1
Signed-off-by: Huw Davies <huw(a)codeweavers.com>
As we ignore these NotifyGrab / NotifyUngrab w.r.t focus decisions, some applications are unaware of mouse grabs being lost and sometimes cursor clipping is lost. We have to keep the last clip rectangle and restore it when grab is released. This has been squashed with the foreground window check from Zhiyi Zhang <zzhang(a)codeweavers.com> to fix an issue that happens when switching from a fullscreen window - because there's some additional focus events involved - but in general, if the window that is getting focus cannot be activated: When FocusIn/NotifyWhileGrabbed is received, SetForegroundWindow is not called if the window cannot be activated. When the FocusIn/NotifyUngrab event arrives for the same window, we have to check the foreground window before restoring cursor clipping rectangle. For reference, the event sequence when pressing Alt-Tab - for WMs that grab the keyboard - is the following: 1. FocusOut/NotifyGrab, when WM grabs the keyboard. 2. FocusOut/NotifyWhileGrabbed, while WM switches windows, this calls SetForegroundWindow(GetDesktopWindow()). The event sequence for normal windows ends here, but for fullscreen windows, there may be these additional events: 3. FocusIn/NotifyWhileGrabbed, which may not change Wine foreground window if it cannot be activated. 4. FocusIn/NotifyUnGrab, when WM releases the keyboard while switching windows, this is ignored but it should not retry to grab the cursor, because window is not foreground. 5. FocusOut/NotifyNormal, when WM finishes switching the windows. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/event.c | 8 ++++++++ dlls/winex11.drv/mouse.c | 24 ++++++++++++++++++++++++ dlls/winex11.drv/x11drv.h | 1 + 3 files changed, 33 insertions(+) diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c index 447aa5000ca..59ab4ce4ce3 100644 --- a/dlls/winex11.drv/event.c +++ b/dlls/winex11.drv/event.c @@ -788,6 +788,7 @@ static BOOL X11DRV_FocusIn( HWND hwnd, XEvent *xev ) break; case NotifyUngrab: keyboard_grabbed = FALSE; + retry_grab_clipping_window(); return TRUE; /* ignore wm specific NotifyUngrab / NotifyGrab events w.r.t focus */ } @@ -888,6 +889,13 @@ static BOOL X11DRV_FocusOut( HWND hwnd, XEvent *xev ) break; case NotifyGrab: keyboard_grabbed = TRUE; + + /* This will do nothing due to keyboard_grabbed == TRUE, but it + * will save the current clipping rect so we can restore it on + * FocusIn with NotifyUngrab mode. + */ + retry_grab_clipping_window(); + return TRUE; /* ignore wm specific NotifyUngrab / NotifyGrab events w.r.t focus */ } diff --git a/dlls/winex11.drv/mouse.c b/dlls/winex11.drv/mouse.c index a268cd9b341..97e2935103f 100644 --- a/dlls/winex11.drv/mouse.c +++ b/dlls/winex11.drv/mouse.c @@ -125,6 +125,9 @@ XContext cursor_context = 0; static HWND cursor_window; static HCURSOR last_cursor; static DWORD last_cursor_change; +static RECT last_clip_rect; +static HWND last_clip_foreground_window; +static BOOL last_clip_refused; static RECT clip_rect; static Cursor create_cursor( HANDLE handle ); @@ -395,8 +398,15 @@ static BOOL grab_clipping_window( const RECT *clip ) if (keyboard_grabbed) { WARN( "refusing to clip to %s\n", wine_dbgstr_rect(clip) ); + last_clip_refused = TRUE; + last_clip_foreground_window = GetForegroundWindow(); + last_clip_rect = *clip; return FALSE; } + else + { + last_clip_refused = FALSE; + } /* enable XInput2 unless we are already clipping */ if (!data->clip_hwnd) enable_xinput2(); @@ -470,6 +480,20 @@ void reset_clipping_window(void) ClipCursor( NULL ); /* make sure the clip rectangle is reset too */ } +/*********************************************************************** + * retry_grab_clipping_window + * + * Restore the current clip rectangle or retry the last one if it has + * been refused because of an active keyboard grab. + */ +void retry_grab_clipping_window(void) +{ + if (clipping_cursor) + ClipCursor( &clip_rect ); + else if (last_clip_refused && GetForegroundWindow() == last_clip_foreground_window) + ClipCursor( &last_clip_rect ); +} + BOOL CDECL X11DRV_ClipCursor( const RECT *clip ); /*********************************************************************** diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index 8418caea0c0..d4e476facb2 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -624,6 +624,7 @@ extern void sync_window_cursor( Window window ) DECLSPEC_HIDDEN; extern LRESULT clip_cursor_notify( HWND hwnd, HWND new_clip_hwnd ) DECLSPEC_HIDDEN; extern void ungrab_clipping_window(void) DECLSPEC_HIDDEN; extern void reset_clipping_window(void) DECLSPEC_HIDDEN; +extern void retry_grab_clipping_window(void) DECLSPEC_HIDDEN; extern BOOL clip_fullscreen_window( HWND hwnd, BOOL reset ) DECLSPEC_HIDDEN; extern void move_resize_window( HWND hwnd, int dir ) DECLSPEC_HIDDEN; extern void X11DRV_InitKeyboard( Display *display ) DECLSPEC_HIDDEN; -- 2.23.0.rc1
Signed-off-by: Huw Davies <huw(a)codeweavers.com>
This flag should only indicate a successful call to XGrabPointer. If not then we could assume we have ownership of the pointer when we don't. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/event.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c index 59ab4ce4ce3..90f139dd0fa 100644 --- a/dlls/winex11.drv/event.c +++ b/dlls/winex11.drv/event.c @@ -986,7 +986,6 @@ static BOOL X11DRV_MapNotify( HWND hwnd, XEvent *event ) if (event->xany.window == x11drv_thread_data()->clip_window) { - clipping_cursor = TRUE; return TRUE; } if (!(data = get_win_data( hwnd ))) return FALSE; @@ -1007,8 +1006,6 @@ static BOOL X11DRV_MapNotify( HWND hwnd, XEvent *event ) */ static BOOL X11DRV_UnmapNotify( HWND hwnd, XEvent *event ) { - if (event->xany.window == x11drv_thread_data()->clip_window) - clipping_cursor = FALSE; return TRUE; } -- 2.23.0.rc1
Signed-off-by: Huw Davies <huw(a)codeweavers.com>
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/mouse.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/winex11.drv/mouse.c b/dlls/winex11.drv/mouse.c index 97e2935103f..fff6fc7047f 100644 --- a/dlls/winex11.drv/mouse.c +++ b/dlls/winex11.drv/mouse.c @@ -465,6 +465,7 @@ void ungrab_clipping_window(void) TRACE( "no longer clipping\n" ); XUnmapWindow( display, clip_window ); + if (clipping_cursor) XUngrabPointer( display, CurrentTime ); clipping_cursor = FALSE; SendMessageW( GetDesktopWindow(), WM_X11DRV_CLIP_CURSOR, 0, 0 ); } -- 2.23.0.rc1
Signed-off-by: Huw Davies <huw(a)codeweavers.com>
XWarpPointer will always succeed, regardless of grabs, so if the pointer is already grabbed, by some other application, we should not ask to warp it. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/mouse.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/dlls/winex11.drv/mouse.c b/dlls/winex11.drv/mouse.c index fff6fc7047f..8d1dc5e35d7 100644 --- a/dlls/winex11.drv/mouse.c +++ b/dlls/winex11.drv/mouse.c @@ -1467,8 +1467,21 @@ BOOL CDECL X11DRV_SetCursorPos( INT x, INT y ) return FALSE; } + if (!clipping_cursor && + XGrabPointer( data->display, root_window, False, + PointerMotionMask | ButtonPressMask | ButtonReleaseMask, + GrabModeAsync, GrabModeAsync, root_window, None, CurrentTime ) != GrabSuccess) + { + WARN( "refusing to warp pointer to %u, %u without exclusive grab\n", pos.x, pos.y ); + return FALSE; + } + XWarpPointer( data->display, root_window, root_window, 0, 0, 0, 0, pos.x, pos.y ); data->warp_serial = NextRequest( data->display ); + + if (!clipping_cursor) + XUngrabPointer( data->display, CurrentTime ); + XNoOp( data->display ); XFlush( data->display ); /* avoids bad mouse lag in games that do their own mouse warping */ TRACE( "warped to %d,%d serial %lu\n", x, y, data->warp_serial ); -- 2.23.0.rc1
Signed-off-by: Huw Davies <huw(a)codeweavers.com>
On Fri, 30 Aug 2019 at 14:49, Rémi Bernon <rbernon(a)codeweavers.com> wrote:
XWarpPointer will always succeed, regardless of grabs, so if the pointer is already grabbed, by some other application, we should not ask to warp it.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/mouse.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
This breaks the d3d8 and d3d9 test_cursor_pos() tests: device.c:1122: Test failed: Didn't receive MOUSEMOVE 5 (125, 125).
On 9/9/19 2:25 PM, Henri Verbeet wrote:
On Fri, 30 Aug 2019 at 14:49, Rémi Bernon <rbernon(a)codeweavers.com> wrote:
XWarpPointer will always succeed, regardless of grabs, so if the pointer is already grabbed, by some other application, we should not ask to warp it.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/mouse.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
This breaks the d3d8 and d3d9 test_cursor_pos() tests:
device.c:1122: Test failed: Didn't receive MOUSEMOVE 5 (125, 125).
I guess it could be the MotionNotify event caused by XWarpPointer that may not be received anymore, as the cursor grab is called on the root window, but I believe they were supposed to be ignored anyway (see warp_serial and is_old_motion_event function). Also, when running these tests on the testbot, I don't see any failure (I commented out the other tests, but that should be ok, right?): * https://testbot.winehq.org/JobDetails.pl?Key=56365 * https://testbot.winehq.org/JobDetails.pl?Key=56366 Same thing when running the tests locally. Tested in Xephyr with openbox, and with my session's X server with mutter. Could you give me more details about how you got the failure? Cheers, -- Rémi Bernon <rbernon(a)codeweavers.com>
participants (3)
-
Henri Verbeet -
Huw Davies -
Rémi Bernon