Created attachment 271542 [details] [review]
wayland: Set/unset wayland focus on mutter grab/ungrab operations

This ensures that we send the proper leave and enter events to wayland
clients.

Particularly, this solves a bug in SSD xwayland windows where clicking
and dragging on the title bar to move the window only works on the odd
turn (unless the pointer moves away from the title bar between
tries). This happens because xwayland gets a button press but doesn't
see the release so when it gets the next button press it discards it
because its pointer button tracking logic says that the button is
already pressed. Sending the proper wayland pointer leave event fixes
it since wayland clients must forget about button state at that point.

Review of attachment 271542 [details] [review]:

::: src/core/display.c
@@ +1751,3 @@
+{
+  MetaWaylandCompositor *compositor = meta_wayland_compositor_get_default ();
+

I feel like the code to set it to NULL if we have a grab should be in here.

  MetaWindow *focus_window;

  if (display->grab_op == META_GRAB_OP_NONE)
    focus_window = NULL;
  else if (meta_display_xwindow_is_a_no_focus_window (display, display->focus_xwindow))
    focus_window = NULL;
  else if (...)
    focus_window = display->focus_window;

  meta_wayland_compositor_set_input_focus (compositor, display->focus_window);

But that doesn't help us for pointer focus. I'm undecided about that. I really don't like how it's split across multiple places. Perhaps we should hack in some code to repick() to make it focus NULL if we have a non-NULL grab op, since that's what we do here.

@@ +1754,3 @@
+  if (meta_display_xwindow_is_a_no_focus_window (display, display->focus_xwindow))
+    meta_wayland_compositor_set_input_focus (compositor, NULL);
+  else if (display->focus_window && display->focus_window->surface)

Theoretically, if we always only call this when we're a Wayland compositor, this should *never* be false, but this is a separate cleanup.

Wait a minute, is this for keyboard or pointer? The keyboard focus must not move when we grab, otherwise entering and exiting the overview would unfocus the active window or focus a different one.

Uh, why would sending leave events and setting the Wayland keyboard focus to NULL change how the overview behaves?

Because if go the overview in x11 the app is not unfocused (does not go to backdrop state), and because if we don't have a focused window we call focus_default_window () on exit which is not necessary right.
Setting the pointer focus is fine, setting keyboard is not, just like we don't call XSetInputFocus under X11

We don't modify display->focus_window which is what gnome-shell tracks, only the Wayland focus (which is what's used to deliver enter/leave and that's it).

Decorations being focused is driven by the appears-focused property, and that's driven by display->focus_window as well.

Well, it's also used to deliver key events, but that obviously doesn't have any role when grabbed/modal in the overview.

No decorations being focused is driven by the app getting wl_keyboard.enter/leave, because decorations are client side.

(In reply to comment #8)
> No decorations being focused is driven by the app getting
> wl_keyboard.enter/leave, because decorations are client side.

For gtk+ this is driven by xdg_surface.activated/deactivated, not by wl_keyboard.enter/leave.

Created attachment 272066 [details] [review]
wayland-pointer: Drop unused arg from focus grab interface method

Created attachment 272067 [details] [review]
wayland: Set/unset wayland focus on mutter grab/ungrab operations

This ensures that we send the proper leave and enter events to wayland
clients.

Particularly, this solves a bug in SSD xwayland windows where clicking
and dragging on the title bar to move the window only works on the odd
turn (unless the pointer moves away from the title bar between
tries). This happens because xwayland gets a button press but doesn't
see the release so when it gets the next button press it discards it
because its pointer button tracking logic says that the button is
already pressed. Sending the proper wayland pointer leave event fixes
it since wayland clients must forget about button state at that point.

Created attachment 272069 [details] [review]
wayland-pointer: Drop unused arg from focus grab interface method

Review of attachment 272066 [details] [review]:

OK.

Review of attachment 272067 [details] [review]:

Hm, could we somehow get a repick while grabbed? I'm a bit worried about a situation like that.

Review of attachment 272069 [details] [review]:

Uh, OK.

Created attachment 272089 [details] [review]
wayland: Set/unset wayland focus on mutter grab/ungrab operations

--

The previous patch didn't actually work right. Going through
default_grab_focus() doesn't work here because that function is a nop
if button_count > 0. Instead I changed it back to calling
meta_wayland_pointer_set_focus(pointer, NULL) directly. This means we don't
update pointer->current in this case but it shouldn't matter since we
do this when beggining a GRAB_OP and thus there won't be pointer events being
delivered to wayland clients until the GRAP_OP ends and at that point
we do a seat_repick() which takes care of updating pointer->current.

Created attachment 272090 [details] [review]
wayland-seat: Don't send pointer enter/leave events during a GRAB_OP

meta_wayland_seat_repick() can be called in various cases while mutter
has a GRAB_OP ongoing which means we could be sending wrong pointer
enter/leave events.
--

(In reply to comment #14)
> Hm, could we somehow get a repick while grabbed? I'm a bit worried
> about a situation like that.

Indeed it looks like it could happen. How about this?

Comment on attachment 272069 [details] [review]
wayland-pointer: Drop unused arg from focus grab interface method

Attachment 272069 [details] pushed as 62e45b6 - wayland-pointer: Drop unused arg from focus grab interface method

Review of attachment 272090 [details] [review]:

Similar to how we did sync_input_focus, is there anything wrong with:

    if (display->grab_op != META_GRAB_OP_NONE)
      surface = NULL;
    else
      {
        if (for_event)
          ...
      }

?

Mh... one thing that pops into my mind now... What about gtk+ grab ops (the CLICKING_* stuff)?
For those, we need to send events to wayland, so that they're forwarded to XWayland and from there to gtk+.
Or we need to forge events to gtk+ from clutter events...

Created attachment 272199 [details] [review]
wayland-seat: Don't send pointer enter/leave events during a GRAB_OP

--
(In reply to comment #19)

> Similar to how we did sync_input_focus, is there anything wrong
> with:

Like this? If sync_input_focus() is being called from all the correct
places this should be nop but yeah, seems safer.

Review of attachment 272199 [details] [review]:

::: src/wayland/meta-wayland-pointer.c
@@ +587,3 @@
+void
+meta_wayland_pointer_update_current_focus (MetaWaylandPointer *pointer,
+                                           MetaWaylandSurface *surface)

I don't need the need for the separate function anymore, but it can't hurt. We probably should be moving stuff like repick to MetaWaylandPointer anyway.

Review of attachment 272089 [details] [review]:

Looks good.

Created attachment 272201 [details] [review]
wayland: Exempt CLICKING grab ops when syncing wayland input focus

If we have a CLICKING grab op we still need to send events to xwayland
so that we get them back for gtk+ to process thus we can't steer
wayland input focus away from it.
--

(In reply to comment #20)
> Mh... one thing that pops into my mind now... What about gtk+ grab
> ops (the CLICKING_* stuff)?  For those, we need to send events to
> wayland, so that they're forwarded to XWayland and from there to
> gtk+.  Or we need to forge events to gtk+ from clutter events...

Indeed, good catch. This patch fixes it but this stuff is getting
ugly. Also, I don't like the function name here but I can't think of
anything short that conveys what this is about...

Review of attachment 272201 [details] [review]:

OK, we have a bunch of different "types" of grab operations:

  * "Normal" mode. Pointer and keyboard get sent to clients. Keybindings are OK.

  * User grab operations where we basically want to change the behavior of pointer and keyboard. These are the standard move / resize grabs. These should take over the normal behavior of pointer / keyboard, as far as I'm aware. We don't play events to clients, and keybindings don't activate. The keybindings.c explicitly special-cases this, to add support for the "Escape" key when doing a mouse move/resize, and to add arrow keys when doing a keyboard move/resize.

  * Frame button operations. These are used as state tracking, so we know what button we clicked on, since we can't use a widget system like GtkButton. Under X11, at least, normal events and keybindings activate normally (e.g. click and hold on the close button and press Alt+F10, and the window will be maximized). I don't think it particularly matters whether we keep this behavior. I also think that we don't particularly need this to be part of a grab op. We could track this in a separate field. At the very least, we should probably have META_GRAB_OP_CLICKING_FRAME_CONTROL and then have a separate field.

  * Compositor grab. Keybindings should act like "normal mode", and we shouldn't filter out events from the compositor, but we should not replay them to clients. We take a keyboard/pointer grab to ensure this under X11, and simply test against GRAB_OP_NONE under Wayland/XWayland.

  * Wayland pointer grab. This was a new addition from me, and refers to the type of grab that we use for Wayland popups. The only rule is that all Wayland events must go to the grabbed client, in owner-events mode. As far as I'm aware, we're allowed to make our own rules about keybindings activating and other behaviors.

So, we have a number of different policies based on the current grab op:

  Q: Should we "react" to mouse operations? e.g. if we click on a window while we have a grab, should we raise and focus it? Same for enter/leave events in a mouse focus mode.
  A: Right now, we react in all modes but a compositor grab and a Wayland popup grab. This is currently handled with the grab_op_should_block_mouse_events function in display.c, along with code that checks it in meta_display_handle_event.

  Q: Should we send events to the compositor?
  A: Right now the policy is a bit complicated. We always route events to the compositor if we have a compositor grab, of course, but not if we have a Wayland popup grab, and if we handle something inside mutter, like a mouse op / keybinding, we also skip the compositor. This is implemented with the bypass_clutter variable in meta_display_handle_event.

  Q: Should we send events to clients? Either Wayland or XWayland?
  A: If somebody (either an X client or a Wayland client) has a grab, always deliver events to them. "Yes" in normal mode as well. "No" in move/resize, though some may disagree. "No" in compositor grab mode. Frame control clicking is a bit complicated, since we have to route through XWayland for them. This is implemented with the bypass_wayland variable in meta_display_handle_event.

  Q: Should keybindings be allowed?
  A: I'm not sure what the policy should be here. Obviously in normal mode or compositor grab mode, we should always activate keybindings, but what about resizing / frame controls / Wayland grabs? I think we have to decide our own policy here. This is implemented with a variety of fancy custom code in keybindings.c, along with Florian's filter_keybinding plugin vfunc, although that's somewhat orthogonal.

Ideally, this would all be managed centrally in a place like display.c, instead of split between compositor.c and meta-wayland-surface.c, but I understand that we have various silly architectural issues like "core/ isn't supposed to know that Clutter exists at all, and has no idea what a ClutterStage is".

There's also lots of code that checks meta_op_is_moving to implement that whole state machine. Again, ideally, grab_op would simply be used to route events instead of also drive an state machine about moving/resizing.

So, instead of meta_grab_op_is_wayland / meta_grab_op_is_clicking, I'd prefer to see something based in terms of these policies. Something along the lines of grab_op_should_block_mouse_events (which is also a bad name for what it does, I admit).

Pushing for now as discussed on IRC. Jasper has a WIP patch to address
comment 25.

Attachment 272089 [details] pushed as 1b29113 - wayland: Set/unset wayland focus on mutter grab/ungrab operations
Attachment 272199 [details] pushed as 8968501 - wayland-seat: Don't send pointer enter/leave events during a GRAB_OP
Attachment 272201 [details] pushed as 76dc0ca - wayland: Exempt CLICKING grab ops when syncing wayland input focus