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

[iOS] Fix:Keyboard inset is not correct when presenting a native ViewController on FlutterViewController #29862

Merged
jmagman merged 2 commits into from
Nov 22, 2021
Merged

[iOS] Fix:Keyboard inset is not correct when presenting a native ViewController on FlutterViewController #29862

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f72695a
[iOS] Fix keyboard inset is not correct when presenting and native Vi…
luckysmg Nov 20, 2021
ac4fa44
Add viewport update unit test
jmagman Nov 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1200,9 +1200,10 @@ - (void)startKeyBoardAnimation:(NSTimeInterval)duration {
}
completion:^(BOOL finished) {
Copy link
Member

@jmagman jmagman Nov 22, 2021

Choose a reason for hiding this comment

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

finished may be false here because the keyboardAnimationView with the animation was removed from the superView before the animation was completed? Not sure though.

if (self.displayLink == currentDisplayLink) {
// Indicates the displaylink captured by this block is the original one,which also
// indicates the animation has not been interrupted from its beginning. Moreover,indicates
// the animation is over and there is no more animation about to exectute.
Comment on lines +1203 to +1205
Copy link
Member

@jmagman jmagman Nov 22, 2021

Choose a reason for hiding this comment

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

Suggested change
// Indicates the displaylink captured by this block is the original one,which also
// indicates the animation has not been interrupted from its beginning. Moreover,indicates
// the animation is over and there is no more animation about to exectute.
// Indicates the displayLink captured by this block is the original one, which also
// indicates the animation has not been interrupted. Moreover, indicates
// the animation is over and there is no more to execute.

[self invalidateDisplayLink];
}
if (finished) {
Comment on lines -1204 to -1205
Copy link
Member

@gaaclarke gaaclarke Nov 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, this looks like a good change to me. This will be tricky to write a test for.

Copy link
Member

@jmagman jmagman Nov 22, 2021

Choose a reason for hiding this comment

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

This will definitely be tricky to test. I don't love the partial mocks we use for this now since we're not really testing the behavior, but that the code is being executed in the right order

engine/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm

Lines 187 to 190 in 2962099

id viewControllerMock = OCMPartialMock(viewController);
[viewControllerMock viewDidDisappear:YES];
OCMVerify([viewControllerMock ensureViewportMetricsIsCorrect]);
OCMVerify([viewControllerMock invalidateDisplayLink]);

This doesn't really test that the change is fixed, but you could add an expectation/verification that the engine -updateViewportMetrics: is called after startKeyBoardAnimation.

engine/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm

Line 178 in 2962099

OCMVerify([viewControllerMock startKeyBoardAnimation:0.25]);

engine/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm

Line 338 in 2962099

OCMVerify([mockEngine updateViewportMetrics:viewportMetrics]);

Copy link
Member

@jmagman jmagman Nov 22, 2021

Choose a reason for hiding this comment

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

This test passed on this diff and fails on main:

                   }];
+
+  XCTestExpectation* expectation = [self expectationWithDescription:@"update viewport"];
+  OCMStub([mockEngine updateViewportMetrics:flutter::ViewportMetrics()])
+      .ignoringNonObjectArgs()
+      .andDo(^(NSInvocation* invocation) {
+        [expectation fulfill];
+      });
   id viewControllerMock = OCMPartialMock(viewController);
   [viewControllerMock keyboardWillChangeFrame:notification];
   OCMVerify([viewControllerMock startKeyBoardAnimation:0.25]);
+  [self waitForExpectationsWithTimeout:5.0 handler:nil];

Copy link
Member

@jmagman jmagman Nov 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I took the liberty of pushing this test to your fork since this bug fix is blocking an internal release. Hope you don't mind. 🙂

ac4fa44

[self removeKeyboardAnimationView];
[self ensureViewportMetricsIsCorrect];
}
Expand Down
8 changes: 8 additions & 0 deletions shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,9 +173,17 @@ - (void)testkeyboardWillChangeFrameWillStartKeyboardAnimation {
@"UIKeyboardAnimationDurationUserInfoKey" : [NSNumber numberWithDouble:0.25],
@"UIKeyboardIsLocalUserInfoKey" : [NSNumber numberWithBool:isLocal]
}];

XCTestExpectation* expectation = [self expectationWithDescription:@"update viewport"];
Copy link
Member

@jmagman jmagman Nov 22, 2021

Choose a reason for hiding this comment

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

@gaaclarke would you mind reviewing this test change? I just added it.

Copy link
Member

@gaaclarke gaaclarke Nov 22, 2021

Choose a reason for hiding this comment

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

LGTM

OCMStub([mockEngine updateViewportMetrics:flutter::ViewportMetrics()])
.ignoringNonObjectArgs()
.andDo(^(NSInvocation* invocation) {
[expectation fulfill];
});
id viewControllerMock = OCMPartialMock(viewController);
[viewControllerMock keyboardWillChangeFrame:notification];
OCMVerify([viewControllerMock startKeyBoardAnimation:0.25]);
[self waitForExpectationsWithTimeout:5.0 handler:nil];
}

- (void)testEnsureViewportMetricsWillInvokeAndDisplayLinkWillInvalidateInViewDidDisappear {
Expand Down