-
Notifications
You must be signed in to change notification settings - Fork 6k
Fix issues related to keyboard inset #37719
Conversation
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
Thx @vashworth for working on fixing this. (Just some advices and sharing my thoughts, maybe not very appropriate😄). I think now we will deal with different situtation in keyboard notification function, and we use some judgement to judge the keyboard is I think maybe we can do sth to make this more clear like this
Here is the some demo code. Enum KeyboardAttachMode {
docked,
undocked,
floating
.......
}
- (KeyboardAttachMode)computeKeyboardAttachMode:(NSNotification noti){
CGRect endFrame = noti["UIKeyboardEndFrameUserKey"]
if (endFrame xxxxxx) {
return KeyboardAttachMode.docked;
} else if (xxxx) {
return KeyboardAttachMode.floating;
}
}
- (double)computeInset(KeyboardAttachMode mode, NSNotification notification) {
double inset = 0;
if (mode == .docked) {
inset = xxxxxx
} else mode == .floating{
} else .....{
}
return inset;
}
- (void)keyboardShow {
KeyboardAttachMode mode = computeKeyboardAttachMode(noti);
double computeInset = computeInset(mode,noti);
if (computeInset != self.targetButtomInset) {
self.targetBottomInset = computeInset;
startKeyboardAnimation();
}
}
- (void)keyboardWillChangeFrame {
KeyboardAttachMode mode = computeKeyboardAttachMode(noti);
double computeInset = computeInset(mode,noti);
if (computeInset != self.targetButtomInset) {
self.targetBottomInset = computeInset;
startKeyboardAnimation();
}
}
- (void)keyboardWillHide {
KeyboardAttachMode mode = computeKeyboardAttachMode(noti);
double computeInset = computeInset(mode,noti);
if (computeInset != self.targetButtomInset) {
self.targetBottomInset = computeInset;
startKeyboardAnimation();
}
} I think this will be more clear O(∩_∩)O |
shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
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.
Is it possible to use scenario app test to do some UITests? Maybe have a UITextField as PlatformView to open up keyboard, undock the keyboard and take a screenshot?
I think you mentioned this in the team meeting, is it not possible because there is no way to make the keyboard undock in XCUITests?
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
- (void)keyboardWillChangeFrame:(NSNotification*)notification { | ||
// Immediately prior to a change in keyboard frame, this notification is triggered. | ||
// There are some cases where UIKeyboardWillShowNotification & UIKeyboardWillHideNotification | ||
// do not act as expected and this is used to catch those cases. |
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.
Does the change frame notification work in all cases? Maybe we should eliminate UIKeyboardWillShowNotification
and UIKeyboardWillHideNotification
if the change frame notification is more reliable?
Could be a nice refactor in a follow up PR if we can use frame change notification for all cases.
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.
So none of them are very reliable which is why I used all three of them. Here's some of the use cases for example
- When predictive-only/minimized keyboard is dragged, it sends a
UIKeyboardWillShowNotification
notification where the keyboard = CGRectZero, which we use to determine it's "floating" and should have inset 0 . Whereas inUIKeyboardWillChangeFrameNotification
we skip when keyboard = CGRectZero, because otherwise it will set inset to 0 prematurely in some cases like when you dragged a docked keyboard but don't actually change it from docked. - When keyboard is changed from docked to floating, it's supposed to send a
UIKeyboardWillHideNotification
notification but it doesn't, so we handle inUIKeyboardWillChangeFrameNotification
instead - When keyboard goes from docked to undocked, the
UIKeyboardWillChangeFrameNotification
position will not be completely below the screen so it will still have an inset, which is at least one reason why we also needUIKeyboardWillHideNotification
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.
Ah, gotcha. I think logic this complex deserves a design documentation :) And we should have a comment in the code to link to the design doc. (The same way PlatformView should have and never did :p )
I think for this PR, it would be nice to explain "some cases" a little more in the comments. Or add a link in the code to your comment here: #37719 (comment)
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm
Outdated
Show resolved
Hide resolved
Did you mean "UIKeyboardWillChangeFrameNotification"? :) |
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
…emove rotation logic
…if keyboard intersects with screen to accomodate for repeating decimals, format
|
Nice, tests now passing! Thanks for tracking those notification issues @vashworth. |
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!
@luckysmg thanks so much for the thorough review on this one! Now that you have contributor permissions 🎉 can you mark when it's up to your approval standards?
@vashworth let's hold off merging this until that review is complete.
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
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.
Left a few questions.
Your design doc is really helpful for me to understand the code. It makes the PR review a lot easier! Great work!
std::unique_ptr<fml::WeakPtrFactory<FlutterEngine>> _weakFactory = | ||
std::make_unique<fml::WeakPtrFactory<FlutterEngine>>(engine); | ||
XCTestExpectation* invokeExpectation = | ||
[self expectationWithDescription:@"isLiveTextInputAvailableInvoke"]; | ||
FlutterPlatformPlugin* plugin = | ||
[[FlutterPlatformPlugin alloc] initWithEngine:_weakFactory->GetWeakPtr()]; | ||
[[[FlutterPlatformPlugin alloc] initWithEngine:_weakFactory->GetWeakPtr()] autorelease]; |
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 all of these.
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
} | ||
|
||
// Ignore keyboard notifications if engine’s viewController is not current viewController. | ||
// Engine’s viewController is not current viewController. | ||
if ([_engine.get() viewController] != 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.
does this happen when an app shows multiple flutter view controller on the same screen?
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, from my understanding this can happen when you're using add-to-app stuff like this flutter/flutter#39036 (comment). This is a good point, though. I didn't test use case like that.
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.
Update: I did test this use case manually and everything appeared to be working correctly.
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
|
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 O(∩_∩)O
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!
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.
Thank you for tracking all these corner cases down! LGTM!
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
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 making those changes. Your design doc is very helpful reviewing the PR.
… loaded, update a comment
…117109) * 1a1b1feee Roll Skia from 537e1e8c1ca6 to 729ccbfb87bc (7 revisions) (flutter/engine#38277) * 3b18821e1 Roll Fuchsia Linux SDK from A0jnUUORf2LQu1z2V... to e2lfUFBW5ddtTZBbw... (flutter/engine#38280) * beb94ea2c Roll Skia from 729ccbfb87bc to 3171deabd88a (4 revisions) (flutter/engine#38279) * 8f9071ab9 Roll Fuchsia Mac SDK from FQQdl8AGAsALFniHl... to u-tC0QEGUT4xQ4KOo... (flutter/engine#38282) * aa78cd8d6 add link to website (flutter/engine#38273) * 955447b35 pylint all Python scripts under testing/ (flutter/engine#38268) * 3f22e1958 [web] correct float count in runtime effect (flutter/engine#38288) * 479bb736f Fix issues related to keyboard inset (flutter/engine#37719) * 6cd85616b [macOS] Refactor rendering infrastructure (flutter/engine#37789) * d1533d12b [web] Make Canvaskit's malloc more useful (flutter/engine#38130) * db5605ea7 Fix new `unnecessary_parenthesis` diagnostics. (flutter/engine#38291)
* fix keyboard inset not collapsing when expected * fix some formatting * fix issue with rotating with undocked and split keyboard * fix formatting * fix behavior on slide over view * fix formatting * refactor to make logic more clear * move enum to header file, remove unneeded parameters, syntax fixes, remove rotation logic * ignore notification if app state is not active, change way it checks if keyboard intersects with screen to accomodate for repeating decimals, format * fix leaking unit test * use viewIfLoaded and update tests to fix mocking * change ignore logic related to application state to be more specific * add more comments * add more comments * change function name to be more clear, add warning log if view is not loaded, update a comment
* fix keyboard inset not collapsing when expected * fix some formatting * fix issue with rotating with undocked and split keyboard * fix formatting * fix behavior on slide over view * fix formatting * refactor to make logic more clear * move enum to header file, remove unneeded parameters, syntax fixes, remove rotation logic * ignore notification if app state is not active, change way it checks if keyboard intersects with screen to accomodate for repeating decimals, format * fix leaking unit test * use viewIfLoaded and update tests to fix mocking * change ignore logic related to application state to be more specific * add more comments * add more comments * change function name to be more clear, add warning log if view is not loaded, update a comment
…lutter#117109) * 1a1b1feee Roll Skia from 537e1e8c1ca6 to 729ccbfb87bc (7 revisions) (flutter/engine#38277) * 3b18821e1 Roll Fuchsia Linux SDK from A0jnUUORf2LQu1z2V... to e2lfUFBW5ddtTZBbw... (flutter/engine#38280) * beb94ea2c Roll Skia from 729ccbfb87bc to 3171deabd88a (4 revisions) (flutter/engine#38279) * 8f9071ab9 Roll Fuchsia Mac SDK from FQQdl8AGAsALFniHl... to u-tC0QEGUT4xQ4KOo... (flutter/engine#38282) * aa78cd8d6 add link to website (flutter/engine#38273) * 955447b35 pylint all Python scripts under testing/ (flutter/engine#38268) * 3f22e1958 [web] correct float count in runtime effect (flutter/engine#38288) * 479bb736f Fix issues related to keyboard inset (flutter/engine#37719) * 6cd85616b [macOS] Refactor rendering infrastructure (flutter/engine#37789) * d1533d12b [web] Make Canvaskit's malloc more useful (flutter/engine#38130) * db5605ea7 Fix new `unnecessary_parenthesis` diagnostics. (flutter/engine#38291)
Refactors keyboard logic to use a combination of UIKeyboardWillShowNotification, UIKeyboardWillChangeFrameNotification, UIKeyboardWillHideNotification notifications to determine the keyboard inset.
When a docked keyboard is shown, or when it changes from undocked/floating to docked, UIKeyboardWillShowNotification is triggered. And when a keyboard is hidden or changes from docked to undocked/floating, UIKeyboardWillHideNotification is triggered. So it would make sense to calculate and set the inset when UIKeyboardWillShowNotification happens and set the inset to 0 when UIKeyboardWillHideNotification happens.
However, there are some cases where these events are missing, have incomplete values, or do something unexpected, so we use UIKeyboardWillChangeFrameNotification in addition to help handle those cases.
Note: This fix makes a best effort to accommodate Stage Manger, but does not fix all issues related to Stage Manager. In addition, it does not address issues related to the inset when the keyboard is open and the screen orientation is rotated.
Fixes flutter/flutter#86847.
Fixes flutter/flutter#104689.
Fixes flutter/flutter#109845.
Fixes flutter/flutter#115528.
Fixes flutter/flutter#53565.
Other use cases I tested manually. I tested each case in Full Screen View, Split View, Slide Over View. I tested each case with a custom keyboard with a floating decimal height (see Custom keyboard extension code sample below).
Devices I tested with:
Sample code I used for the flutter app: flutter/flutter#104689 (comment)
Custom keyboard extension code sample
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.