Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Every wl_pointer::button event is followed immediately by a wl_pointer::motion #1148

Closed
ids1024 opened this issue Sep 27, 2023 · 7 comments · Fixed by #1173
Closed

Every wl_pointer::button event is followed immediately by a wl_pointer::motion #1148

ids1024 opened this issue Sep 27, 2023 · 7 comments · Fixed by #1173

Comments

@ids1024
Copy link
Member

ids1024 commented Sep 27, 2023

This behavior seems undesirable and incorrect, even if clients normally aren't impacted by absolute motion to the same position. In particular it is an issue with pointer locking, since wl_pointer::motion events shouldn't be sent at all.

gdb shows unset_grab is responsible here. It seems the issue is that PointerGrab::button for ClickGrab is calling unset_grab, which then sends a motion event.

Should a grab really be used here? I guess a motion event may be necessary with other types of grabs, but not with this one, since it doesn't block motion events. But it's not clear how to do this. Perhaps wlroots could be worth looking at, since they have somewhat similar mechanisms...

With #1126 this motion also ends up in the same frame as the button event. Which maybe is correct if it is sent; or maybe it's implicit in the protocol that motion and button are separate event types that need to be in different frames? Though more generally, if set_grab and unset_grab send motion events, those should be followed by a frame.

@Drakulix
Copy link
Member

Should a grab really be used here? I guess a motion event may be necessary with other types of grabs, but not with this one, since it doesn't block motion events.

I think the grab has it's values here. But I would challenge that set_grab and unset_grab send events. Why is this current necessary and could we shift this responsibility to downstream?

@ids1024
Copy link
Member Author

ids1024 commented Oct 12, 2023

It seems in Sway this is handled with seatop_down, which restores seatop_default when the last button is release. This is seperate from wlroots pointer grabs.

Anyway, there are several problems here:

  • Synthesizing calls to motion anywhere lower in the stack than where pointer constraints are handled is broken with pointer locking, since it generates motion events that should be suppressed.
    • So calling it here is wrong if we handle that in the compositor's input event handler.
  • If this event is sent, it probably shoudn't be part of the frame of the button event? So it would be desirable for the vent to be sent after that is over and that frame has been sent.
  • A motion isn't necessary or desirable on ungrab for this particular grab, but may be necessary for others.

@Drakulix
Copy link
Member

Drakulix commented Oct 12, 2023

A motion isn't necessary or desirable on ungrab for this particular grab

Right, so we shouldn't send it.

but may be necessary for others.

The necessity should thus be expressed by the Grab-trait or preferrably the grab would somehow emit this event itself, if needed.

@ids1024
Copy link
Member Author

ids1024 commented Oct 12, 2023

Yeah, the specific grab could call it. Or their could be an associated const boolean indicating this is needed.

But this doesn't address the first point: no wl_pointer.motion motion events should be emitted when a pointer lock is active. That seems harder with how things currently work.

Though I'm not sure if PointerGrabs other than ClickGrab would occur while the pointer is locked. I guess if pointer constraints set a pointer grab, that could also help here.

@Drakulix
Copy link
Member

But this doesn't address the first point: no wl_pointer.motion motion events should be emitted when a pointer lock is active. That seems harder with how things currently work.

Though I'm not sure if PointerGrabs other than ClickGrab would occur while the pointer is locked. I guess if pointer constraints set a pointer grab, that could also help here.

Yeah that is tougher to solve. But the two other grabs that spring directly to my mind are move and resize grabs. And if the application is somehow requesting a valid move, while the pointer is locked, then imo the lock should be lifted.

In general we might want to consider pointer grabs breaking locks/constrains. If the pointer is "grabbed" by the compositor, it can't be locked/constraint by a client. (With the ClickGrab being the obvious exception.)

Optionally (if we don't want to special case the ClickGrab somehow), a grab could denote, if it should break locks and grabs. And if it doesn't, it shouldn't send motion events. That is somewhat easy to describe in the interface and can probably also be enforced in code.

@ids1024
Copy link
Member Author

ids1024 commented Oct 12, 2023

Good point. We could disable any active pointer constraints when a grab is activated; though this would have to exclude ClickGrab.

Moving the motion calls into just the grabs that need this should fix the main issue here at least, though there may be more to improve later.

@ids1024
Copy link
Member Author

ids1024 commented Oct 13, 2023

Oh yeah, and then there's the part where unset_grab can't actually access the grab (or even determine if there is an active grab) where this button/motion pair occurs since self.grab is GrabStatus::Borrowed.

ids1024 added a commit to ids1024/smithay that referenced this issue Oct 18, 2023
Used in `ClickGrab` to prevent `motion` events from occurring with every
`button` event. Otherwise, behavior should be unchanged. This matches
the argument taken by `KeyboardInnerHandle::unset_grab`.

This seems like the simplest solution. It would also be possible to add
a method to the `PointerGrab` trait indicating if focus should be
restored, but that's complicated since `unset_grab` can't access the
grab when it's `Borrowed`, so it would have to add a bool to
`GrabStatus::Borrowed`, etc.

This still doesn't send a `frame`, but since this takes a serial and a
time, it probably will be sent along with other pointer events, and
hopefully part of a `frame`. The Wayland spec isn't all that specific
about when things can/should be part of a `frame`...

Calling `motion` is also incorrect with pointer constraints, but grabs
other than `ClickGrab` generally shouldn't exist while a constraint is
active. It would be good to enforce that some way.

Fixes Smithay#1148.
ids1024 added a commit to ids1024/smithay that referenced this issue Oct 18, 2023
Used in `ClickGrab` to prevent `motion` events from occurring with every
`button` event. Otherwise, behavior should be unchanged. This matches
the argument taken by `KeyboardInnerHandle::unset_grab`.

This seems like the simplest solution. It would also be possible to add
a method to the `PointerGrab` trait indicating if focus should be
restored, but that's complicated since `unset_grab` can't access the
grab when it's `Borrowed`, so it would have to add a bool to
`GrabStatus::Borrowed`, etc.

This still doesn't send a `frame`, but since this takes a serial and a
time, it probably will be sent along with other pointer events, and
hopefully part of a `frame`. The Wayland spec isn't all that specific
about when things can/should be part of a `frame`...

Calling `motion` is also incorrect with pointer constraints, but grabs
other than `ClickGrab` generally shouldn't exist while a constraint is
active. It would be good to enforce that some way.

Fixes Smithay#1148.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants