Multiple touches of a stylus should be considered from the same device #56075

dkwingsmt · 2024-10-24T00:48:55Z

Problem: The issue above only occurred when using stylus on Web. Multiple touches were treated as different devices: new devices were added and former devices that had the pointer lifted were never removed. This was further caused by the design that Flutter uses JS's PointerEvent.pointerId as the device ID, which increases for each touch.

Flutter had to use PointerEvent.pointerId because JS doesn't provide a way to distinguish between multiple styluses. This PR fixes this issue by simply supporting only one stylus, since support for multi-stylus can be really hard. How multi-stylus should be supported can be discussed upon such demands in the future, but at least for now, iPad doesn't support multi-stylus and it's not something I can test.

This PR also rewrote some comments that I got confused by while reading the code.

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

If you need help, consider asking for advice on the #hackers-new channel on Discord.

ditman

LGTM! Small question about the switch returned in _getPointerId.

ditman · 2024-10-24T18:22:14Z

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

+    return switch(_pointerTypeToDeviceKind(event.pointerType!)) {
+      ui.PointerDeviceKind.mouse => _mouseDeviceId,
+      ui.PointerDeviceKind.stylus => _stylusDeviceId,
+      _ => event.pointerId!.toInt(),


I guess _ case covers the multi-tapping with fingers or in the trackpad?

(Should we make this switch exhaustive, for clarity?)

The switch case as well as _ is just a refactor of what was before the PR, except that I added a branch for stylus. I don't think it changes how it behaves for trackpad.

Also, AFAIK, trackpad events are handled as wheel events in _convertWheelEventToPointerData, which does not call _getPointerId, but always use _mouseDeviceId instead.

ditman · 2024-10-24T18:23:58Z

This PR also rewrote some comments that I got confused by while reading the code.

(PS: Thanks for this!)

dkwingsmt · 2024-10-24T19:28:24Z

@ditman Thanks for your suggestion of making the switch exhaustive. I've made the change, during which I found out that I didn't include invertedStylus. :)

I've made it clear that trackpad isn't reachable in the switch statement.

ditman

Small question about multi-touching on a "trackpad" (those shouldn't be "wheel" events, I think?

ditman · 2024-10-31T22:01:11Z

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

+    // Move event listeners should be added to `_globalTarget` instead of
+    // `_viewTarget`. This is because `_viewTarget` (the root) captures pointers
+    // by default, meaning a pointer that starts within `_viewTarget` continues
+    // sending move events to its listener even when dragged outside.


I haven't revisited this yet, BUT, there's a way to have the behavior that we needed _globalTarget for from the _viewTarget!!

See this: https://developer.mozilla.org/en-US/docs/Web/API/Element/setPointerCapture

Yeah, that's what I thought as well. Maybe you can change it in a future PR :)

I've added a TODO with issue flutter/flutter#157968

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

…ouches

…same device (flutter/engine#56075)

…158127) flutter/engine@05cb5d7...25c7e47 2024-11-04 [email protected] Retry mac_unopt one time in presubmit (flutter/engine#56319) 2024-11-04 [email protected] [Impeller] faster descriptor type mapping. (flutter/engine#56351) 2024-11-04 [email protected] Roll Skia from 75740b68a282 to e2ad60ea8039 (8 revisions) (flutter/engine#56354) 2024-11-04 [email protected] Move detection of cutouts in Android engine to `onApplyWindowInsets` (flutter/engine#55992) 2024-11-04 [email protected] Roll Dart SDK from 69cec5dc51f9 to f238183cf168 (1 revision) (flutter/engine#56353) 2024-11-04 [email protected] Roll Skia from bab7d954758b to 75740b68a282 (1 revision) (flutter/engine#56349) 2024-11-04 [email protected] Roll Fuchsia Linux SDK from 07KmbdEtnhkg_tUhe... to amgHXcqtplha8LuI_... (flutter/engine#56348) 2024-11-03 [email protected] Roll Skia from 6944cd128603 to bab7d954758b (2 revisions) (flutter/engine#56346) 2024-11-02 [email protected] Roll Dart SDK from 61bf0877807e to 69cec5dc51f9 (2 revisions) (flutter/engine#56335) 2024-11-02 [email protected] Multiple touches of a stylus should be considered from the same device (flutter/engine#56075) 2024-11-02 [email protected] Roll Skia from 89ac72bb4922 to 6944cd128603 (2 revisions) (flutter/engine#56331) 2024-11-02 [email protected] [web] Transfer focus to view rootElement on blur/remove. (flutter/engine#55045) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 07KmbdEtnhkg to amgHXcqtplha If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

flutter/engine#56075) This PR fixes flutter#156223. **Problem:** The issue above only occurred when using stylus on Web. Multiple touches were treated as different devices: new devices were added and former devices that had the pointer lifted were never removed. This was further caused by the design that Flutter uses JS's `PointerEvent.pointerId` as the device ID, which increases for each touch. Flutter had to use `PointerEvent.pointerId` because JS doesn't provide a way to distinguish between multiple styluses. This PR fixes this issue by simply supporting only one stylus, since support for multi-stylus can be really hard. How multi-stylus should be supported can be discussed upon such demands in the future, but at least for now, iPad doesn't support multi-stylus and it's not something I can test. This PR also rewrote some comments that I got confused by while reading the code. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

dkwingsmt added 2 commits October 23, 2024 17:44

Tests

2a4747e

Changes

2274f7b

github-actions bot added the platform-web Code specifically for the web engine label Oct 24, 2024

Fix lint

afbea48

dkwingsmt requested a review from ditman October 24, 2024 18:16

ditman approved these changes Oct 24, 2024

View reviewed changes

Exhaustive switch

60a2fc3

Merge branch 'main' into stylus-touches

fa98db7

ditman approved these changes Oct 31, 2024

View reviewed changes

dkwingsmt mentioned this pull request Oct 31, 2024

Investigate whether Web can listen to "pointermove" events on _viewTarget flutter/flutter#157968

Open

dkwingsmt and others added 4 commits October 31, 2024 15:31

Fix style and add TODO

8085430

Merge remote-tracking branch 'origin/main' into stylus-touches

0960998

Merge remote-tracking branch 'dkwingsmt/stylus-touches' into stylus-t…

bf0cd49

…ouches

Merge branch 'main' into stylus-touches

c8d2864

dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 2, 2024

auto-submit bot merged commit 114f7dd into flutter:main Nov 2, 2024
30 checks passed

dkwingsmt deleted the stylus-touches branch November 2, 2024 07:11

engine-flutter-autoroll mentioned this pull request Nov 2, 2024

Roll Flutter Engine from 05cb5d7f7939 to 718f3a0af52b (4 revisions) flutter/flutter#158046

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 2, 2024

114f7dd7c Multiple touches of a stylus should be considered from the …

3e3dae1

…same device (flutter/engine#56075)

engine-flutter-autoroll mentioned this pull request Nov 2, 2024

Roll Flutter Engine from 05cb5d7f7939 to 718f3a0af52b (4 revisions) flutter/flutter#158052

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 2, 2024

114f7dd7c Multiple touches of a stylus should be considered from the …

9984cf4

…same device (flutter/engine#56075)

engine-flutter-autoroll mentioned this pull request Nov 2, 2024

Roll Flutter Engine from 05cb5d7f7939 to 718f3a0af52b (4 revisions) flutter/flutter#158059

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 2, 2024

114f7dd7c Multiple touches of a stylus should be considered from the …

b3c05f7

…same device (flutter/engine#56075)

engine-flutter-autoroll mentioned this pull request Nov 4, 2024

Roll Flutter Engine from 05cb5d7f7939 to a215a3e97fa8 (5 revisions) flutter/flutter#158107

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 4, 2024

114f7dd7c Multiple touches of a stylus should be considered from the …

dc9242a

…same device (flutter/engine#56075)

engine-flutter-autoroll mentioned this pull request Nov 4, 2024

Roll Flutter Engine from 05cb5d7f7939 to 58f56178cabe (7 revisions) flutter/flutter#158111

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 4, 2024

114f7dd7c Multiple touches of a stylus should be considered from the …

99efb83

…same device (flutter/engine#56075)

engine-flutter-autoroll mentioned this pull request Nov 4, 2024

Roll Flutter Engine from 05cb5d7f7939 to 4fb69c36cad1 (9 revisions) flutter/flutter#158116

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 4, 2024

114f7dd7c Multiple touches of a stylus should be considered from the …

fb66224

…same device (flutter/engine#56075)

engine-flutter-autoroll mentioned this pull request Nov 4, 2024

Roll Flutter Engine from 05cb5d7f7939 to 2bea69447ab6 (10 revisions) flutter/flutter#158117

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 4, 2024

114f7dd7c Multiple touches of a stylus should be considered from the …

cdc418c

…same device (flutter/engine#56075)

engine-flutter-autoroll mentioned this pull request Nov 4, 2024

Roll Flutter Engine from 05cb5d7f7939 to aa3423f77d94 (11 revisions) flutter/flutter#158122

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 4, 2024

114f7dd7c Multiple touches of a stylus should be considered from the …

77e1411

…same device (flutter/engine#56075)

engine-flutter-autoroll mentioned this pull request Nov 4, 2024

Roll Flutter Engine from 05cb5d7f7939 to 25c7e471e2ef (12 revisions) flutter/flutter#158127

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 4, 2024

114f7dd7c Multiple touches of a stylus should be considered from the …

52958a5

…same device (flutter/engine#56075)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Multiple touches of a stylus should be considered from the same device #56075

Multiple touches of a stylus should be considered from the same device #56075

Uh oh!

dkwingsmt commented Oct 24, 2024 •

edited

Loading

Uh oh!

ditman left a comment

Uh oh!

ditman Oct 24, 2024

Uh oh!

dkwingsmt Oct 24, 2024

Uh oh!

ditman commented Oct 24, 2024

Uh oh!

dkwingsmt commented Oct 24, 2024

Uh oh!

ditman left a comment

Uh oh!

ditman Oct 31, 2024

Uh oh!

dkwingsmt Oct 31, 2024

Uh oh!

dkwingsmt Oct 31, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Multiple touches of a stylus should be considered from the same device #56075

Multiple touches of a stylus should be considered from the same device #56075

Uh oh!

Conversation

dkwingsmt commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

ditman Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

ditman commented Oct 24, 2024

Uh oh!

dkwingsmt commented Oct 24, 2024

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

ditman Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dkwingsmt commented Oct 24, 2024 •

edited

Loading