-
Notifications
You must be signed in to change notification settings - Fork 6k
[macOS] Put FlutterTextInputPlugin in view hierarchy #33827
[macOS] Put FlutterTextInputPlugin in view hierarchy #33827
Conversation
eef9255 to
3f51a94
Compare
00a82f0 to
f50304a
Compare
LongCatIsLooong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @cbracken
shell/platform/darwin/macos/framework/Source/FlutterKeyboardManager.h
Outdated
Show resolved
Hide resolved
| } else if ([method isEqualToString:kShowMethod]) { | ||
| // Ensure the plugin is in hierarchy. | ||
| // When accessibility text field becomes first responder AppKit sometimes | ||
| // removes the plugin from hierarchy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find the place where the text input plugin is added to the view hierarchy. You mean with this change the accessibility text field still causes the text input plugin to be removed as a subview when it has the focus?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When FlutterTextField has focus and FlutterTextInputPlugin becomes the field editor of ([FlutterTextField startEditing]) the input plugin gets reparented inside the text field.
shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm
Outdated
Show resolved
Hide resolved
| } | ||
| [_client becomeFirstResponder]; | ||
| if (_client != nil) { | ||
| [self.window makeFirstResponder:_client]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for fixing this! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the wrong fix. The TextInputField is actually not meant to be first responder. Instead the FlutterTextInputPlugin is supposed to be the first responder. What the original becomeFirstResponder did was not making the text field first responder, it made FlutterTextInputPlugin current field editor for the text field. It happened as a sideeffect of becomeFirstResponder calling selectText, which sets the field editor as current editor.
So instead of overriding becomeFirstResponder and calling it directly (which shouldn't be done) I replaced it with [FlutterTextField startEditing], which makes the text input plugin current field editor.
75fdba0 to
eb2b414
Compare
252ba98 to
4c8aa84
Compare
dkwingsmt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@cbracken, @LongCatIsLooong, any objections to getting this merged? |
Emoji picker is different than the other IME popovers. In order for the picker to be attached properly,
FlutterTextInputPluginmust be in view hierarchy and first responder.Notable changes in this PR:
FlutterTextInputPluginis added to hierarchy inTextInput.show, removed inTextInput.hide.[TextInputContext activate]anddeactivatecalls are not needed anymore and were removed. The context gets activated automatically now. I also removed theTestTextInputContextIsKeptAliveunit test. Unfortunately I wasn't able to get the context to activate in "headless" unit test environment, but in actual application the context gets activated/deactivated as expected (tested by overriding theactivate/deactivatemethods on context).performKeyEquivalent:. ThekeyUpevent monitor inFlutterViewControllertakes care of this. It needed to be modified to allowTextInputPluginas first responder (which is also works with accessibility text field)[FlutterTestInputPlugin performKeyEquivalent:]now simply calls[FlutterViewController keyDown:]. With one caveat: When theCMD+CTRL+SPACEemoji picker shortcut event gets redispached tonextResponderbyFlutterKeyboardManager, thenextResponderis usuallyNSWindowand it routes the event to[FlutterTestInputPlugin performKeyEquivalent:]. Sending it again to controller would result in endless cycle, so theFlutterTextInputPluginchecks if the event in question is currently being redispatched by manager and if it is, it will not handle it. Not handling the event is also required for the emoji picker to actually show up.becomeFirstResponderwith[NSWindow makeFirstResponder:]. Unlike UIKit, in AppKit the becomeFirstResponder documentation explicitely says not to call this method directly and in some cases it leads to crash with unhandledNSInternalInconsistencyException.[FlutterTextField becomeFirstResponder], which was called directly has been replaced by[FlutterTextField startEditing]. This method setsFlutterTextPluginas field editor to theFlutterTextField. This was previously happening as a side effect ofbecomeFirstRespondercallingselectText.Partially fixes flutter/flutter#85328
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
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.