Web trackpad pan #36346

moffatman · 2022-09-22T13:09:15Z

Use a kind of heuristic to guess at whether an event is from mouse or trackpad.

Part of flutter/flutter#64210

Sequence

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the [CLA].
All existing and new tests are passing.

justinmc

Sorry I did miss this PR among the other trackpad PRs!

lib/web_ui/lib/src/engine/pointer_binding.dart

justinmc

LGTM 👍

CC @mdebbar in case you or anyone else more familiar with the web engine wants to take a look before this is merged.

Edit: Just noting that this PR shouldn't be merged until the framework PR is merged. I tried them out together and left a review over there with some problems I ran into.

lib/web_ui/lib/src/engine/pointer_binding.dart

mdebbar

PR looks good to me with some minor suggestions.

lib/web_ui/lib/src/engine/pointer_binding.dart

Piinks · 2022-11-18T23:09:42Z

I'd like to test this out on Monday, I have been working on animating discrete mouse wheel data in flutter/flutter#115314, and ran into the fact that we don't distinguish between trackpad and mouse on web.
I've been catching up on the discussion in flutter/flutter#112171 (comment) as well.
I have a bunch of different mice and trackpads I have been testing with, so I can do the same here. Thanks for contributing this! This may enable a lot of different fixes in the framework!

skia-gold · 2022-11-20T22:25:29Z

Gold has detected about 70 new digest(s) on patchset 9.
View them at https://flutter-engine-gold.skia.org/cl/github/36346

skia-gold · 2022-11-20T23:25:29Z

Gold has detected about 2 new digest(s) on patchset 9.
View them at https://flutter-engine-gold.skia.org/cl/github/36346

moffatman · 2022-11-21T00:34:51Z

Ah I think the PR's Gold is broken now, I had the issue before, will probably have to recreate it...

Piinks · 2022-11-21T20:17:19Z

Ah I think the PR's Gold is broken now, I had the issue before, will probably have to recreate it...

I think you can resolve it by creating a new commit and not force pushing. Skia Gold is easily confused by force pushes unfortunately.

Piinks · 2022-11-21T21:05:43Z

Oh it looks like Gold is fine. 👍

Piinks · 2022-11-21T23:15:10Z

Ok I have tested this out with 4 mice and 2 trackpads running web on Mac. 😁

I ran a simple flutter app with an infinite number of scrolling items in a list, and had (in the framework) the event data output to tell me:

trackpad or mouse
time stamp
unconverted scroll delta (before it is divided by the device pixel ratio)
final scroll delta reported to the framework

For trackpads, every single one was correctly identified as a trackpad. 🎉

For the 4 mice, all of them were able to fool this into thinking it was a trackpad when I scrolled the wheel really fast. It was easier to do on some compared to others, and it would only occur once per occasion - the event before and after were correctly identified as mouse. The incoming delta was ~40 or ~320 in the instances I saw it occur.

On one of the cheap mouses I have, it was really easy to hit the 40 delta, so it was easier to reproduce. On one of the gaming mice, it was harder to trigger it, and would happen with an input of 320. I am guessing these devices all have a different level of precision.

Here is some of that output, let me know if it helps, or if I can gather more info for you. :)
This is one of the cheap mice that was really easy to trigger with a fling on the wheel.

Unconverted delta: 4.000244140625
PointerDeviceKind.mouse - 0:15:28.540299 : Offset(-0.0, 4.0)
PointerDeviceKind.mouse
Unconverted delta: 40.0189208984375
PointerDeviceKind.trackpad - 0:15:28.556200 : Offset(-0.0, 40.0)
PointerDeviceKind.trackpad
Unconverted delta: 168.1787109375
PointerDeviceKind.mouse - 0:15:28.564299 : Offset(-0.0, 168.2)
PointerDeviceKind.mouse
Unconverted delta: 500.9765625
PointerDeviceKind.mouse - 0:15:28.580200 : Offset(-0.0, 501.0)
PointerDeviceKind.mouse
Unconverted delta: 684.9090576171875
PointerDeviceKind.mouse - 0:15:28.596200 : Offset(-0.0, 684.9)
PointerDeviceKind.mouse
Unconverted delta: 365.3118896484375
PointerDeviceKind.mouse - 0:15:28.604200 : Offset(-0.0, 365.3)
PointerDeviceKind.mouse

moffatman · 2022-11-22T01:49:16Z

I was able to reproduce it, looks like sometimes macOS scroll acceleration can by chance happen to line up with expected values for a trackpad. Should be solvable with by cross-checking with the previous event, since this shouldn't happen for more than one event in a row.

moffatman · 2022-11-22T17:26:04Z

@Piinks See if you have the same problems with the latest change

Piinks · 2022-11-22T17:26:58Z

Thanks! I will check it out today!

Piinks · 2022-11-22T22:16:03Z

Still able to trip it at ~40:

Engine says: PointerDeviceKind.mouse
Unconverted delta: 4.000244140625
PointerDeviceKind.mouse - 0:00:05.808799 : Offset(-0.0, 4.0)
PointerDeviceKind.mouse
Engine says: PointerDeviceKind.trackpad
Unconverted delta: 39.991455078125
PointerDeviceKind.trackpad - 0:00:05.824900 : Offset(-0.0, 40.0)
PointerDeviceKind.trackpad
Engine says: PointerDeviceKind.mouse
Unconverted delta: 159.752197265625
PointerDeviceKind.mouse - 0:00:05.840900 : Offset(-0.0, 159.8)
PointerDeviceKind.mouse
Engine says: PointerDeviceKind.mouse
Unconverted delta: 229.3438720703125
PointerDeviceKind.mouse - 0:00:05.848900 : Offset(-0.0, 229.3)
PointerDeviceKind.mouse
Engine says: PointerDeviceKind.mouse
Unconverted delta: 1211.998291015625
PointerDeviceKind.mouse - 0:00:05.888900 : Offset(-0.0, 1212.0)
PointerDeviceKind.mouse

Let me know if there is more info I can gather.

moffatman · 2022-11-22T23:02:44Z

Can you capture the issue again with

print('deltaX=${event.deltaX}, deltaY=${event.deltaY}, wheelDeltaX=${event.wheelDeltaX}, wheelDeltaY=${event.wheelDeltaY}, timeStamp=${event.timeStamp}');

at the beginning of _isTrackpadEvent in pointer_binding.dart? Also would be good to see at least 2 events before the incorrect one.

Piinks · 2022-11-28T21:38:33Z

Can you capture the issue again with ...

Totally, whipping this up now. I was out for the holiday.

Also would be good to see at least 2 events before the incorrect one.

In the examples I shared, that is the whole event, from starting to ending the scroll on the stepped wheel. The incorrect one showed up on the second event.

Piinks · 2022-11-28T22:05:25Z

Here it is with one quick twirl on the mouse wheel:

Performing hot restart...                                          153ms
Restarted application in 153ms.
deltaX=-0.0, deltaY=4.000244140625, wheelDeltaX=0, wheelDeltaY=-120, timeStamp=27866.300000190735
Mouse
deltaX=-0.0, deltaY=39.9945068359375, wheelDeltaX=0, wheelDeltaY=-120, timeStamp=27882.200000286102
Trackpad
deltaX=-0.0, deltaY=433.90625, wheelDeltaX=0, wheelDeltaY=-1200, timeStamp=27906.200000286102
Mouse
deltaX=-0.0, deltaY=654.576416015625, wheelDeltaX=0, wheelDeltaY=-1800, timeStamp=27922.300000190735
Mouse
deltaX=-0.0, deltaY=764.7760009765625, wheelDeltaX=0, wheelDeltaY=-2160, timeStamp=27938.200000286102
Mouse

Piinks · 2022-11-29T00:35:10Z

Nice! I just tested it with all of the devices I have, it was correct every time. 🎉

Piinks

@yjbanov was also interested in this.. FYI

This LGTM!

yjbanov · 2022-11-29T01:10:45Z

Thanks @Piinks for jumping in and testing it thoroughly! Great to see progress made. I'm curious what failures would look and feel like if this logic fails. We can't enumerate all browser engines, and some browser engines may be customized to use different numbers. In that case, will we observe slight degradations in scrolling quality? Or could the issues be severe?

moffatman · 2022-11-29T02:02:52Z

This doesn't affect scrolling, it's only for custom interactions such as InteractiveViewer. If there was some problem you might see strangeness there where during a pan it might zoom by accident on one event in a series. We can gate it to only certain browser / OS combinations if you want. Already Firefox is ruled out. I personally have only verified it on macOS Chrome and Safari.

Piinks · 2022-11-29T20:17:44Z

Thanks for explaining @moffatman. I plan to use this in the framework to execute an animation when mouse wheel scroll input comes from a mouse, ignoring trackpad - as an additional example of how this will be used. From my understanding, this change will not necessarily change anything in the framework unless we make a change there based on the input device.

Basically, bias towards choosing mouse, but if timestamps are available, we can check the previous event and ensure that false-mouses are avoided.

Apparently some high-precision mice use 120 instead of 240 as the wheelDelta per tick.

moffatman · 2022-12-01T20:33:02Z

flutter/flutter#112171 needs to be approved and merged first due to the assertion changes

Also fix setting of timeStamp in test.

moffatman · 2022-12-05T01:11:18Z

@Piinks I updated the logic again, can you recheck with the various devices you have and see if there is a regression? Thanks 😄

Piinks · 2022-12-05T20:18:26Z

Sure thing! I have even more mice now. 😳

Piinks · 2022-12-05T22:39:08Z

Tested with 7 mice and 2 trackpads. I was not able to fool it. @justinmc had different luck last time with the magic mouse. On my magic mouse, I swiped and flung on it a bunch but did not see it come through as a mouse. 👍

justinmc · 2022-12-05T22:42:05Z

@Piinks That's great! I'll try again with my magic mouse tonight and then plan to merge flutter/flutter#112171 if I can't reproduce that bug.

justinmc

Just some last minute nits.

justinmc · 2022-12-05T22:51:14Z

lib/web_ui/lib/src/engine/pointer_binding.dart

@@ -349,7 +350,7 @@ mixin _WheelEventListenerMixin on _BaseAdapter {
    return (wheelDelta - (-3 * delta)).abs() > 1;
  }

-  bool _isTrackpadEvent(DomWheelEvent event, DomWheelEvent? lastEvent) {
+  bool _isTrackpadEvent(DomWheelEvent event, DomWheelEvent? lastEvent, bool? lastEventWasTrackpad) {


Is it possible to use _lastWheelEventWasTrackpad directly instead of passing it in?

Also, now that _isTrackpadEvent doesn't call itself, maybe you don't need to pass in _lastWheelEvent either.

That's true, I'll change it to just use the members directly.

justinmc · 2022-12-05T22:53:41Z

lib/web_ui/lib/src/engine/pointer_binding.dart

@@ -264,6 +264,7 @@ abstract class _BaseAdapter {
  final PointerDataConverter _pointerDataConverter;
  final KeyboardConverter _keyboardConverter;
  DomWheelEvent? _lastWheelEvent;
+  bool? _lastWheelEventWasTrackpad;


Nit: What do you think about making this non-nullable and initializing it to false? Currently it probably represents reality more accurately as-is, because a value of null means that there was no last wheel event. However, that null state is not used anywhere.

Up to you what you think is better.

I have it this way as in some previous iterations the null state did matter.

justinmc

LGTM 👍

I left a review on the framework PR (flutter/flutter#112171) saying that I was unable to break this using the Magic Mouse this time. Thanks for fixing that and for cleaning up those last few comments!

Rerunning the Fuchsia test, it looked like an infrastructure flake.

Piinks · 2022-12-12T22:39:14Z

The framework change landed, I am just waiting on a lean build from that in the framework and then we can land this. 🎉

…116939) * b97d3545f [Impeller] Speculatively attempt to fix Metal PSO construction errors on host targets. (flutter/engine#38229) * 99f3d1931 Web trackpad pan (flutter/engine#36346) * e144f81e9 [impeller] Fix typo in compiller help (flutter/engine#38214)

* Guess at trackpad pans on web * Add test * Update comment * Handle macOS accelerated scroll wheel * Fix test after last commit * Disable on firefox * Pull out _isTrackpadEvent and add doc links * Fix issue with floating point / integer conversion error. * Workaround for magic mouse events which happen to be divisible by 120. * Refactor to handle bad luck in accelerated mouse deltas. Basically, bias towards choosing mouse, but if timestamps are available, we can check the previous event and ensure that false-mouses are avoided. * Use 120 wheelDelta to identify mouse-accelerated events instead of 240 Apparently some high-precision mice use 120 instead of 240 as the wheelDelta per tick. * Handle multiple bad-luck events in a row. Also fix setting of timeStamp in test. * Cleanup parameters

…lutter#116939) * b97d3545f [Impeller] Speculatively attempt to fix Metal PSO construction errors on host targets. (flutter/engine#38229) * 99f3d1931 Web trackpad pan (flutter/engine#36346) * e144f81e9 [impeller] Fix typo in compiller help (flutter/engine#38214)

ditman · 2023-12-21T20:48:43Z

lib/web_ui/lib/src/engine/pointer_binding.dart

+      // While not in any formal web standard, `blink` and `webkit` browsers use
+      // a delta of 120 to represent one mouse wheel turn. If both dimensions of


/~https://github.com/chromium/chromium/blob/fd8a8914ca0183f0add65ae55f04e287543c7d4a/ui/events/event.cc#L676-L684

flutter-dashboard bot added the platform-web Code specifically for the web engine label Sep 22, 2022

moffatman mentioned this pull request Sep 22, 2022

InteractiveViewer discrete trackpad panning flutter/flutter#112171

Merged

8 tasks

justinmc reviewed Sep 29, 2022

View reviewed changes

lib/web_ui/lib/src/engine/pointer_binding.dart Outdated Show resolved Hide resolved

lib/web_ui/lib/src/engine/pointer_binding.dart Outdated Show resolved Hide resolved

moffatman force-pushed the web_trackpad_pan branch from 4f0b40d to 56f9e97 Compare October 2, 2022 23:04

justinmc approved these changes Oct 26, 2022

View reviewed changes

lib/web_ui/lib/src/engine/pointer_binding.dart Outdated Show resolved Hide resolved

moffatman force-pushed the web_trackpad_pan branch from 56f9e97 to 4b5d311 Compare October 27, 2022 05:55

mdebbar approved these changes Oct 31, 2022

View reviewed changes

lib/web_ui/lib/src/engine/pointer_binding.dart Outdated Show resolved Hide resolved

lib/web_ui/lib/src/engine/pointer_binding.dart Outdated Show resolved Hide resolved

moffatman force-pushed the web_trackpad_pan branch 2 times, most recently from aa41020 to 55505af Compare November 13, 2022 00:59

moffatman force-pushed the web_trackpad_pan branch from fcbd2b7 to 59bb9f9 Compare November 20, 2022 23:02

moffatman force-pushed the web_trackpad_pan branch from f2ebf49 to b3d1a27 Compare November 29, 2022 00:21

Piinks approved these changes Nov 29, 2022

View reviewed changes

moffatman added 5 commits December 1, 2022 13:01

Pull out _isTrackpadEvent and add doc links

50c21a0

Fix issue with floating point / integer conversion error.

0f2400e

Workaround for magic mouse events which happen to be divisible by 120.

31d04af

Refactor to handle bad luck in accelerated mouse deltas.

85a0a58

Basically, bias towards choosing mouse, but if timestamps are available, we can check the previous event and ensure that false-mouses are avoided.

Use 120 wheelDelta to identify mouse-accelerated events instead of 240

426cac0

Apparently some high-precision mice use 120 instead of 240 as the wheelDelta per tick.

moffatman force-pushed the web_trackpad_pan branch from b3d1a27 to 426cac0 Compare December 1, 2022 18:01

Handle multiple bad-luck events in a row.

a07c922

Also fix setting of timeStamp in test.

justinmc reviewed Dec 5, 2022

View reviewed changes

Cleanup parameters

48b93bd

justinmc approved these changes Dec 6, 2022

View reviewed changes

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 12, 2022

auto-submit bot merged commit 99f3d19 into flutter:main Dec 12, 2022

engine-flutter-autoroll mentioned this pull request Dec 13, 2022

Roll Flutter Engine from 6c190ea1e8df to e144f81e92d8 (3 revisions) flutter/flutter#116939

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 13, 2022

99f3d1931 Web trackpad pan (flutter/engine#36346)

4584ea1

moffatman mentioned this pull request Jan 5, 2023

Trackpad scrolling when mouse is down on web throws an assertion flutter/flutter#118051

Closed

Piinks mentioned this pull request Jan 30, 2023

Add unique device id for trackpad on web #39260

Merged

8 tasks

Piinks mentioned this pull request Dec 12, 2023

Differentiate trackpad from mouse wheels on web flutter/flutter#139990

Open

ditman reviewed Dec 21, 2023

View reviewed changes

		// While not in any formal web standard, `blink` and `webkit` browsers use
		// a delta of 120 to represent one mouse wheel turn. If both dimensions of

Web trackpad pan #36346

Web trackpad pan #36346

Conversation

moffatman commented Sep 22, 2022 • edited Loading

Sequence

Pre-launch Checklist

justinmc left a comment

Choose a reason for hiding this comment

justinmc left a comment • edited Loading

Choose a reason for hiding this comment

mdebbar left a comment

Choose a reason for hiding this comment

Piinks commented Nov 18, 2022

skia-gold commented Nov 20, 2022

skia-gold commented Nov 20, 2022

moffatman commented Nov 21, 2022

Piinks commented Nov 21, 2022

Piinks commented Nov 21, 2022

Piinks commented Nov 21, 2022

moffatman commented Nov 22, 2022

moffatman commented Nov 22, 2022

Piinks commented Nov 22, 2022

Piinks commented Nov 22, 2022

moffatman commented Nov 22, 2022

Piinks commented Nov 28, 2022

Piinks commented Nov 28, 2022

Piinks commented Nov 29, 2022

Piinks left a comment

Choose a reason for hiding this comment

yjbanov commented Nov 29, 2022 • edited Loading

moffatman commented Nov 29, 2022

Piinks commented Nov 29, 2022

moffatman commented Dec 1, 2022

moffatman commented Dec 5, 2022

Piinks commented Dec 5, 2022

Piinks commented Dec 5, 2022

justinmc commented Dec 5, 2022

justinmc left a comment

Choose a reason for hiding this comment

justinmc Dec 5, 2022

Choose a reason for hiding this comment

justinmc Dec 5, 2022

Choose a reason for hiding this comment

moffatman Dec 5, 2022

Choose a reason for hiding this comment

justinmc Dec 5, 2022

Choose a reason for hiding this comment

moffatman Dec 5, 2022

Choose a reason for hiding this comment

justinmc left a comment • edited Loading

Choose a reason for hiding this comment

Piinks commented Dec 12, 2022

ditman Dec 21, 2023

Choose a reason for hiding this comment

moffatman commented Sep 22, 2022 •

edited

Loading

justinmc left a comment •

edited

Loading

yjbanov commented Nov 29, 2022 •

edited

Loading

justinmc left a comment •

edited

Loading