[ios][platform_view] Fix Platform view gesture recognizer with iPad pencil getting stuck

[hellohuanlin]

Summary

I came across our the "gesture recognizer delegate" implementation and it is quite odd (see below). After fixing it, the problem is resolved. However, it's hard to reason about how it's related to iPad pencil, since it's internal logic that we don't know (see my research below).

Gesture recognizer delegate

Existing odd implementation

• shouldBeRequiredToFailByGestureRecognizer:

otherGestureRecognizer != self is always YES because the delegate set to self, hence gestureRecognizer must be self, hence otherGestureRecognizer must not be self.

• shouldRequireFailureOfGestureRecognizer:

otherGestureRecognizer == self is always NO, for the same reason described above.

new implementation:

After digging into various PRs, the idea seems to be that we want to have a precedence of "Forwarding recognizer > Delaying recognizer > Other recognizers in platform view".

• shouldBeRequiredToFailByGestureRecognizer:

return otherGestureRecognizer != _forwardingRecognizer means Delaying recognizer needs to be higher precedence than all non-Forwarding recognizer. (aka "Delaying recognizer > Other recognizers in platform view")

• shouldRequireFailureOfGestureRecognizer:

return otherGestureRecognizer == _forwardingRecognizer means Delaying recognizer needs to have lower precedence than forwarding recognizer. (aka "Forwarding recognizer > Delaying recognizer").

Some research

This is a tricky one since pencil and finger triggers exactly the same callbacks. It turns out that when pencil is involved after finger interaction, the platform view's "forwarding" gesture recognizer is stuck at failed state. This seems to be an iOS bug, because according to the API doc, it should be reset back to "possible" state:

No action message is sent and the gesture recognizer is reset to UIGestureRecognizerStatePossible.

However, when iPad pencil is involved, the state is not reset. I tried to KVO the state property, and wasn't able to capture the change. This means the state change very likely happened internally within the recognizer via the backing ivar of the state property.

List which issues are fixed by this PR. You must list at least one issue.

Fixes flutter/flutter#136244

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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.

[hellohuanlin]

Since we set self as the delegate, otherGestureRecognizer != self is always YES.

[hellohuanlin]

I can confirm this fixed the issue, but need to make sure it doesn't break anything

[jmagman]

Logic introduced in #6665
Can you try running this on the framework tests, hopefully the gesture recognizer tests can run on it https://github.com/flutter/engine/blob/main/docs/Testing-presubmit-Engine-PRs-with-the-Flutter-framework.md

[hellohuanlin]

Running the presubmit test now.

I also manually tested a few scenario:

1. Scrollable list of ads are clickable
2. Non scrollable list of ads are clickable
3. Scrollable list of web views are clickable, but not zoomable or scrollable (since flutter's scrolling list deals with the gesture)
4. Non scrollable list of web views are clickable, and also zoomable and scrollable (since flutter's doesn't deal with the gesture, so touch events gets propagated into the web view)
5. A simple platform view of UIButton's callback is still called

Also iPad pencil doesn't break the gestures in above examples.

[hellohuanlin]

Tested the webview package's example app -

• Finger zooming, scrolling, clicking still works.
• Scrolling using iPad pencil also works.
• Zooming with finger + pencil doesn't stuck.

[hellohuanlin]

Tested on google map package's example app (this app is pretty well made and it covers lots of gesture functionalities, so it gives me lots of confidence). I tried all the following features related to touch/gesture:

• Under "user interface" page, i tried disable/enable zooming and scrolling.
• Under "Map click" page, long press and tap gesture works
• Under "Scrolling map" page:
  a. Able to scroll the list by dragging in regions outside of both maps. Tried both finger and pencil.
  b. The first map consumes all touch events. So dragging inside this map won't scroll the list, as expected;
  c. The second map consumes vertical drags but still get other gestures. So dragging inside it scrolls the flutter list as expected; and zooming and tapping on the map still works (able to interact with the map); zooming with finger first and then pencil won't stuck.

[hellohuanlin]

Tried web view example again with the main branch - confirmed same behavior as expected.

[hellohuanlin]

Tried google map example again with the main branch - also same behavior as expected.

[hellohuanlin]

Verified the regression in previous workaround (UIButton's callback not being called) doesn't happen in this PR.

[jmagman]

Do we have gesture integration tests? Can we add something to scenario app to test some of the manual tests you did?

[hellohuanlin]

Do we have gesture integration tests? Can we add something to scenario app to test some of the manual tests you did?

For this particular bug: I don't think it's possible to mimic ipad pencil interaction in XCUITests. I couldn't find any suitable API (e.g. XCUIElement APIs).

The manual tests I did was to ensure we don't regress our general gesture logic (unfortunately they won't help catch this bug). We don't have any of those tests, but I think we should add those to improve our code quality. That would be a pretty big project though, so I'm leaning towards having it as a separate project. I can create a ticket maybe for next year. WDYT? @jmagman

I don't think Scenario test will do - we need the framework part as well (e.g. to check if GestureDetector widget gets the callbacks).

[jmagman]

The manual tests I did was to ensure we don't regress our general gesture logic (unfortunately they won't help catch this bug). We don't have any of those tests, but I think we should add those to improve our code quality. That would be a pretty big project though, so I'm leaning towards having it as a separate project. I can create a ticket maybe for next year. WDYT?

That would be really helpful, thank you!

LGTM let's try it.

[auto-submit]

Time to revert pull request flutter/engine/55724 has elapsed.
You need to open the revert manually and process as a regular pull request.