Fix iOS key events in platform views

[KammererTob]

This PR fixes key events not getting passed through to platform views in iOS (regression introduced here: #20972). The only change made was calling the super method in the event handlers.

I haven't added any tests because i was unsure where to put them. So maybe someone more knowledgeable can help here.

flutter/flutter#74044

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.
• The reviewer has submitted any presubmit flakes in this PR using the [engine presubmit flakes form] before re-triggering the failure.

[flutter-dashboard]

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.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

[chinmaygarde]

The UIResponder documentation does state the following:

"When creating your own subclasses, call super to forward any events that you do not handle yourself."

So the patch is sound. I am not sure if Flutter has affordances to consume the key event and prevent it from propagation. To answer that and guidance on testing cc @gaaclarke

[gaaclarke]

"When creating your own subclasses, call super to forward any events that you do not handle yourself."

@chinmaygarde My reading of this is that you should be calling super conditionally, only if you do not handle it yourself. dispatchPresses: doesn't seem to report if it handled the event or not, so we can't know if we should forward it to super. cc @cbracken since he knows about keyboard event handling.

I haven't added any tests because i was unsure where to put them.

Here is where you can add unit tests: https://github.com/flutter/engine/blob/master/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm

I have concerns about the ramifications of sending the events to super though. I think you could end up with certain events getting handled twice and an integration test would be better. I wouldn't be opposed if you found a way to do that efficiently in FlutterViewControllerTest.mm, the more proper option is to add it to scenario_app tests which is our official integration / golden image test runner. You can find that here: https://github.com/flutter/engine/blob/master/testing/scenario_app/ios/Scenarios/ScenariosTests/FlutterViewControllerTest.m

[KammererTob]

The assumption for this PR was the following (just for clarification): The current implementation (with this PR merged) restores the original flow of the events (with the addition of making the RawKeyboard implementation on the framework side work for iOS). Before the event handlers were added in the first PR all events would also have been passed up the responder chain. From my understanding the dispatchPresses message is not handling any key events on the engine side, but only sends them to the framework. There, the messages are only used for RawKeyboard (at least as far as i can see), which was non-functional for iOS before the first PR. So in my mind there should not be any double event handling. But i agree that an integration test would probably make sense to be sure.

[chinmaygarde]

Fair point. And I don't think there is a way for the framework to intervene on event propagation on the platform thread anyway. In the sprit of restoring the old behavior, I think this approach is fine. Any updates on the integration test per Aarons last comment?

[KammererTob]

I will try to get to it this weekend or early next week.

[KammererTob]

I might have encountered a potential issue. I am not that expierenced when it comes to iOS SDKs, but it looks like the integration tests are using an iOS 13.0 Simulator (which is the metal SDK baseline for the simulator i believe). The key events were only added in 13.4, so the test probably also need to run at least on 13.4. Is my assumption wrong, and if not is there something i can/should do?

[chinmaygarde]

@gaaclarke @cbracken Should we have a chat about what the behavior ought to be? I feel like we haven't answered that question yet.

[chinmaygarde]

May I add a cal entry for a face to face sync on this? I feel like we are a bit directionless here.

[chinmaygarde]

We were trying to reason about the side effects of this patch in a quick conversation. I think this should land for the following reasons:

• This is a regression introduced after #20972.
• This won't mess with existing touch interceptors in Flutter because the those views are children in the touch responder chain.

Sorry it took a while to sort this out as I wasn't sure about the direction here. Testing this is going to be hard without without the simulator availability. Landing this as is. Thanks for the patch.

[cbracken]

lgtm after in-person sync on this.

[fluttergithubbot]

This pull request is not suitable for automatic merging in its current state.

• The status or check suite build_and_test_linux_unopt_debug has failed. Please fix the issues identified (or deflake) before re-applying this label.

[chinmaygarde]

This needs to be rebased before landing. I'll do it later today.

[exKAZUu]

@chinmaygarde (Sorry for disturbing you.)
I hope this PR will be merged soon since the issue prevents us from upgrading flutter from 1 to 2.

[dot-wei2021]

@chinmaygarde (Sorry for disturbing you.)
I hope this PR will be merged soon since the issue prevents us from upgrading flutter from 1 to 2.

me too

[cbracken]

I've rebased locally, but don't have permissions to push to the blackhorse-one:keys-platform-view branch. This has been pushed to #26412.

@KammererTob, if you're able to rebase against tip-of-tree and git push, we can land this patch; otherwise, I'm more than happy to land #26412, which is the same patch (but rebased) and has you has author on the commit.

Let us know which way you'd like to go, and thanks again for your patch!

[cbracken]

Landed in #26412 -- thanks again for the fix!