PlatformViewsController always make sure the touch events are finished #22406

cyanglaz · 2020-11-09T20:47:03Z

Description

Regression: After #20708, touch events might not able to finish if FlutterViewController is unset in the middle of the touch events. This PR asks the ForwardGestureRecognizer to hold a reference to the FlutterViewController at least until the end of the touch events.

Related Issues

Fixes flutter/flutter#66097

Tests

testSetFlutterViewControllerToNilInTheMiddleOfTouchEventShouldStillAllowGesturesToBeHandled

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

I have submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

dnfield · 2020-11-09T21:08:41Z

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

+  if (!_flutterViewController) {
+    return;
+  }


nit: I think these aren't necessary, since sending a message to nil is a noop.

dnfield · 2020-11-09T21:09:20Z

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

+    }
+  }
+  [_flutterViewController touchesBegan:touches withEvent:event];
  _currentTouchPointersCount += touches.count;


What's the implication of not incrementing this counter if we have no VC?

Should we be setting to zero in the early return above?

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

cyanglaz

@dnfield @gaaclarke Updated per your comments. PTAL

gaaclarke · 2020-11-10T00:50:02Z

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


 - (void)touchesBegan:(NSSet*)touches withEvent:(UIEvent*)event {
-  [_platformViewsController->getFlutterViewController() touchesBegan:touches withEvent:event];
+  _flutterViewController = _platformViewsController->getFlutterViewController();


Is it possible that _platformViewsController->getFlutterViewController() could return different values between calls touchesBegan before touchesEnded has happened?

touch-1 began

flutter view controller changed

touch-2 began

touch-1 ended

touch-2 ended

If so you'd have to be super vigilant and track the FlutterViewController at touchesBegan for each event.

Has this been addressed in the recent commit(s)?

Updated the code to support this scenario. I also added a test for this.

…quence

cyanglaz · 2020-11-11T17:44:34Z

testing/ios/IosUnitTests/IosUnitTests.xcodeproj/project.pbxproj

 		0D6AB6C922BB05E200EEE540 /* IosUnitTestsTests.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = IosUnitTestsTests.xctest; sourceTree = BUILT_PRODUCTS_DIR; };
 		0D6AB6CF22BB05E200EEE540 /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = "<group>"; };
 		0D6AB73E22BD8F0200EEE540 /* FlutterEngineConfig.xcconfig */ = {isa = PBXFileReference; lastKnownFileType = text.xcconfig; path = FlutterEngineConfig.xcconfig; sourceTree = "<group>"; };
+		68FF0A70255A3B4E00165B60 /* FlutterPlatformViewsTest.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterPlatformViewsTest.mm; sourceTree = "<group>"; };


I found that this test file is not referenced by the project, so while I'm here, I added it to the project so it helps the debugging process easier.

It isn't suppose to be in here. That is linked in as part of ios_test_flutter_mrc

engine/shell/platform/darwin/ios/BUILD.gn

Line 180 in 5d4d06e

"framework/Source/FlutterPlatformViewsTest.mm",

You'll get duplicate symbols if you do this.

You still need to remove this.

gaaclarke · 2020-11-11T19:23:59Z

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

+  if (_currentTouchPointersCount == 0) {
+    // At the start of each gesture sequence, we reset the `_flutterViewController`,
+    // so that all the touch events in the same sequence are forwarded to the same `_flutterViewController`.
+    _flutterViewController = _platformViewsController->getFlutterViewController();
+  }
+  [_flutterViewController touchesBegan:touches withEvent:event];


This still makes me a bit nervous. Particularly the fact that _flutterViewController is a weak reference and we are sending messages to it potentially after _platformViewsController has gotten rid of it. My suggestion was to keep a map between events and FlutterViewControllers.

- (void)touchesBegan:(NSSet*)touches withEvent:(UIEvent*)event { _flutterViewControllers[event] = _platformViewsController->getFlutterViewController(); [_flutterViewControllers[event] touchesBegan:touches withEvent:event]; } -(void)touchesEnded:(NSSet*)touches withEvent:(UIEvent*)event { [_flutterViewControllers[event] touchesEnded:touches withEvent:event]; [_flutterViewControllers removeObject:event]; }

This has the added benefit of having a retain on the flutter view controller while it is being interacted with and everyone gets the correct message. You'll have to verify that the same event is getting passed across all of the calls.

gaaclarke · 2020-11-12T00:57:05Z

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

+  [_flutterViewController.get() touchesCancelled:touches withEvent:event];
+  _currentTouchPointersCount -= touches.count;
+  if (_currentTouchPointersCount == 0) {
+    self.state = UIGestureRecognizerStateFailed;
+    _flutterViewController.reset(nil);
+  }


In the case where we get Begin-Begin-Cancel-End the state will no longer be Failed like it was previously, is that expected?

Begin-Begin-Cancel-End makes the count to 0 at the last End, so it state would still be failed.
I don't think Begin-Begin-Cancel-End would be possible anyway, if a Cancel happens during a sequence, it cancels all the touches had begun.

gaaclarke · 2020-11-12T00:57:45Z

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

+  if (_currentTouchPointersCount < 0) {
+    // _currentTouchPointersCount should never be less than 0.
+    // To not breaking production code, we reset the count to 0 if a negative value
+    // is reached.
+    _currentTouchPointersCount = 0;
+  }


This shouldn't be possible now that you've changed the cancel logic, right?

Right, removed

gaaclarke

LGTM once the PlatformViewsTest is removed from the project. There are still some things I'm not clear about but comparing this code to what was before it, this is an improvement and a fix. Thanks chris.

gaaclarke · 2020-11-13T17:51:46Z

testing/ios/IosUnitTests/IosUnitTests.xcodeproj/project.pbxproj

 		0D6AB6C922BB05E200EEE540 /* IosUnitTestsTests.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = IosUnitTestsTests.xctest; sourceTree = BUILT_PRODUCTS_DIR; };
 		0D6AB6CF22BB05E200EEE540 /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = "<group>"; };
 		0D6AB73E22BD8F0200EEE540 /* FlutterEngineConfig.xcconfig */ = {isa = PBXFileReference; lastKnownFileType = text.xcconfig; path = FlutterEngineConfig.xcconfig; sourceTree = "<group>"; };
+		68FF0A70255A3B4E00165B60 /* FlutterPlatformViewsTest.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterPlatformViewsTest.mm; sourceTree = "<group>"; };


You still need to remove this.

cyanglaz · 2020-11-13T18:01:22Z

@gaaclarke Removed the reference.

@dnfield PTAL

… finished (flutter/engine#22406)

flutter#22406)

Chris Yang added 3 commits November 9, 2020 12:42

Let touch events go through

87ba06a

format

dd80223

Merge branch 'master' into gesture

183fc62

flutter-dashboard bot added the platform-ios label Nov 9, 2020

googlebot added the cla: yes label Nov 9, 2020

google-cla bot added the cla: yes label Nov 9, 2020

cyanglaz requested review from amirh, dnfield and gaaclarke November 9, 2020 20:47

dnfield reviewed Nov 9, 2020

View reviewed changes

gaaclarke reviewed Nov 9, 2020

View reviewed changes

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

review

c1529b3

cyanglaz commented Nov 10, 2020

View reviewed changes

gaaclarke reviewed Nov 10, 2020

View reviewed changes

only update flutter view controller if the touchBegan starts a new se…

d6433a3

…quence

cyanglaz commented Nov 11, 2020

View reviewed changes

add comments

48472f4

gaaclarke reviewed Nov 11, 2020

View reviewed changes

Chris Yang added 3 commits November 11, 2020 16:51

review

366196d

review

b096e04

format

fc7d934

gaaclarke reviewed Nov 12, 2020

View reviewed changes

review

b349a69

gaaclarke reviewed Nov 13, 2020

View reviewed changes

remove platformviewstest reference in xcode

9ee26e3

dnfield approved these changes Nov 13, 2020

View reviewed changes

Chris Yang added 3 commits November 13, 2020 13:49

Merge branch 'master' into gesture

6f067c2

rebase test code

f93bc21

fix the previous rebase

bca3d18

cyanglaz 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 Nov 13, 2020

fluttergithubbot merged commit 92e5a95 into flutter:master Nov 13, 2020

engine-flutter-autoroll mentioned this pull request Nov 14, 2020

Roll Engine from e06f10638598 to 92e5a95a14cd (7 revisions) flutter/flutter#70512

Closed

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

92e5a95 PlatformViewsController always make sure the touch events are…

cc458a7

… finished (flutter/engine#22406)

engine-flutter-autoroll mentioned this pull request Nov 14, 2020

Roll Engine from e06f10638598 to e4e2dc6f673b (8 revisions) flutter/flutter#70517

Closed

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

92e5a95 PlatformViewsController always make sure the touch events are…

f85e122

… finished (flutter/engine#22406)

engine-flutter-autoroll mentioned this pull request Nov 14, 2020

Roll Engine from e06f10638598 to 98628c3a098c (9 revisions) flutter/flutter#70519

Closed

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

92e5a95 PlatformViewsController always make sure the touch events are…

3fd74b3

… finished (flutter/engine#22406)

engine-flutter-autoroll mentioned this pull request Nov 14, 2020

Roll Engine from e06f10638598 to 713600985f95 (11 revisions) flutter/flutter#70524

Closed

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

92e5a95 PlatformViewsController always make sure the touch events are…

2e0f72d

… finished (flutter/engine#22406)

engine-flutter-autoroll mentioned this pull request Nov 14, 2020

Roll Engine from e06f10638598 to b0966e8123f6 (12 revisions) flutter/flutter#70530

Closed

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

92e5a95 PlatformViewsController always make sure the touch events are…

91bc536

… finished (flutter/engine#22406)

engine-flutter-autoroll mentioned this pull request Nov 14, 2020

Roll Engine from e06f10638598 to 6ea505eac428 (13 revisions) flutter/flutter#70532

Closed

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

92e5a95 PlatformViewsController always make sure the touch events are…

fe108b9

… finished (flutter/engine#22406)

engine-flutter-autoroll mentioned this pull request Nov 14, 2020

Roll Engine from e06f10638598 to b673845877b3 (14 revisions) flutter/flutter#70537

Closed

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

92e5a95 PlatformViewsController always make sure the touch events are…

e2a3038

… finished (flutter/engine#22406)

engine-flutter-autoroll mentioned this pull request Nov 14, 2020

Roll Engine from e06f10638598 to 5a35faf4341e (15 revisions) flutter/flutter#70538

Closed

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

92e5a95 PlatformViewsController always make sure the touch events are…

afce7c3

… finished (flutter/engine#22406)

engine-flutter-autoroll mentioned this pull request Nov 14, 2020

Roll Engine from e06f10638598 to e91aa1111d33 (16 revisions) flutter/flutter#70540

Closed

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

92e5a95 PlatformViewsController always make sure the touch events are…

d7cefad

… finished (flutter/engine#22406)

engine-flutter-autoroll mentioned this pull request Nov 15, 2020

Roll Engine from e06f10638598 to 3e77d854a7cb (18 revisions) flutter/flutter#70554

Merged

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

92e5a95 PlatformViewsController always make sure the touch events are…

97e4ac9

… finished (flutter/engine#22406)

cyanglaz deleted the gesture branch November 16, 2020 17:25

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

PlatformViewsController always make sure the touch events are finished (

173e72a

flutter#22406)

PlatformViewsController always make sure the touch events are finished #22406

PlatformViewsController always make sure the touch events are finished #22406

Uh oh!

Conversation

cyanglaz commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

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!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyanglaz commented Nov 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cyanglaz commented Nov 9, 2020 •

edited

Loading