Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add unique device id for trackpad on web #39260

Merged
merged 3 commits into from Feb 13, 2023
Merged

Add unique device id for trackpad on web #39260

merged 3 commits into from Feb 13, 2023

Conversation

Piinks
Copy link
Contributor

@Piinks Piinks commented Jan 30, 2023

Fixes flutter/flutter#118051

After #36346, it was possible to trip an assertion in the framework since we were differentiating mouse & trackpad on web - but weren't giving them unique IDs. This adds that.

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.

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

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Jan 30, 2023
moffatman
moffatman reviewed Jan 30, 2023
View reviewed changes
lib/web_ui/test/engine/pointer_binding_test.dart Outdated
@@ -1330,6 +1334,7 @@ void testMain() {
packets[4].data[0].signalKind, equals(ui.PointerSignalKind.scroll));
expect(
packets[4].data[0].kind, equals(ui.PointerDeviceKind.trackpad));
expect(packets[4].data[0].pointerIdentifier, equals(-2));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be device?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh 🤦‍♀️ - thanks. Still recovering from jet lag.

@moffatman
Copy link
Contributor

I think the test problem is that now there are two different device IDs, so the first mouse event will have a synthesized add event.

@moffatman moffatman closed this Jan 30, 2023
@moffatman moffatman reopened this Jan 30, 2023
@moffatman
Copy link
Contributor

Wrong button 😁

yjbanov
yjbanov approved these changes Jan 30, 2023
View reviewed changes
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but cc @mdebbar @htoor3 who own the input area.

htoor3
htoor3 approved these changes Jan 30, 2023
View reviewed changes
Copy link
Contributor

@htoor3 htoor3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Piinks
Copy link
Contributor Author

Piinks commented Jan 30, 2023

I think the test problem is that now there are two different device IDs, so the first mouse event will have a synthesized add event.

Hmm. I will take another look tomorrow. Thanks!

@Piinks
Copy link
Contributor Author

Piinks commented Feb 13, 2023

I think the test problem is that now there are two different device IDs, so the first mouse event will have a synthesized add event.

@moffatman did you mean that the test just need to be updated? I figured the prior synthesized event may need to be removed or cancelled, but that doesn't seem right. There could be a mouse and a trackpad both at play at the same time.

@moffatman
Copy link
Contributor

Yeah, the test just needs to be updated for the new behaviour. packets[5] now has [add, scroll] instead of just [scroll]. Since it's now using a different device ID from the earlier events.

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 13, 2023
@auto-submit auto-submit bot merged commit 89d41d1 into flutter:main Feb 13, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 14, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 14, 2023
@engine-flutter-autoroll
89d41d13e Add unique device id for trackpad on web (flutter/engine#39260
22fb460 
)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 14, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 14, 2023
@engine-flutter-autoroll
89d41d13e Add unique device id for trackpad on web (flutter/engine#39260
6d4b18f 
)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 14, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 14, 2023
@engine-flutter-autoroll
89d41d13e Add unique device id for trackpad on web (flutter/engine#39260
d2f1e1e 
)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 14, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 14, 2023
@engine-flutter-autoroll
89d41d13e Add unique device id for trackpad on web (flutter/engine#39260
1950b20 
)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 14, 2023
@engine-flutter-autoroll
Roll Flutter Engine from 31a4648cbe99 to c4f51bc78644 (7 revisions) (#…
b08cc8b 
…120656)

* 8cd648d1d Roll Dart SDK from f80c5db8736a to ea59504416a8 (1 revision) (flutter/engine#39594)

* 9ac09ced1 [Impeller] Fix unsafe access for clip stencil coverage (flutter/engine#39595)

* 99a81a81f Add support for double tap action from Apple Pencil 2 (flutter/engine#39267)

* 89d41d13e Add unique device id for trackpad on web (flutter/engine#39260)

* f7dfb2b63 remove use of SkCanvas and DLCanvasRecorder from ui.Canvas native code (flutter/engine#39599)

* c2e165e36 Fix multi-function compute (flutter/engine#39603)

* c4f51bc78 Revert "Add support for double tap action from Apple Pencil 2 (#39267)" (flutter/engine#39607)
0xZOne pushed a commit to 0xZOne/engine that referenced this pull request Feb 20, 2023
@Piinks @0xZOne
Add unique device id for trackpad on web (flutter#39260)
7ae750f 
* Unique device id for trackpad on web

* ++
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moffatman moffatman moffatman left review comments

@yjbanov yjbanov yjbanov approved these changes

@mdebbar mdebbar mdebbar approved these changes

@htoor3 htoor3 htoor3 approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
Milestone
No milestone
5 participants
@Piinks @moffatman @yjbanov @mdebbar @htoor3