xwm: Fix wrong input offset for certain clients

Submitted by Scott Moreau on March 18, 2018, 6:22 p.m.

Details

Message ID 1521397337-19298-3-git-send-email-oreaus@gmail.com
State New
Headers show
Series "xwm: Handle changing override redirect flag" ( rev: 2 ) in Wayland

Not browsing as part of any series.

Commit Message

Scott Moreau March 18, 2018, 6:22 p.m.
Some windows might get a create_notify event without the
override redirect flag set and then get a confiure_notify
event before map_request is received. This means that when
weston_wm_window_get_child_position is called in response
to configure_notify, the wrong offsets are computed resulting
in wrong input offsets for some clients like steam.
---
 xwayland/window-manager.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
index c307e19..2307f1a 100644
--- a/xwayland/window-manager.c
+++ b/xwayland/window-manager.c
@@ -711,10 +711,12 @@  weston_wm_handle_configure_request(struct weston_wm *wm, xcb_generic_event_t *ev
 	if (configure_request->value_mask & XCB_CONFIG_WINDOW_HEIGHT)
 		window->height = configure_request->height;
 
-	if (window->frame)
+	if (window->frame) {
 		frame_resize_inside(window->frame, window->width, window->height);
+		weston_wm_window_get_child_position(window, &x, &y);
+	} else
+		x = y = 0;
 
-	weston_wm_window_get_child_position(window, &x, &y);
 	values[i++] = x;
 	values[i++] = y;
 	values[i++] = window->width;

Comments

On Sun, 18 Mar 2018 12:22:17 -0600
Scott Moreau <oreaus@gmail.com> wrote:

> Some windows might get a create_notify event without the
> override redirect flag set and then get a confiure_notify
> event before map_request is received. This means that when
> weston_wm_window_get_child_position is called in response
> to configure_notify, the wrong offsets are computed resulting
> in wrong input offsets for some clients like steam.

Hi,

do I understand correctly that the window is never set as
override-redirect?

> ---
>  xwayland/window-manager.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
> index c307e19..2307f1a 100644
> --- a/xwayland/window-manager.c
> +++ b/xwayland/window-manager.c
> @@ -711,10 +711,12 @@ weston_wm_handle_configure_request(struct weston_wm *wm, xcb_generic_event_t *ev
>  	if (configure_request->value_mask & XCB_CONFIG_WINDOW_HEIGHT)
>  		window->height = configure_request->height;
>  
> -	if (window->frame)
> +	if (window->frame) {
>  		frame_resize_inside(window->frame, window->width, window->height);
> +		weston_wm_window_get_child_position(window, &x, &y);
> +	} else
> +		x = y = 0;

The else branch should have braces as well because the other branch
does.

>  
> -	weston_wm_window_get_child_position(window, &x, &y);
>  	values[i++] = x;
>  	values[i++] = y;
>  	values[i++] = window->width;

For a not yet mapped X11 window, this change looks correct. If there is
no 'window->frame', then the app window has not been reparented either,
which means there is no offset we could apply. In that case, the x, y
will be relative to the root window. We can set them to any number,
e.g. zero, because the window has not been mapped yet. Only when we get
MapRequest and content, the Wayland window manager may actually pick a
position and then we move the X11 window stack to its appropriate
coordinates.

However, if we get a ConfigureRequest for a window that has already
been mapped, and if that window happens to not have a frame, would this
not smash the window to the top-left corner of the screen?

That raises an interesting question: can we have a window that does not
have a frame and also is not override-redirect (O-R means we never see
the ConfigureRequest)?

If XWM sees the MapRequest, it practically always creates the frame. So
the only way to hit that corner case is, if the X11 client creates the
window as a normal window, changes it to O-R, maps it, changes it to
non-O-R, and then attempts to move. Maybe we can live with that, seems
like a pretty deep hole the X11 client is digging?

But if we have your patch to handle O-R state changes, then I suppose
we wouldn't have this corner-case at all, because it would create the
frame, right?

I have to say I don't quite understand how the wrong offsets this patch
fixes can confuse an X11 client's input. Nevertheless, I think I can
give:

Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>

unless something I said here is not true.


I still wonder... the Wayland window manager picks the window position
when it maps it, so could this patch be just a bandaid for a very
specific case, and the real bug is actually somewhere in the Wayland
window manager side for forgetting to call ->send_position() in this
specific case. If that's true, then sending the zeroes like this patch
does is just accidentally correct for the app you tested with.


Thanks,
pq