-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Don't drag draggables on scroll events #29842
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
Conversation
Why do we need a custom picker / why does the regular picker fire on scroll events? Should it be fixed generally on the regular picker? |
A scroll event comes in looking just like a button press (but with a higher number). It does not seem unreasonable to allow these to trigger pick events in general but to disable them for draggable things specifically. |
I don't think I understand this: "Pick" is logically selecting an element by clicking on it. No matter the technical implementation, why should scrolling ever be interpreted as pick? In what use case would that make sense? |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anybody can merge after adding the suggested documentation.
@@ -1500,6 +1500,10 @@ def __init__(self, ref_artist, use_blit=False): | |||
] | |||
] | |||
|
|||
@staticmethod | |||
def _picker(artist, mouseevent): | |||
return (artist.contains(mouseevent) and mouseevent.name != "scroll_event"), {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return (artist.contains(mouseevent) and mouseevent.name != "scroll_event"), {} | |
""" | |
A custom picker to prevent dragging on scroll events. | |
Dragging is initialized through a pick events, which is typically a | |
mouse click. However, mouse scrolling also fires pick events and | |
we need to filter them out so that scrolling does not initalize | |
dragging. | |
""" | |
return (artist.contains(mouseevent) and mouseevent.name != "scroll_event"), {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to adding a comment - I thought this was a bit verbose for a private helper function, so I added a one-liner instead - what do you think?
PR summary
Fixes #29142; replaces #29270. The solution here is to set a custom picker on
DraggableBase
that doesn't fire if the event is a scroll event.PR checklist