Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@cbracken
Copy link
Member

@cbracken cbracken commented May 25, 2021

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.

flutter/flutter#74044

This patch is #25365 by @KammererTob rebased to tip of tree.

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@google-cla
Copy link

google-cla bot commented May 25, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label May 25, 2021
@cbracken cbracken requested a review from chinmaygarde May 25, 2021 21:04
@google-cla
Copy link

google-cla bot commented May 25, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@cbracken
Copy link
Member Author

@googlebot I consent.

@google-cla
Copy link

google-cla bot commented May 25, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@KammererTob
Copy link
Contributor

@googlebot I consent.

@google-cla google-cla bot added cla: yes and removed cla: no labels May 26, 2021
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.

Issue: flutter/flutter#74044
@cbracken cbracken 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 May 26, 2021
@fluttergithubbot fluttergithubbot merged commit 576430a into flutter:master May 26, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 27, 2021
@cbracken cbracken deleted the keys-platform-view branch May 27, 2021 17:38
Copy link

@fbartho fbartho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hypothetically, the super-call could delete resources that the dispatchPresses implicitly relies on.

So when tearing stuff down, you want to teardown system resources after your own resources, just like when building some stuff up, you want to build the system resources before building your own resources.

- (void)pressesEnded:(NSSet<UIPress*>*)presses withEvent:(UIEvent*)event API_AVAILABLE(ios(9.0)) {
- (void)pressesEnded:(NSSet<UIPress*>*)presses
withEvent:(UIPressesEvent*)event API_AVAILABLE(ios(9.0)) {
[super pressesEnded:presses withEvent:event];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this have been moved to end-of-method?

Just like onViewDidDisappear

- (void)pressesCancelled:(NSSet<UIPress*>*)presses
withEvent:(UIEvent*)event API_AVAILABLE(ios(9.0)) {
withEvent:(UIPressesEvent*)event API_AVAILABLE(ios(9.0)) {
[super pressesCancelled:presses withEvent:event];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this have been moved to end-of-method?

Just like onViewDidDisappear

@olibanjoli
Copy link

olibanjoli commented Jul 8, 2021

any chance this comes with flutter 2.3.0 release? or even as bugfix release on 2.2?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes platform-ios waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants