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

Fixed a case where the KeyUp event is not called by Command+Key on MacOS#45565

Closed
zuvola wants to merge 6 commits into
flutter-team-archive:mainfrom
zuvola:fix_keyup
Closed

Fixed a case where the KeyUp event is not called by Command+Key on MacOS#45565
zuvola wants to merge 6 commits into
flutter-team-archive:mainfrom
zuvola:fix_keyup

Conversation

@zuvola

@zuvola zuvola commented Sep 8, 2023

Copy link
Copy Markdown

Fixes flutter/flutter#133832

The KeyUp event was not being called when the first responder was the FlutterViewWrapper.

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.

@knopp

knopp commented Sep 9, 2023

Copy link
Copy Markdown
Contributor

I don't know if this is the correct solution. Why is FlutterViewWrapper first responder in the first place?

@zuvola

zuvola commented Sep 14, 2023

Copy link
Copy Markdown
Author

@knopp
FlutterTextInputPlugin sets self.nextResponder as first responder, so FlutterViewWrapper, the parent of FlutterTextInputPlugin, becomes the first responder It seems to be the case.

https://github.com/flutter/engine/blob/727fd967973a931bd24f331ec31ad928ab285b37/shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm#L384-L392

Should I have fixed the resignAndRemoveFromSuperview?

@knopp

knopp commented Sep 17, 2023

Copy link
Copy Markdown
Contributor

I'm not sure what the correct solution is here, I will look into it, but I think the responder chain should be consistent before and after text input plugin is added/removed from hierarchy.

@knopp knopp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this is correct solution. The problem here is that firstResponder is not being consistent, which should be fixed by #46032.

@zuvola

zuvola commented Sep 19, 2023

Copy link
Copy Markdown
Author

Ok, I understand. Thanks for the correction.

@zuvola zuvola closed this Sep 19, 2023
@zuvola zuvola deleted the fix_keyup branch September 19, 2023 23:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Development

Successfully merging this pull request may close these issues.

Unable to move focus with shortcuts.

2 participants