GNOME Bugzilla – Bug 755164
Pointer is not picked properly after the window below it changes
Last modified: 2020-04-03 20:08:21 UTC
Right now all operations that change the window below the pointer will yield the wrong results (alt-tab, workspace changes, windows being (un)maximized...), the wl_pointer.enter/leave events will be inconsistent, and the pointer surface will also be wrong as a result. In bug #755162 I attached a Clutter patch to update the input devices after relayouts, this results in the actor under the pointer(s) to be updated, and crossing/motion events to be synthesized. That fixes the most part of it, although right now mutter ignores any pointer events not coming from hw devices, I'm attaching a patch that will make mutter pick the current surface again on those synthesized events.
Created attachment 311558 [details] [review] wayland: Ensure we repick the pointer on synthesized motion events Relayouts in clutter may trigger synthesized motion events if the actor below the pointer changes. In that situation we do need to repick() the MetaWaylandPointer to end up with the right current wayland surface.
Review of attachment 311558 [details] [review]: ::: src/wayland/meta-wayland-seat.c @@ +302,3 @@ + meta_wayland_pointer_repick (&seat->pointer); + return; + } I fail to see why we need CLUTTER_MOTION here. When testing, if the actor under the pointer changed so that the cursor ended up on another actor, I got the ENTER/LEAVE events. And why should we only deal with LEAVE/ENTER events if they are not from a supported hardware device? Testing shows that at least when running on KMS, if we always check CLUTTER_ENTER/LEAVE no matter what "event_from_supported_hardware_device()" returns, and just do the pointer repick if the device type is either a touchpad or pointer, we'd get correct focus messages, not only for stage changes (where _supported_hardware_device() returned FALSE), but also for VT switching (where _suppported_hardware_device() returned TRUE). A nitpick would also be to use a switch statement instead of an if, for consistency.
Created attachment 339697 [details] [review] clutter: Update pointer position on master clock's update stage Ensure the pointer position is up-to-date after the stage got all actors relayout.
Created attachment 339698 [details] [review] wayland: Ensure we repick the pointer on synthesized crossing events Relayouts in clutter may trigger synthesized crossing events if the actor below the pointer changes. In that situation we do need to repick() the MetaWaylandPointer to end up with the right current wayland surface.
Review of attachment 339697 [details] [review]: ::: clutter/clutter/clutter-stage.c @@ +1189,3 @@ #endif /* CLUTTER_ENABLE_DEBUG */ + _clutter_device_manager_update_devices (clutter_device_manager_get_default ()); Did you do any performance regression checking regarding this? As far as I can see, this will be called each frame, and each time it'll "pick" the actor, meaning traversing the stage -> drawing (simple) opengl -> glReadPixels(). It sounds dangerous to me to do it too often.
Review of attachment 339698 [details] [review]: This looks good to me.
Created attachment 346277 [details] [review] clutter: Update pointer position on master clock's update stage Ensure the pointer position is up-to-date after the stage got all actors relayout.
This new patch sets a flag so we only update the pointer after any actual relayouts. I guess we could additionally check if the pointer devices are inside the redrawn areas, but this already cuts most pointer picking going on.
Review of attachment 346277 [details] [review]: I agree with Jonas and I'm not sure doing this only if there is a relayout on the current frame is much better. OTOH, we already do the whole picking dance on every input event, in particular on pointer motion events, which *is* noticeable on cpu usage profiles (with e.g. sysprof).
There is really not much option around pointer picking with GL, and if we want to keep the actor under the pointer up-to-date, I don't see much way around doing this in other place than here. I'll redo the patch so it checks the clip area first, but I don't think we can cut this down to "when it's absolutely and essentially needed".
Created attachment 346966 [details] [review] clutter: Update pointer position on master clock's update stage Ensure the pointer position is up-to-date for the pointers inside the clip area after the stage got actors relayout.
Created attachment 346967 [details] [review] switcherPopup: Ignore implicit enter events when the popup is mapped If the popup happens to be mapped beneath the pointer, mutter will now emit an implicit enter notify event (i.e. not caused by pointer motion). In this case the switcherPopup still goes and selects the item, which results in too sensitive alt-tab menus if the pointer happens to be in the wrong place.
Review of attachment 346966 [details] [review]: ::: clutter/clutter/clutter-stage.c @@ +1165,3 @@ + CLUTTER_INPUT_MODE_MASTER || + clutter_input_device_get_device_type (dev) == + CLUTTER_KEYBOARD_DEVICE) What about touch, which doesn't have a single coordinate? There are a bunch of other devices I suppose also don't have a "coordinate" as such.
Review of attachment 346967 [details] [review]: I suppose that, technically the patch that introduces this is a API change, but besides that, this change looks good.
(In reply to Jonas Ådahl from comment #13) > Review of attachment 346966 [details] [review] [review]: > > ::: clutter/clutter/clutter-stage.c > @@ +1165,3 @@ > + CLUTTER_INPUT_MODE_MASTER || > + clutter_input_device_get_device_type (dev) == > + CLUTTER_KEYBOARD_DEVICE) > > What about touch, which doesn't have a single coordinate? There are a bunch > of other devices I suppose also don't have a "coordinate" as such. Touch has no (or very finicky) crossing semantics, ongoing touches just go to the implicitly grabbed client that first received the corresponding CLUTTER_TOUCH_BEGIN. I guess we could eventually add crossing event types for touches, and we ought to support these here too, but right now we can't express per-touchpoint crossings. And other devices like drawing tablets do get a master device of their own each, so this check should support these. In practice, it's less important for these devices because if the cursor is actually showing you're operating with the tablet, and in that case you'll receive a flurry of motion events anyway.
(In reply to Carlos Garnacho from comment #15) > (In reply to Jonas Ådahl from comment #13) > > Review of attachment 346966 [details] [review] [review] [review]: > > > > ::: clutter/clutter/clutter-stage.c > > @@ +1165,3 @@ > > + CLUTTER_INPUT_MODE_MASTER || > > + clutter_input_device_get_device_type (dev) == > > + CLUTTER_KEYBOARD_DEVICE) > > > > What about touch, which doesn't have a single coordinate? There are a bunch > > of other devices I suppose also don't have a "coordinate" as such. > > Touch has no (or very finicky) crossing semantics, ongoing touches just go > to the implicitly grabbed client that first received the corresponding > CLUTTER_TOUCH_BEGIN. > What I meant is that you check for non-keyboard, and then assume it'll have a single coordinate that makes sense. Shouldn't you just check for the ones that might have? I.e. touchpad and pointer, and for tool or whatever it is that has a pointer for a tablet. For touch, I suppose we only care about "leave" here, as new touch focus should require a new touch-down (IIRC).
(In reply to Jonas Ådahl from comment #16) > (In reply to Carlos Garnacho from comment #15) > > (In reply to Jonas Ådahl from comment #13) > > > Review of attachment 346966 [details] [review] [review] [review] [review]: > > > > > > ::: clutter/clutter/clutter-stage.c > > > @@ +1165,3 @@ > > > + CLUTTER_INPUT_MODE_MASTER || > > > + clutter_input_device_get_device_type (dev) == > > > + CLUTTER_KEYBOARD_DEVICE) > > > > > > What about touch, which doesn't have a single coordinate? There are a bunch > > > of other devices I suppose also don't have a "coordinate" as such. > > > > Touch has no (or very finicky) crossing semantics, ongoing touches just go > > to the implicitly grabbed client that first received the corresponding > > CLUTTER_TOUCH_BEGIN. > > > > What I meant is that you check for non-keyboard, and then assume it'll have > a single coordinate that makes sense. Shouldn't you just check for the ones > that might have? I.e. touchpad and pointer, and for tool or whatever it is > that has a pointer for a tablet. It looks up non-keyboard master devices. As touch events use the core pointer as the master device right away (which is arguably wrong), there's only the mouse-driven pointer and per-tablet master pointers to care about. In the future, I think it'd make sense to change this master/slave device distinction to devices and "cursors" (or "sprites" in X terminology). I guess the model we adopt should still remain compatible with the "everything drives the Virtual Core Pointer" in X11. > > For touch, I suppose we only care about "leave" here, as new touch focus > should require a new touch-down (IIRC). I'm unclear here. The protocol patch refining grab semantics is precisely just for grabs, we eg. don't break implicit grabs on pointers because of a hierarchy change, IMHO it's more consistent to do the same on touch.
(In reply to Carlos Garnacho from comment #17) > (In reply to Jonas Ådahl from comment #16) > > (In reply to Carlos Garnacho from comment #15) > > > (In reply to Jonas Ådahl from comment #13) > > > > Review of attachment 346966 [details] [review] [review] [review] [review] [review]: > > > > > > > > ::: clutter/clutter/clutter-stage.c > > > > @@ +1165,3 @@ > > > > + CLUTTER_INPUT_MODE_MASTER || > > > > + clutter_input_device_get_device_type (dev) == > > > > + CLUTTER_KEYBOARD_DEVICE) > > > > > > > > What about touch, which doesn't have a single coordinate? There are a bunch > > > > of other devices I suppose also don't have a "coordinate" as such. > > > > > > Touch has no (or very finicky) crossing semantics, ongoing touches just go > > > to the implicitly grabbed client that first received the corresponding > > > CLUTTER_TOUCH_BEGIN. > > > > > > > What I meant is that you check for non-keyboard, and then assume it'll have > > a single coordinate that makes sense. Shouldn't you just check for the ones > > that might have? I.e. touchpad and pointer, and for tool or whatever it is > > that has a pointer for a tablet. > > It looks up non-keyboard master devices. As touch events use the core > pointer as the master device right away (which is arguably wrong), there's > only the mouse-driven pointer and per-tablet master pointers to care about. > > In the future, I think it'd make sense to change this master/slave device > distinction to devices and "cursors" (or "sprites" in X terminology). I > guess the model we adopt should still remain compatible with the "everything > drives the Virtual Core Pointer" in X11. I forgot to say, I can maybe check the related slave devices match what we'd expect for the master pointer, but it feels a less direct way to get to the few selected devices.
Review of attachment 346966 [details] [review]: looks good to me, I think we should go ahead with it for now ::: clutter/clutter/clutter-stage.c @@ +1238,3 @@ + { + _clutter_input_device_update (pointers->data, NULL, TRUE); + pointers = g_slist_remove_link (pointers, pointers); I believe you want delete_link() here to free the list element
(In reply to Jonas Ådahl from comment #14) > I suppose that, technically the patch that introduces this is a API change, Yeah, now that I think about it we have a few cases in gnome-shell that work around the lack of these events. Just grep for sync_hover and sync_pointer. I wonder if we should remove those now?
(In reply to Rui Matos from comment #20) > (In reply to Jonas Ådahl from comment #14) > > I suppose that, technically the patch that introduces this is a API change, > > Yeah, now that I think about it we have a few cases in gnome-shell that work > around the lack of these events. Just grep for sync_hover and sync_pointer. > I wonder if we should remove those now? You are right indeed. A bunch of those seem to compensate for the lack of crossing events on relayout. Others happen around various (un)grabs and need closer inspection. (In reply to Rui Matos from comment #19) > Review of attachment 346966 [details] [review] [review]: > > looks good to me, I think we should go ahead with it for now > > ::: clutter/clutter/clutter-stage.c > @@ +1238,3 @@ > + { > + _clutter_input_device_update (pointers->data, NULL, TRUE); > + pointers = g_slist_remove_link (pointers, pointers); > > I believe you want delete_link() here to free the list element Oops, yes :).
Created attachment 352429 [details] [review] clutter: Update pointer position on master clock's update stage Ensure the pointer position is up-to-date for the pointers inside the clip area after the stage got actors relayout.
Review of attachment 352429 [details] [review]: lgtm.
Attachment 339698 [details] pushed as 4982007 - wayland: Ensure we repick the pointer on synthesized crossing events Attachment 352429 [details] pushed as 4b23eb0 - clutter: Update pointer position on master clock's update stage
Created attachment 352434 [details] [review] switcherPopup: Ignore implicit enter events when the popup is mapped If the popup happens to be mapped beneath the pointer, mutter will now emit an implicit enter notify event (i.e. not caused by pointer motion). In this case the switcherPopup still goes and selects the item, which results in too sensitive alt-tab menus if the pointer happens to be in the wrong place. In order to preserve the older behavior, make it rely on motion events only, so pointer-driven highlighting always involves user interaction.
Review of attachment 352434 [details] [review]: > In order to preserve the older behavior That's not quite correct, as you can now wiggle the pointer on an unselected item to select it (e.g. move pointer over item, tab away, wiggle pointer). FWIW, as far as mouse interaction with the switcher is useful in the first place, the new behavior makes more sense to me, just being nitpicky here :-)
Right :). I changed the commit log accordingly, and filed bug #783005 for the stuff pointed out at comment #20. Attachment 352434 [details] pushed as e94de67 - switcherPopup: Ignore implicit enter events when the popup is mapped
This broke popups via Xwayland. My Firefox right-click menus were acting wierd, but I found an easier way to reproduce: 1. Run GDK_BACKEND=x11 zenity --warning 2. Right-click-and-hold over the label 3. Continue holding the button, dragging the pointer to "Select all" 4. Release button Expected result: The label should be selected Actual result: Nothing happens. I bisected it and the commit introducing the issue is commit 498200776c57aa25b35deecd56f43f28f53959e1 Author: Carlos Garnacho <carlosg@gnome.org> Date: Thu Sep 17 16:13:31 2015 +0200 wayland: Ensure we repick the pointer on synthesized crossing events Relayouts in clutter may trigger synthesized crossing events if the actor below the pointer changes. In that situation we do need to repick() the MetaWaylandPointer to end up with the right current wayland surface. https://bugzilla.gnome.org/show_bug.cgi?id=755164
Does not reproduce anymore, reclosing.