Skip to content

Conversation

@stuartmorgan-g
Copy link
Contributor

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:

  • Should discrete pointer scroll and gesture pointer scroll use different events, rather than sharing? That would avoid the need to handle pointer scroll events in both the propagating and non-propagating paths handle the same event, and the somewhat odd behavior of the return value of scroll handlers being ignored in some cases, but would add more event types. (Just one for now, but I expect whatever pattern is used here will likely be repeated for zoom/trackpad pinch.)
  • How should the pointer scroll details classes be defined? I duplicated the drag details to see how similar they would end up, and they ended up being pretty much identical. I didn't want to use drag details as-is since these aren't drags, and renaming them would cause public API churn. Is extracting a superclass that Drag and PointerScroll derive from without any changes worthwhile?

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);
Copy link
Contributor

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.

Copy link
Contributor Author

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:

  1. 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.
  2. 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.

@Hixie
Copy link
Contributor

Hixie commented Sep 18, 2018

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. :-)

@stuartmorgan-g
Copy link
Contributor Author

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.

@Hixie
Copy link
Contributor

Hixie commented Oct 6, 2018

@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.

@stuartmorgan-g
Copy link
Contributor Author

Sounds good. I've pulled out #22762 as a starting point.

@Hixie
Copy link
Contributor

Hixie commented Oct 6, 2018

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.

@stuartmorgan-g
Copy link
Contributor Author

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.)

@stuartmorgan-g
Copy link
Contributor Author

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.)

@zoechi zoechi added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. f: gestures flutter/packages/flutter/gestures repository. labels Nov 28, 2018
@stuartmorgan-g
Copy link
Contributor Author

@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.

@ethanblake4
Copy link

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.

@stuartmorgan-g
Copy link
Contributor Author

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.)

@Hixie
Copy link
Contributor

Hixie commented Feb 22, 2019

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.

stuartmorgan-g pushed a commit to flutter/engine that referenced this pull request Mar 5, 2019
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.
@stuartmorgan-g
Copy link
Contributor Author

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

f: gestures flutter/packages/flutter/gestures repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants