-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Add support for scrollwheels and trackpad scrolling. #21953
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
Some input sources, such as macOS trackpads, provide scroll events as continuous gestures rather than discrete events like scroll wheels. This adds support for treating those gestures as Flutter gestures, enabling things like overscroll for gesture-based scroll events.
|
|
||
| /// Override this method to dispatch events. | ||
| void dispatchEvent(PointerEvent event, HitTestResult result); | ||
| void dispatchEvent(PointerEvent event, HitTestResult result, bool propagating); |
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.
we probably can't change this API, that's a pretty serious breaking change.
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.
This is the case I was thinking of when I asked about breaking API changes recently. The other options I can see:
- Add a dispatchPropagatingEvent method to this interface instead of adding the param. Because HitTestDispatcher is used as an interface, I can't automatically add an empty implementation, so that breaks the same number of clients as this approach.
- Add a new interface with the new method, and then make GestureBinding (and WidgetTester) implement both interfaces. The downside here is that logically the dispatching of propagating events seems like it belongs in the interface that's about dispatching events.
Given that only one non-test class in all of Flutter implemented this interface I assumed it was unlikely that third parties were using it directly, but I could certainly do 2 instead if the breaking concern is bigger than the long-term design cruft of a second interface.
|
I don't really understand the design here. Can you give an overview of the general approach, to give me context for the review? There's a lot of low-level changes which I'm skeptical about but I'd like to see if I can understand the overall approach before giving negative feedback. :-) |
|
High level, there are two kinds of scrolling behavior I'm implementing: discrete event (e.g., scroll wheel ticks) where each scroll event is unrelated to the others, and pointer gesture (e.g., trackpad scroll on macOS) where the the events are part of a stream that includes a start, updates, and an end. On macOS at least the latter can't be fully implemented using the former because the gesture version does iOS-style overscroll. (If there are significant concerns about the gesture version though, landing just the discrete event implementation would still be a huge improvement over no scroll support at all.) I can easily split the two scrolling types into separate PRs if you'd prefer; I only bundled them since I thought it would be useful to see how the two paths compare. The commits are already separate, with the gesture code being the last commit in this PR (10ebef4) and the discrete version being everything before that. The discrete version adds an alternate event dispatch system that uses an added propagation-with-stopping model. (Another option would be to implement it by synthesizing a start/update/end sent through the gesture scroll path, but that felt hacky, especially since I'd need to add metadata that would cause it to suppress the normal overscroll behavior.) The gesture version adds a parallel system to the existing drag scroll recognizer/controller/activity system for pointer gesture scrolling. It uses the arena like the other gestures to ensure that only one in a nested set of scrollables handles the gesture. (Currently the gesture scroll uses the same top-level event type as the discrete scroll. Another approach would be to make new generic pointer gesture start/update/end events, annotated with the event type, similar to what is done at the PointerData level in the supporting engine patch.) I would have liked to unify the drag-scroll and pointer-gesture-scroll paths more, but that would have broken a whole lot more APIs (which used "drag" types and names) and thus didn't seem viable. Given that this is plumbing new types of events (both discrete propagating events and a different type of gestures that don't correspond to down/move/up sequences) I didn't see a way to do it without low-level changes. But I'm not attached to this specific design so negative feedback is welcome; I did this as a patch rather than a design doc to start with since I felt like it was necessary to see the concrete details in context to be able to discuss further. I'm happy to step back and write docs for any big-picture questions that come up during review. |
|
@stuartmorgan there's a lot going on with this PR, including a lot of new API surface, which makes it non-trivial to review. It might be easiest if we break it down into separable components and review each one in turn. Alternatively, I'd be happy to drop by your desk and we can look at it together and discuss the design with a whiteboard. |
|
Sounds good. I've pulled out #22762 as a starting point. |
|
Thanks, that really helped with explaining this PR too. Can you talk a little about the thinking behind using the gesture recognizer logic for the trackpad side of things? (As opposed, to, say, using the same propagation mechanism, but just augmenting it with a "velocity" field that is non-zero when the user lets go.) I'm actually surprised to hear that macOS exposes any of this information, I had assumed they faked it all as physical mouse wheel HID events. |
|
Originally I thought I would do that (just use discrete events for everything) as well, but it turns out that macOS has trackpad scroll handling that has all the same semantics as iOS gesture scrolling. For instance, if you have a MacBook, try scrolling with a trackpad in Finder list/column view, or play with two-finger horizontal swipe in Safari. The behaviors are exactly what Flutter's current drag gestures do: you can overscroll, you can hold an overscroll by keeping your fingers on the trackpad preventing physics from taking over until you release them, you can fling, etc. And if you're doing a trackpad scroll the event delivery locks to the view where the gesture started, regardless of the location of the cursor during the rest of the event, just as with Flutter's gesture handling system. If you only have discrete events, most of that becomes impossible; there's no way to implement a hold, for instance, without any concept of a gesture being in progress vs. complete. It's certainly possible to take macOS trackpad scroll events and treat them like a very fine-grained scroll wheel that gets sent into the system implemented in #22762, but you can't get the right behavior by doing that. (As I mentioned earlier, it's still a huge improvement over the current desktop behavior.) |
|
Any further information I can provide to help move this/22762 forward? (There's an engine patch to add the event data that I've been waiting on sending out until there's at least consensus on 22762.) |
|
@Hixie Does the reasoning for using the gesture system here make sense given the comment above? Let me know if you want to chat offline about the approach here. If you think the overall strategy is reasonable I can re-sync this with master and my latest round of scrollwheel changes, so the gesture part can be potentially be reviewed in parallel. |
|
Just chiming in to note that Windows also has support for 'continuous' scrolling with overscroll etc. via the Direct Manipulation API (for Win32) or the GestureRecognizer API (for UWP). Many Win32 apps do not support it currently, but as someone with a Precision Touchpad-equipped laptop it would be really nice to see Flutter support. |
|
Capturing from discussion: the next step here is for me to write up a doc of the trackpad interactions we expect to handle, including details of the OS events. (In the meantime, re-synced with the current scrollwheel branch so that this applies on head again, just so the current state isn't badly bitrotted.) |
|
Looking at the mouse wheel PR, I think one API that might make sense is for the start of a "meaningful signal sequence" to be dispatched using the "signal" logic, and for that to include some mechanism (e.g. a Stream) to listen to the rest of the events related to that specific sequence. |
Adds support for pointer signals, in a way that will support both discrete events (e.g., scroll wheels, flutter/flutter#22762) and continuous gestures (e.g., trackpad scroll, flutter/flutter#21953). Also exposes these new event options to the embedder. Does not include code to send the new events from the platform shells.
|
Per offline discussion, closing this for now; it'll need the doc mentioned above, and likely reworking to address Hixie's last comment, and it will likely be 2+ weeks before I'll have time to focus on this again. I do still plan to add the improved trackpad scrolling support, I just don't want this to continue to clutter the queue until then. |
This isn't landable in its current state (needs to be resync'd to master, missing tests) but is far enough along that it should be a good starting point for discussion.
In addition to validating the overall big-picture design, some specific design questions that I think need to be explored: