New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[iOS] Send connectionClosed message when resignFirstResponder to ensure framework focus state is correct. #40703
Conversation
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 % nits
shell/platform/darwin/ios/framework/Source/FlutterEngineTest.mm
Outdated
Show resolved
Hide resolved
Did you mean to link to a framework PR instead? |
shell/platform/darwin/ios/framework/Source/FlutterEngineTest.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterTextInputDelegate.h
Outdated
Show resolved
Hide resolved
There's already this message: https://master-api.flutter.dev/flutter/services/TextInputClient/connectionClosed.html |
@@ -1043,7 +1043,8 @@ - (BOOL)canBecomeFirstResponder { | |||
- (BOOL)resignFirstResponder { | |||
BOOL success = [super resignFirstResponder]; | |||
if (success) { | |||
[self.textInputDelegate flutterTextInputViewDidResignFirstResponder:self]; | |||
[self.textInputDelegate flutterTextInputViewDidResignFirstResponder:self |
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.
nit: Looks like this one will be called when either the Flutter framework or UIKit tells the input view to give up its first responder status. It seems redundant for the former since there's no need to inform the Flutter framework when it is the one that initiated the focus change.
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.
Yes. When framework close the keyboard first, this will send a msg to framework but will be handled return
by
And only when platform starts to unfocus, the _currentConnection!._client.connectionClosed();
will be called.
Currently, engine side doens't have a good simple way to know the original focus/unfocus signal is from framework or from platform, this maybe can be done by set some var like BOOL sendFromFramework
to judge, but I think that will be overkill........(Maybe I missed something that can judge this)
So I think this cost can be acceptable? ^_^
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.
Yeah that makes sense the old connection is already torn down by the framework so messages associated with the old client id will be thrown away.
Oh.thx for reminding me that. I will try invoke that method to unfocus. |
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 LGTM, would like @LongCatIsLooong to confirm concerns were addressed.
Thx everyone reviewing this. Applying label for merging O(∩_∩)O |
…er to ensure framework focus state is correct. (flutter/engine#40703)
…rstResponder to ensure framework focus state is correct. (flutter/engine#40703) (flutter/flutter#123753)
…rstResponder to ensure framework focus state is correct. (flutter/engine#40703) (flutter/flutter#123753)
…rstResponder to ensure framework focus state is correct. (flutter/engine#40703) (flutter/flutter#123753)
…rstResponder to ensure framework focus state is correct. (flutter/engine#40703) (flutter/flutter#123753)
…rstResponder to ensure framework focus state is correct. (flutter/engine#40703) (flutter/flutter#123753)
…rstResponder to ensure framework focus state is correct. (flutter/engine#40703) (flutter/flutter#123753)
…rstResponder to ensure framework focus state is correct. (flutter/engine#40703) (flutter/flutter#123753)
…rstResponder to ensure framework focus state is correct. (flutter/engine#40703) (flutter/flutter#123753)
…rstResponder to ensure framework focus state is correct. (flutter/engine#40703) (flutter/flutter#123753)
…rstResponder to ensure framework focus state is correct. (flutter/engine#40703) (flutter/flutter#123753)
…rstResponder to ensure framework focus state is correct. (flutter/engine#40703) (flutter/flutter#123753)
…rstResponder to ensure framework focus state is correct. (flutter/engine#40703) (flutter/flutter#123753)
…rstResponder to ensure framework focus state is correct. (flutter/engine#40703) (flutter/flutter#123753)
…er to ensure framework focus state is correct. (flutter/engine#40703) (flutter#123753) Roll Flutter Engine from 7dcbf0edf6b4 to bad50ddc9f3d (1 revision)
…rstResponder to ensure framework focus state is correct. (flutter/engine#40703) (flutter/flutter#123753)
…rstResponder to ensure framework focus state is correct. (flutter/engine#40703) (flutter/flutter#123753)
Fix:
Before this patch:
see in linked issue.
After this patch:
Pre-launch Checklist
///
).