Add support for mice on iOS 13.4+ #19965

gw280 · 2020-07-22T22:31:02Z

Description

Initial pass at adding support for iOS 13.4+ pointer interactions. This currently supports mouse hover and basic scroll events.

Related Issues

flutter/flutter#55809

flutter-dashboard · 2020-07-22T22:31:05Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

gw280 · 2020-07-22T22:50:11Z

A few things:

This doesn't support inertial scrolling, and I'm going to recommend that's tackled in a future PR.
I think the state machine isn't correctly implemented here for the pointer_data.change state, I'm actively investigating what the remaining cases that need to be addressed are.
When you hit the scroll limits of a scrollable widget, the parent widget starts to scroll. This seems to be the existing behaviour on e.g. macOS desktop, so this seems more like a UX question rather than an engineering question, but the semantics don't really match that of other iPadOS apps that support touchpad/mouse events.
The scrolling isn't done on rails right now - so even if you scroll what you think is just vertically, you'll likely do some horizontal scrolling too as the translation from the UIPanGestureRecognizer is applied in both x and y. I'm thinking about the best way to add a threshold to determine if we only want to scroll in X or Y.

gaaclarke

Noice! Just a few nits. What about adding a test?

gaaclarke · 2020-07-23T17:03:31Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+  // Flutter expects pointers to be added before events are sent for them.
+  bool flutter_state_is_added = false;
+
+  // Current coordinate of the mouse cursor


Are the units logical or real pixels?

gaaclarke · 2020-07-23T17:06:44Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

  // UIScrollView with height zero and a content offset so we can get those events. See also:
  // https://github.com/flutter/flutter/issues/35050
  fml::scoped_nsobject<UIScrollView> _scrollView;
+#ifdef __IPHONE_13_4


These don't have to be ifdef'd. I think it's better to limit the amount of conditional compilation.

gaaclarke · 2020-07-23T17:08:08Z

shell/platform/darwin/ios/framework/Headers/FlutterViewController.h

 * Dart-related state and asynchronous tasks when navigating back and forth between a
 * FlutterViewController and other `UIViewController`s.
 */
+#ifdef __IPHONE_13_4


You shouldn't need this, the presence of UIPointerInteractionDelegate will exist depending on the SDK used for compilation. We don't need to support old versions of Xcode.

gaaclarke · 2020-07-23T17:09:00Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

      << "FlutterViewController's view is loaded but is not attached to a FlutterEngine";
  [_engine.get() attachView];

+#ifdef __IPHONE_13_4


I'd also remove this preprocessor guard.

gaaclarke · 2020-07-23T17:10:02Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

  return self.presentedViewController != nil || self.isPresentingViewControllerAnimating;
 }

+#ifdef __IPHONE_13_4


gaaclarke · 2020-07-23T17:12:22Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+    auto packet = std::make_unique<flutter::PointerDataPacket>(1);
+    _mouseState.location = request.location;
+
+    flutter::PointerData pointer_data = [self generatePointerDataForMouse];


nit: A static C function for generatePointerDataForMouse would execute faster because of dynamic dispatch.

gaaclarke · 2020-07-23T17:13:18Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+    flutter::PointerData pointer_data = [self generatePointerDataForMouse];
+
+    pointer_data.signal_kind = flutter::PointerData::SignalKind::kNone;
+    packet->SetPointerData(0, pointer_data);


Please annotate what "0" means by pulling it into a variable or with arg comments.

gaaclarke · 2020-07-23T17:20:35Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+
+  flutter::PointerData pointer_data = [self generatePointerDataForMouse];
+  pointer_data.signal_kind = flutter::PointerData::SignalKind::kScroll;
+  pointer_data.scroll_delta_x = (translation.x - _mouseState.last_translation.x) * scale;


super nit: you are scaling these values twice, once when you receive them and once when they are used as "last_translation". You could avoid the multiplication by storing the scaled value.

gw280 · 2020-07-31T23:35:43Z

@chinmaygarde following up from your question here (for others: what specifically I'd like looked at for review):

I'm not sure how the event pipeline works beyond submitting them via dispatchPointerDataPacket on the Engine. I'm wondering whether I should be doing some batching locally before sending them, or is the current method of generating a single event packet for each incoming cursor event ok? It seems a little heavyweight to me to be building a PointerDataPacket object each time that only contains a single packet.
There are several fields in PointerData (see https://api.flutter.dev/flutter/dart-ui/PointerData-class.html) that I've not populated when generating the mouse events. I've mostly copied the fields to populate by using the Windows port as a reference (see https://github.com/flutter/engine/blob/master/shell/platform/windows/flutter_windows_view.cc#L250). The only thing I haven't added which is in the Windows port is the timestamp field, which I assume will be needed.
I've decided not to bother dealing with button logic for the time being as button events are dispatched as UITouch events through the existing touchesBegan, so they mostly work right now. In the future we will likely want to differentiate between mouse clicks and screen touches. Does this sound reasonable?

chinmaygarde · 2020-09-17T21:48:54Z

cc @gw280: Just going through old PR and this one looks stale. Feel free to re-open if I am mistaken.

alexmarkley · 2020-09-21T12:57:25Z

Hey @gw280 any chance you want to take another pass at this? I think the priority on this is going to rapidly escalate at the end of this year: flutter/flutter#54663

dupuchba · 2024-03-21T09:32:01Z

hey @gw280 , thanks for working on this feature.
I'd like to know how can I help with this feature and maybe why is it stale (technical issues, time related..)
I think this could unlock the "shape shifting" pointer when using the iPad with a mouse (maybe even tracking for vision pro)

Might be related flutter/flutter#129640

gw280 requested review from chinmaygarde and gaaclarke July 22, 2020 22:31

flutter-dashboard bot added the platform-ios label Jul 22, 2020

googlebot added the cla: yes label Jul 22, 2020

auto-assign bot requested a review from liyuqian July 22, 2020 22:33

Add support for mice on iOS 13.4+

b53a90c

gw280 force-pushed the gwright-ios-mouse branch from 0c96611 to b53a90c Compare July 22, 2020 22:36

gaaclarke reviewed Jul 23, 2020

View reviewed changes

chinmaygarde closed this Sep 17, 2020

Kavantix mentioned this pull request Sep 20, 2020

iOS scroll wheel support flutter/flutter#54663

Closed

gw280 mentioned this pull request Dec 31, 2020

Support Mice in iPadOS 13.4+ #23362

Merged

13 tasks

Add support for mice on iOS 13.4+ #19965

Add support for mice on iOS 13.4+ #19965

Uh oh!

Conversation

gw280 commented Jul 22, 2020

Description

Related Issues

Uh oh!

flutter-dashboard bot commented Jul 22, 2020

Uh oh!

gw280 commented Jul 22, 2020

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gw280 commented Jul 31, 2020

Uh oh!

chinmaygarde commented Sep 17, 2020

Uh oh!

alexmarkley commented Sep 21, 2020

Uh oh!

dupuchba commented Mar 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants