[iOS] Fix:Keyboard inset is not correct when presenting a native ViewController on FlutterViewController by luckysmg · Pull Request #29862 · flutter/engine

luckysmg · 2021-11-20T06:34:12Z

Before:

before.mov

After:

After.mov

List which issues are fixed by this PR. You must list at least one issue.
flutter/flutter#93944

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 Hixie said 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].

…ewController on FlutterViewController

flutter-dashboard · 2021-11-20T06:34:14Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

gaaclarke · 2021-11-22T17:51:26Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

-        }
-        if (finished) {


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

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]);

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];

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

gaaclarke

LGTM modulo @jmagman's approval for a testing exception. I can't think of a good way to test. It would probably have to be a golden image test that we don't have a test harness for.

jmagman

Code LGTM, I think we can add a verification that the viewPort metrics are updated after startKeyBoardAnimation.

jmagman · 2021-11-22T18:11:01Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

@@ -1200,9 +1200,10 @@ - (void)startKeyBoardAnimation:(NSTimeInterval)duration {
      }
      completion:^(BOOL finished) {


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

jmagman · 2021-11-22T18:14:11Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+          // 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.


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.

jmagman · 2021-11-22T18:20:18Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

-        }
-        if (finished) {


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]);

jmagman · 2021-11-22T19:16:00Z

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

                    @"UIKeyboardIsLocalUserInfoKey" : [NSNumber numberWithBool:isLocal]
                  }];
+
+  XCTestExpectation* expectation = [self expectationWithDescription:@"update viewport"];


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

jmagman · 2021-11-22T23:27:49Z

@luckysmg Thanks for fixing this so quickly. I'm going to merge this, if you have any code changes to the test I wrote please let me know and we can discuss in a follow-up commit.
Engine tree is actually green, see flutter/flutter#93872.

Merging.

…ative ViewController on FlutterViewController (flutter/engine#29862)

…ewController on FlutterViewController (flutter#29862)

Request from b/207432361

…ewController on FlutterViewController (flutter#29862)

…ewController on FlutterViewController (#29862) (#30162) Co-authored-by: WenJingRui <2539699336@qq.com>

[iOS] Fix keyboard inset is not correct when presenting and native Vi…

f72695a

…ewController on FlutterViewController

flutter-dashboard bot added platform-ios needs tests labels Nov 20, 2021

google-cla bot added the cla: yes label Nov 20, 2021

ColdPaleLight requested review from gaaclarke and jmagman November 20, 2021 07:13

luckysmg changed the title ~~[iOS] Fix keyboard inset is not correct when presenting and native ViewController on FlutterViewController~~ [iOS] Fix:Keyboard inset is not correct when presenting and native ViewController on FlutterViewController Nov 20, 2021

gaaclarke reviewed Nov 22, 2021

View reviewed changes

gaaclarke approved these changes Nov 22, 2021

View reviewed changes

jmagman reviewed Nov 22, 2021

View reviewed changes

Add viewport update unit test

ac4fa44

jmagman removed the needs tests label Nov 22, 2021

jmagman reviewed Nov 22, 2021

View reviewed changes

jmagman merged commit ec4f72b into flutter:main Nov 22, 2021

jmagman mentioned this pull request Nov 22, 2021

Native iOS screen shows keyboard, underlying Flutter view moves text up for keyboard, doesn't reset when keyboard/native screen dismissed flutter/flutter#93944

Closed

engine-flutter-autoroll mentioned this pull request Nov 23, 2021

Roll Engine from fd1151a43bb6 to 3845cee5b721 (4 revisions) flutter/flutter#94081

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 23, 2021

ec4f72b [iOS] Fix:Keyboard inset is not correct when presenting and n…

04ae339

…ative ViewController on FlutterViewController (flutter/engine#29862)

engine-flutter-autoroll mentioned this pull request Nov 23, 2021

Roll Engine from fd1151a43bb6 to 147824762f95 (8 revisions) flutter/flutter#94082

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 23, 2021

ec4f72b [iOS] Fix:Keyboard inset is not correct when presenting and n…

babf267

…ative ViewController on FlutterViewController (flutter/engine#29862)

luckysmg changed the title ~~[iOS] Fix:Keyboard inset is not correct when presenting and native ViewController on FlutterViewController~~ [iOS] Fix:Keyboard inset is not correct when presenting a native ViewController on FlutterViewController Nov 23, 2021

xster pushed a commit to xster/engine that referenced this pull request Nov 23, 2021

[iOS] Fix:Keyboard inset is not correct when presenting and native Vi…

75476d2

…ewController on FlutterViewController (flutter#29862)

xster added a commit that referenced this pull request Nov 23, 2021

Cherrypick #29783 and #29862 to flutter-2.8-candidate.7 (#29898)

d702022

Request from b/207432361

luckysmg deleted the keyboard_anim_fix branch November 25, 2021 08:14

luckysmg restored the keyboard_anim_fix branch November 25, 2021 08:14

luckysmg deleted the keyboard_anim_fix branch November 25, 2021 08:14

xster pushed a commit to xster/engine that referenced this pull request Dec 6, 2021

[iOS] Fix:Keyboard inset is not correct when presenting and native Vi…

26af31d

…ewController on FlutterViewController (flutter#29862)

xster added a commit that referenced this pull request Dec 7, 2021

[iOS] Fix:Keyboard inset is not correct when presenting and native Vi…

d50cb3b

…ewController on FlutterViewController (#29862) (#30162) Co-authored-by: WenJingRui <2539699336@qq.com>

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

		@@ -1200,9 +1200,10 @@ - (void)startKeyBoardAnimation:(NSTimeInterval)duration {
		}
		completion:^(BOOL finished) {

Conversation

luckysmg commented Nov 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Nov 20, 2021

Uh oh!

gaaclarke Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

jmagman Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

jmagman Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

jmagman Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

jmagman Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

jmagman Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

gaaclarke Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

jmagman commented Nov 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

luckysmg commented Nov 20, 2021 •

edited

Loading

gaaclarke Nov 22, 2021 •

edited

Loading

jmagman Nov 22, 2021 •

edited

Loading