iPad trackpad gestures #31591

moffatman · 2022-02-21T21:04:36Z

Start sending PointerPanZoom events from iPadOS when the trackpad is used

Fixes flutter/flutter#23604

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.

chinmaygarde · 2022-03-10T21:15:17Z

cc @jmagman @gaaclarke

jmagman

@cyanglaz if you have bandwidth can you take a look at the logic here and validate it makes sense?

jmagman · 2022-03-18T17:55:43Z

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

  fml::scoped_nsobject<UIScrollView> _scrollView;
  fml::scoped_nsobject<UIHoverGestureRecognizer> _hoverGestureRecognizer API_AVAILABLE(ios(13.4));
-  fml::scoped_nsobject<UIPanGestureRecognizer> _panGestureRecognizer API_AVAILABLE(ios(13.4));
+  fml::scoped_nsobject<UIPanGestureRecognizer> _discretePanGestureRecognizer


I know you're copying the _panGestureRecognizer pattern, but I'm not sure why it's a scoped_nsobject other than someone didn't want to figure out the MRC memory management pattern. Can you try re-writing this with the new ivars as vanilla Objective-C objects?

I don't really know what I'm doing with Objective-C, can you verify what I changed it to is what you are meaning here?

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

cyanglaz · 2022-03-21T17:58:10Z

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

  packet->SetPointerData(/*index=*/0, pointer_data);

  [_engine.get() dispatchPointerDataPacket:std::move(packet)];
+  _mouseState.flutter_state_is_added = pointer_data.change != flutter::PointerData::Change::kRemove;


I'm un-sure about this, it looks fine.

CC @chunhtai do you mind take a look at the logics about pointers in this PR?

This seems to assume the engine shell will synthesize add event when the change is not kRemove? why not always synthesize the add event in embedding so that you don't need to guess what engine shell may be doing

The correct events will be sent to the shell. Nothing needs to be synthesized. This is to synchronize so that in hoverEvent and scrollEvent we don't send the same event twice.

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

chunhtai · 2022-03-22T17:26:47Z

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

  packet->SetPointerData(/*index=*/0, pointer_data);

  [_engine.get() dispatchPointerDataPacket:std::move(packet)];
+  _mouseState.flutter_state_is_added = pointer_data.change != flutter::PointerData::Change::kRemove;


This seems to assume the engine shell will synthesize add event when the change is not kRemove? why not always synthesize the add event in embedding so that you don't need to guess what engine shell may be doing

chunhtai · 2022-03-22T17:28:03Z

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


    packet->SetPointerData(pointer_index++, pointer_data);
+
+    if (touch.phase == UITouchPhaseEnded || touch.phase == UITouchPhaseCancelled) {


does this need to check flutter_state_is_added?

Engine shell does not need to synthesize. The event will be kAdd as needed based on state of the gesture recognizer (see generatePointerDataForMouse). Use of flutter_state_is_added is only to synchronize between hover recognizer and scroll recognizer.

flutter_state_is_added is only for the mouse cursor (hovering only). The quoted code is for each UITouch (it's because UIKit uses touch mechanism to send mouse clicks).

chunhtai · 2022-03-22T17:28:15Z

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

+  // only does this hover logic when device kind is flutter::PointerData::DeviceKind::kMouse.
+  size_t touches_to_remove_count = 0;
+  for (UITouch* touch in touches) {
+    if (touch.phase == UITouchPhaseEnded || touch.phase == UITouchPhaseCancelled) {


same here check for flutter_state_is_added?

See above comment, flutter_state_is_added is only for the mouse hover cursor.

zanderso · 2022-03-31T20:25:07Z

From PR review triage: It looks like @moffatman has responded to feedback. @chunhtai @cyanglaz could you give this another look please?

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

cyanglaz · 2022-04-01T17:38:17Z

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

+    flutter::PointerData add_pointer_data = pointer_data;
+    add_pointer_data.signal_kind = flutter::PointerData::SignalKind::kNone;
+    add_pointer_data.change = flutter::PointerData::Change::kAdd;
+    _mouseState.flutter_state_is_added = true;
+    packet->SetPointerData(/*index=*/0, add_pointer_data);
+    packet->SetPointerData(/*index=*/1, pointer_data);


I assume this is manually adding a "add" event. Is this something we normally do? Is there a side effect for this?
@chunhtai might know.

this looks correct

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

cyanglaz · 2022-04-01T17:42:48Z

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

+    pointer_data.pan_delta_y = 0;  // Delta will be generated in pointer_data_packet_converter.cc.
+    pointer_data.scale = 1;
+  } else {
+    pointer_data.change = flutter::PointerData::Change::kPanZoomEnd;


How about other states like cancel or failed? I looks like we do have a kCancel in PointerData:Change
I'm not familiar with the PointerData, @chunhtai might be able to confirm if other UIGestureRecognizerState needs to be mapped to different PointerData:Change

For trackpad gesture we will only have kPanZoomStart, kPanZoomUpdate, kPanZoomEnd. I didn't make a separate PanZoomCancelled since I didn't find any situation where it would be used differently than PanZoomEnd in the framework.

sounds good. cc @chunhtai to confirm as he is more knowledgable on gestures in flutter.

we should probably still use cancel just to be safe, also can this be a switch statements and put assert on the case that are unexpected?

I updated to use switch so it's more obvious what UIKit phase maps to which flutter phase.

cyanglaz · 2022-04-01T17:44:00Z

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

+    pointer_data.scale = recognizer.scale;
+    pointer_data.rotation = _rotationGestureRecognizer.rotation;
+  } else {
+    pointer_data.change = flutter::PointerData::Change::kPanZoomEnd;


Same comment as panEvent

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

zanderso · 2022-04-07T20:24:12Z

What is the status of this PR?

moffatman · 2022-04-07T21:04:52Z

I think I should request re-review, will do that now

jmagman · 2022-04-19T20:50:14Z

Friendly ping @cyanglaz

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

cyanglaz · 2022-04-19T20:56:34Z

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

+    pointer_data.pan_delta_y = 0;  // Delta will be generated in pointer_data_packet_converter.cc.
+    pointer_data.scale = 1;
+  } else {
+    pointer_data.change = flutter::PointerData::Change::kPanZoomEnd;


sounds good. cc @chunhtai to confirm as he is more knowledgable on gestures in flutter.

cyanglaz · 2022-04-19T21:02:23Z

testing/scenario_app/ios/Scenarios/ScenariosUITests/iPadGestureTests.m

+  [invocation setArgument:&deltaY atIndex:3];
+  [invocation invokeWithTarget:flutterView];
+
+  // The hover pointer is removed after ~3.5 seconds, this ensures that all events are received


Where is the 3.5 seconds defined?

It is the observed behavior on iPadOS. After around 3.5 seconds of not moving the mouse, the system will remove it.

This might be a risk of flaky test. It is possible to find a different way to test this?

We could end the test without checking that the pointer is removed after a delay, since it's just an observed behaviour of the iPad, you're right that it could change with any update. The rest of the test can stay.

I think the better question to ask is whether our implementation relies on this behavior, such as relying on this to send the pointer removal. If so we should keep this test, explain why it's important, and note that this behavior is volatile.

zanderso · 2022-04-28T20:10:38Z

From PR triage: @cyanglaz it looks like this one is ready for another look.

jmagman · 2022-05-09T21:41:13Z

This is on @cyanglaz's radar, he will test locally on his iPad to verify when he gets a chance. Thanks for your patience! 🙂

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

dkwingsmt

Minor changes. I see that it's really hard to test! Great job.

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

testing/scenario_app/ios/Scenarios/ScenariosUITests/iPadGestureTests.m

dkwingsmt · 2022-05-11T09:59:39Z

testing/scenario_app/ios/Scenarios/ScenariosUITests/iPadGestureTests.m

+  [invocation setArgument:&deltaY atIndex:3];
+  [invocation invokeWithTarget:flutterView];
+
+  // The hover pointer is removed after ~3.5 seconds, this ensures that all events are received


I think the better question to ask is whether our implementation relies on this behavior, such as relying on this to send the pointer removal. If so we should keep this test, explain why it's important, and note that this behavior is volatile.

testing/scenario_app/ios/Scenarios/ScenariosUITests/iPadGestureTests.m

chinmaygarde · 2022-05-12T20:18:16Z

@dkwingsmt I think this is good for another review.

dkwingsmt

LGTM, thanks for taking care all of them!

moffatman · 2022-06-07T03:19:05Z

I realized if we only send discrete scroll events on a pointer, it doesn't need to be removed, since it won't ever trigger any hover events. So the code is simpler now, no need to coordinate the reuse of the hover pointer. Also, I will now rely on kAdd being synthesized for the hover pointer in pointer_data_packet_converter.cc, it's already the practice for other pointer identifiers.

@hellohuanlin: In regards to pointer arena, we still need to use the pointer arena within flutter, as even though we know some of its intent, we need to determine which widget will receive the trackpad gesture. For example: nested pannable/scrollable widgets.

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

Clicking-and-dragging with a mouse was causing erroneous scroll data to be generated.

hellohuanlin · 2022-06-09T21:43:08Z

@moffatman i saw you pushed multiple updates. is this PR ready for review again?

moffatman · 2022-06-09T21:49:49Z

@hellohuanlin Yes, please take a look again. I added the "Add" event synthesis so that all cases are consistent.

hellohuanlin

This is looking great. Thanks for your effort addressing feedbacks.

hellohuanlin · 2022-06-09T22:08:04Z

lib/ui/window/pointer_data_packet_converter.cc

+          synthesized_add_event.synthesized = 1;
+          synthesized_add_event.buttons = 0;
+          state = EnsurePointerState(synthesized_add_event);
+          converted_pointers.push_back(synthesized_add_event);


nice! thanks for moving this into the converter

hellohuanlin · 2022-06-09T22:10:29Z

testing/scenario_app/ios/Scenarios/ScenariosUITests/iPadGestureTests.m

+    supportsPointerInteraction =
+        performBoolSelector(XCUIDevice.sharedDevice, supportsPointerInteractionSelector);
+  }
+  XCTSkipUnless(supportsPointerInteraction, "Device does not support pointer interaction.");


do you know if any of our CI test devices supports pointer interaction?

This testing support depends on iOS 15 anyway, so I don't think it's run yet. Right now I believe the simulator in CI is iPhone 8 on iOS 13. We need iPad on iOS15+.

@jmagman probably knows who can help with setting up test devices for iOS15

We can do this in the simulator, we shouldn't need physical devices, like this:

engine/testing/scenario_app/run_ios_tests.sh

Line 43 in 92ad24a

-destination 'platform=iOS Simulator,OS=13.0,name=iPhone 8' \

We need to get flutter/flutter#85558 done so these tests actually run 🙂
It's fine to just match the pattern of the tests around it for now, as long as it passes for you locally.

testing/scenario_app/ios/Scenarios/ScenariosUITests/iPadGestureTests.m

dkwingsmt · 2022-06-14T00:39:54Z

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

+    _discreteScrollingPanGestureRecognizer =
+        [[UIPanGestureRecognizer alloc] initWithTarget:self action:@selector(discreteScrollEvent:)];
+    _discreteScrollingPanGestureRecognizer.allowedScrollTypesMask = UIScrollTypeMaskDiscrete;
+    _discreteScrollingPanGestureRecognizer.allowedTouchTypes = @[];


Can you add your explanation here as comments? I've copied it as below. Modify as you see fit.

Disallow all touch types. When using trackpad scroll, the platform doesn't send touch events, instead it sends scroll events, so disallowing touch types doesn't stop the trackpad callback. If touch events are allowed, touching the screen will be consumed by the pan gesture recognizer instead of being passed through to flutter via touchesBegan etc.

dkwingsmt · 2022-06-14T00:55:11Z

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

    return;
  }

+  // When UIApplicationSupportsIndirectInputEvents is set in Info.plist, mouse clicks will be


Do we ever unset UIApplicationSupportsIndirectInputEvents? If not we might change the "if" to a more decisive word. Also here's a my two cents of entire paragraph. Take as you see fit.

With UIApplicationSupportsIndirectInputEvents set in Info.plist, the platform dispatches mouse clicks as UITouch with touch.type of UITouchTypeIndirectPointer and a different identifier for each click. They are translated into Flutter pointer events with type of kMouse and different device IDs. These devices must be terminated with kRemove events then the touches end, otherwise they will keep triggering hover events.

The touches_to_remove_count tracks how many removal events are needed in this group of touches to properly allocate space for the packet. The removal event of a touch is synthesized immediately after its normal event.

I wrote it that way since UIApplicationSupportsIndirectInputEvents is false by default, I need to make a PR to migrate it to true. But whether that key is true or false, each mouse click will always have a unique identifier, since it comes in as a UITouch. An accommodation is needed since mouse events with device kind touch don't trigger MouseRegion / mouse tracker and there are no side effects from creating a lot of single-use devices in the framework.

Ok, firstly, what exactly is the "mouse device" you're talking about? (It's mostly for the next question)

Secondly, do you mean that the behavior as observed in Flutter will vary based on UIApplicationSupportsIndirectInputEvents, so that if it's false the resulting events will have type touch, and if it's true the events will have type mouse?

When I say mouse device, I guess I mean a sequence of pointer events with a unique identifier and device-kind kMouse. Those sequences have different side effects in the framework than sequences with a unique identifier but device-kind kTouch.

For point 2, yes, that's correct, the event type will change depending on UIApplicationSupportsIndirectInputEvents, as well as a few other things:

I see. I've also found https://developer.apple.com/documentation/uikit/pointer_interactions, which explains that an indirect pointer stands for the trackpad, while direct pointer stands for finger touches or pencils. We probably want to document this behavioral difference in the framework.

Then for the documentation, how about this:

If the UIApplicationSupportsIndirectInputEvents in Info.plist returns YES, then the platform dispatches indirect pointer touches (trackpad clicks) as UITouch with a type of UITouchTypeIndirectPointer and different identifiers for each click. They are translated into Flutter pointer events with type of kMouse and different device IDs. These devices must be terminated with kRemove events when the touches end, otherwise they will keep triggering hover events.

If the UIApplicationSupportsIndirectInputEvents in Info.plist returns NO, then the platform dispatches indirect pointer touches (trackpad clicks) as UITouch with a type of UITouchTypeIndirectPointer and different identifiers for each click. They are translated into Flutter pointer events with type of kTouch and different device IDs. Removing these devices is neither necessary nor harmful.

Therefore Flutter always removes these devices. The touches_to_remove_count tracks how many remove events are needed in this group of touches to properly allocate space for the packet. The remove event of a touch is synthesized immediately after its normal event.

See also:

https://developer.apple.com/documentation/uikit/pointer_interactions?language=objc

https://developer.apple.com/documentation/bundleresources/information_property_list/uiapplicationsupportsindirectinputevents?language=objc

Great summarization, I'll put it in.

dkwingsmt

LGTM. Doc suggestion.

flutter-dashboard bot added the platform-ios label Feb 21, 2022

jmagman reviewed Mar 18, 2022

View reviewed changes

cyanglaz reviewed Mar 21, 2022

View reviewed changes

chunhtai reviewed Mar 22, 2022

View reviewed changes

moffatman force-pushed the trackpad_gestures_ipad branch from beb0e0d to 13f6de5 Compare March 22, 2022 23:24

moffatman requested review from chunhtai and jmagman March 29, 2022 17:23

cyanglaz reviewed Apr 1, 2022

View reviewed changes

moffatman force-pushed the trackpad_gestures_ipad branch 3 times, most recently from b70df69 to 4eb5e62 Compare April 6, 2022 00:51

moffatman requested a review from cyanglaz April 7, 2022 21:04

cyanglaz reviewed Apr 19, 2022

View reviewed changes

moffatman force-pushed the trackpad_gestures_ipad branch from 4eb5e62 to bf1bb56 Compare April 19, 2022 23:25

dkwingsmt reviewed May 11, 2022

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm Outdated Show resolved Hide resolved

dkwingsmt reviewed May 11, 2022

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm Outdated Show resolved Hide resolved

dkwingsmt suggested changes May 11, 2022

View reviewed changes

moffatman force-pushed the trackpad_gestures_ipad branch 2 times, most recently from 5514101 to e7a31aa Compare May 11, 2022 19:12

dkwingsmt reviewed May 11, 2022

View reviewed changes

testing/scenario_app/ios/Scenarios/ScenariosUITests/iPadGestureTests.m Outdated Show resolved Hide resolved

dkwingsmt approved these changes May 13, 2022

View reviewed changes

dkwingsmt requested a review from cyanglaz May 13, 2022 11:24

moffatman force-pushed the trackpad_gestures_ipad branch from 5837d01 to 385061a Compare June 7, 2022 03:40

hellohuanlin reviewed Jun 7, 2022

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm Outdated Show resolved Hide resolved

moffatman force-pushed the trackpad_gestures_ipad branch 2 times, most recently from 787966b to 80cb056 Compare June 8, 2022 02:36

moffatman added 7 commits June 7, 2022 23:05

iPad trackpad gestures

3b2c6f7

Address feedback

b15181f

Feedback 2

210d20b

Feedback 3

aaebdec

Don't allow touches on scroll recognizers

58a5b47

Clicking-and-dragging with a mouse was causing erroneous scroll data to be generated.

Feedback 4

bb9e503

Feedback 5

e0364e4

moffatman force-pushed the trackpad_gestures_ipad branch from 80cb056 to bbb6f80 Compare June 8, 2022 03:05

Feedback 6

0b09015

moffatman force-pushed the trackpad_gestures_ipad branch from bbb6f80 to 0b09015 Compare June 8, 2022 14:22

moffatman requested a review from dkwingsmt June 9, 2022 21:49

hellohuanlin approved these changes Jun 9, 2022

View reviewed changes

Feedback 7

ff6c733

dkwingsmt reviewed Jun 14, 2022

View reviewed changes

dkwingsmt approved these changes Jun 14, 2022

View reviewed changes

Feedback 8

579add7

dkwingsmt added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jun 14, 2022

fluttergithubbot merged commit 3a195fd into flutter:main Jun 14, 2022

engine-flutter-autoroll mentioned this pull request Jun 14, 2022

Roll Flutter Engine from edc2aa4e894b to 5ad8879327be (23 revisions) flutter/flutter#105978

Merged

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

3a195fd iPad trackpad gestures (flutter/engine#31591)

8cf4b2e

houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022

iPad trackpad gestures (flutter#31591)

51e9edd

knopp mentioned this pull request Mar 9, 2023

Fix clicking on magic mouse while pan is in progress flutter/flutter#122014

Closed

8 tasks


		packet->SetPointerData(pointer_index++, pointer_data);

		if (touch.phase == UITouchPhaseEnded \|\| touch.phase == UITouchPhaseCancelled) {

iPad trackpad gestures #31591

iPad trackpad gestures #31591

Uh oh!

Conversation

moffatman commented Feb 21, 2022

Pre-launch Checklist

Uh oh!

chinmaygarde commented Mar 10, 2022

Uh oh!

jmagman 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!

Uh oh!

Uh oh!

Uh oh!

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!

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!

zanderso commented Mar 31, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

zanderso commented Apr 7, 2022

Uh oh!

moffatman commented Apr 7, 2022

Uh oh!

jmagman commented Apr 19, 2022

Uh oh!

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!

zanderso commented Apr 28, 2022

Uh oh!

jmagman commented May 9, 2022