Flutter iOS Interactive Keyboard: Take Screenshot and Handle Pointer Movement #43972

Matt2D · 2023-07-24T21:09:37Z

This PR address the movement aspect of the flutter interactive keyboard. It handles pointer movement while a scroll view widget is visible, and the interactive behavior is chosen for keyboardDismissBehavior. This is a desired behavior of the keyboard that has not yet been implemented.
Design Document:
https://docs.google.com/document/d/1-T7_0mSkXzPaWxveeypIzzzAdyo-EEuP5V84161foL4/edit?pli=1

Issues Address:
flutter/flutter#57609

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.

hellohuanlin

Overall looks great. I will ask @cbracken to also take a look.

hellohuanlin · 2023-07-26T23:20:05Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.h

@@ -164,4 +164,9 @@ FLUTTER_DARWIN_EXPORT
 - (instancetype)initWithOwner:(FlutterTextInputPlugin*)textInputPlugin NS_DESIGNATED_INITIALIZER;

 @end
+
+@interface UIView (FindFirstResponder)
+- (id)firstResponder;


Can you rename to flt_firstResponder in case of naming conflict? (ObjC doesn't support namespace)

Good idea! Went through and made the change.

hellohuanlin · 2023-07-26T23:21:11Z

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

@@ -45,6 +45,7 @@
 static NSString* const kSetSelectionRectsMethod = @"Scribble.setSelectionRects";
 static NSString* const kStartLiveTextInputMethod = @"TextInput.startLiveTextInput";
 static NSString* const kUpdateConfigMethod = @"TextInput.updateConfig";
+static NSString* const kOnPointerMoveMethod = @"TextInput.onPointerMove";


Nit maybe kOnInteractiveKeyboardPointerMoveMethod = @"TextInput.onPointerMoveForInteractiveKeyboard", to be more specific.

Just updated, I'll make sure to try and add Interactive Keyboard more in the future, definitely would help with clarity!

hellohuanlin · 2023-07-26T23:23:11Z

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

@@ -2272,11 +2289,57 @@ - (void)handleMethodCall:(FlutterMethodCall*)call result:(FlutterResult)result {
  } else if ([method isEqualToString:kUpdateConfigMethod]) {
    [self updateConfig:args];
    result(nil);
+  } else if ([method isEqualToString:kOnPointerMoveMethod]) {
+    double pointerY = [args[@"pointerY"] doubleValue];


use CGRect rather than double for UI stuff.

With regards to the pointerY this only contains a single double, so I am not sure that this would classify as UI.

sorry i meant CGFloat

Gotcha, just updated the code!

cyanglaz · 2023-07-27T20:08:52Z

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

+- (void)handlePointerMove:(double)pointerY {
+  double screenHeight = [UIScreen mainScreen].bounds.size.height;
+  double keyboardHeight = _keyboardRect.size.height;
+  // Determines if pointer goes overtop of the keyboard


nits:

Suggested change

// Determines if pointer goes overtop of the keyboard

// Determines if pointer goes overtop of the keyboard.

Will do! Just slipped by me.

cyanglaz · 2023-07-27T20:12:16Z

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

+  // Determines if pointer goes overtop of the keyboard
+  if (screenHeight - keyboardHeight <= pointerY) {
+    // If no screenshot has been taken
+    if (_keyboardViewContainer.subviews.count == 0) {


Using _keyboardViewContainer.subviews.count == 0 doesn't seems to be safe and future proof. For example, what if someone added a new subview for _keyboardViewContainer and didn't know this logic exist? Could you use a different indicator?

Definitely a concern, the same thing occurs in the tests so there definitely is a need to update this approach. I went through and included a UIView that represents the screenshot. Then we can see if it the superview matches instead.

cyanglaz · 2023-07-27T20:27:48Z

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

+      CGRect frameRect = _keyboardRect;
+      frameRect.origin.y = pointerY;
+      _keyboardViewContainer.frame = frameRect;


nits: could you refactor these lines into a method to match what you did in if
Maybe something like:
`[self setKeyboardContainerHeight:pointerY]

Done! Always helpful to simplify code into sections to increase understandability.

cyanglaz · 2023-07-27T20:32:43Z

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

@@ -60,6 +60,8 @@ @interface FlutterSecureTextInputView : FlutterTextInputView

 @interface FlutterTextInputPlugin ()
 @property(nonatomic, assign) FlutterTextInputView* activeView;
+@property(nonatomic, assign) UIView* keyboardViewContainer;


Suggested change

@property(nonatomic, assign) UIView* keyboardViewContainer;

@property(nonatomic, weak) UIView* keyboardViewContainer;

@cyanglaz i could be wrong, but iirc the standard practice is just assign when exposing the property in test header (similar to the activeView properly above). weak is confusing since it's actually a strong property.

Oh. I didn't realize it is only exposing the property. I'm not aware of the standard practice tho but since activeView also used assign, I will take your word for it :)

@cyanglaz and I just discussed offline - it should be readonly here, since your unit test does not need the setter.

@property(nonatomic, readonly) UIView* keyboardViewContainer;

Gotcha! That works for me, just went through and made the changes.

cyanglaz · 2023-08-02T16:19:24Z

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

  } else {
    result(FlutterMethodNotImplemented);
  }
 }

+- (void)handlePointerMove:(CGFloat)pointerY {
+  double screenHeight = [UIScreen mainScreen].bounds.size.height;


We are moving away from using [UIScreen mainScreen]. I don't think we should block this PR because of this, but we will need to follow up to clean up later.
FYI @mossmana

Is the pointerY based on the screen, or the flutter app?

Since it comes from the Framework, I assume it is based on the flutter app? So In scenarios (add to app, or app extension) when the flutter app does not fill the full screen, the calculation below that determines if the pointer goes overtop of the keyboard should be based on the flutter app's height instead of the screen height, correct?

Maybe instead of the screenHeight, we should use the height of the FlutterViewController's view?

We are moving away from using [UIScreen mainScreen]. I don't think we should block this PR because of this, but we will need to follow up to clean up later. FYI @mossmana

@Matt2D While usually I would agree with not blocking the PR, in this particular case, this is a new introduction of [UIScreen mainScreen]. I would prefer that we don't add new instances. You can refer to flutter/flutter#125496 for how we are handling the deprecation in other places.

@cyanglaz and I were discussing this the other day, and it's actually a bit tricky here - we would need to refactor this code to make it re-usable.

I think I can take this part over. Can make a PR today.

@cyanglaz in this case, since this is being used to determine the pointer position on the screen, this is relative to the screen rather than the flutter view.

FYI this is the PR promised: #44303

After that gets landed, you can do something like this here:

UIScreen *screen = _viewController.flutterScreenIfViewLoaded; if (!screen) { return; } CGFloat screenHeight = screen.bounds.size.height; // ...

Thank you for the code snippet, I think the new api is much cleaner! Updated the code using the mentioned solution.

cyanglaz · 2023-08-02T16:32:11Z

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

+}
+
+- (void)takeScreenshot {
+  UIView* keyboardSnap = [[UIScreen mainScreen] snapshotViewAfterScreenUpdates:YES];


ditto about [UIScreen mainScreen]

Updated with the fix provided by the new PR Huan did.

cyanglaz · 2023-08-02T16:33:54Z

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

+    [UIApplication.sharedApplication.delegate.window.rootViewController.view
+        addSubview:_keyboardViewContainer];


Should it be FlutterViewController instead?

In add to app scenario, the rootViewController might not be FlutterViewController, which might be covered by FlutterViewController. So the keyboard might be shown "behind" the FlutterViewController

Since this is positioning a keyboard relative to the full screen -- regardless of whether the FlutterView is full screen, or a subview covering only part of the screen in an add-to-app scenario -- I think we want the root view controller don't we?

Ah yeah that's true. We would need to make sure to position the keyboard screenshot on the very top. Set ZIndex to maxInt maybe?

Definitely worth adding!

cyanglaz · 2023-08-02T16:35:24Z

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

+  // Determines if pointer goes overtop of the keyboard.
+  if (screenHeight - keyboardHeight <= pointerY) {
+    // If no screenshot has been taken.
+    if (_keyboardView.superview != nil) {


Did you mean if (_keyboardView.superview == nil)

Yep you are right! Thanks for catching this.

cyanglaz · 2023-08-02T16:40:08Z

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

+  CGRect frameRect = _keyboardRect;
+  frameRect.origin.y = pointerY;
+  _keyboardViewContainer.frame = frameRect;


minor optional nits:

The logic of this method can also be used for the scenario where the pointer is not overtop the keyboard.
The frameRect's Y can be updated to be the max of pointerY and keyboard height.

CGRect frameRect = _keyboardRect; if (screenHeight - keyboardHeight <= pointerY) { frameRect.origin.y = pointerY; } _keyboardViewContainer.frame = frameRect;

Then in the handlePointerMove, the logic can be simplified as

if (_keyboardView.superview != nil) { [self setKeyboardContainerHeight:pointerY]; } else { [self takeScreenshot]; [self hideKeyboardWithoutAnimation]; }

It turns out that for the change we still need to check the height for takeScreenshot since we don't want to take the screenshot too early:

- (void)handlePointerMove:(CGFloat)pointerY { //View must be loaded at this point. UIScreen* screen = _viewController.flutterScreenIfViewLoaded; double screenHeight = screen.bounds.size.height; double keyboardHeight = _keyboardRect.size.height; if (_keyboardView.superview != nil) { // If screenshot is present. [self setKeyboardContainerHeight:pointerY]; } else { // If no screenshot has been taken. if (screenHeight - keyboardHeight <= pointerY) { //If pointer is above keyboard. [self takeKeyboardScreenshotAndDisplay]; [self hideKeyboardWithoutAnimation]; } } } - (void)setKeyboardContainerHeight:(CGFloat)pointerY { //View must be loaded at this point. UIScreen* screen = _viewController.flutterScreenIfViewLoaded; double screenHeight = screen.bounds.size.height; double keyboardHeight = _keyboardRect.size.height; CGRect frameRect = _keyboardRect; if (screenHeight - keyboardHeight <= pointerY) { frameRect.origin.y = pointerY; } _keyboardViewContainer.frame = frameRect; }

Ah I missed that part. So the caller part can probably be

if (_keyboardView.superview != nil) { [self setKeyboardContainerHeight:pointerY]; } else if (screenHeight - keyboardHeight <= pointerY) { [self takeScreenshot]; [self hideKeyboardWithoutAnimation]; }

Discussed offline - the duplicate logic of screenHeight - keyboardHeight <= pointerY can get error prune.

Another idea is to pass in an additional BOOL to the helper function, but that also makes the code unnecessarily complicate.

I think it makes sense to keep it as it is.

cyanglaz · 2023-08-02T16:44:35Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.h

@@ -164,4 +164,9 @@ FLUTTER_DARWIN_EXPORT
 - (instancetype)initWithOwner:(FlutterTextInputPlugin*)textInputPlugin NS_DESIGNATED_INITIALIZER;

 @end
+
+@interface UIView (FindFirstResponder)
+- (id)flt_firstResponder;


optional nits: use a readonly property instead.

Avoid underscore in obj-c identifiers. We're doing something similar in another outstanding PR on the macOS side and using flutter as the prefix here:
https://github.com/flutter/engine/pull/43101/files#diff-edbbe0be10d99338c2496cad85a27776c79ec8b260825ee4d309b1e4d194d5b2R10

So we'd end up with flutterFirstResponder here.

@cyanglaz @hellohuanlin preferences? We should probably be consistent across embedders.

flutterFirstResponder SGTM

Will do! Thank you for the suggestion

cbracken · 2023-08-02T18:44:31Z

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

+- (void)handlePointerMove:(CGFloat)pointerY {
+  double screenHeight = [UIScreen mainScreen].bounds.size.height;
+  double keyboardHeight = _keyboardRect.size.height;
+  // Determines if pointer goes overtop of the keyboard.


Maybe something like:

Suggested change

// Determines if pointer goes overtop of the keyboard.

// If the pointer is within the bounds of the keyboard.

(And maybe move the comment inside the conditional block)

Makes sense, thank you!

cbracken · 2023-08-02T18:51:33Z

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

+  [UIView setAnimationsEnabled:YES];
+}
+
+- (void)takeScreenshot {


Consider renaming to something more specific for readability -- e.g. take KeyboardSnapshot

Definitely, I also added add to screen to be even better for readability.

cbracken · 2023-08-02T19:02:02Z

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

+                             result:^(id _Nullable result){
+                             }];
+  XCTAssert(textInputPlugin.keyboardView.superview != nil);
+  CGFloat newHeight = 600;


Consider just inlining this into the call below, or move this up above and use in both the event above and the check below.

Definitely!

cbracken · 2023-08-02T19:03:40Z

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

+                             result:^(id _Nullable result){
+                             }];
+  XCTAssert(textInputPlugin.keyboardView.superview != nil);
+  CGFloat newHeight = 600;


Similar comment here.

cbracken · 2023-08-02T19:05:51Z

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

+  }
+}
+
+- (void)testInteractiveKeyboardScreenshotWillBeMovedUpAfterUserScroll {


Maybe rather than just "up" something like "back to original position"?

Good idea! Done

cbracken

Overall looks great - just a few comments aside from those left by others!

…able (#44303) The existing `screenIfViewLoaded` and `windowSceneIfLoaded` functions are private helpers of `FlutterViewController` class. This PR moves the logic to a category of the `UIViewController`, so it can be reused for any `UIViewController`s. *List which issues are fixed by this PR. You must list at least one issue.* #43972 (comment) *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

cyanglaz

LGTM

auto-submit · 2023-08-03T22:44:23Z

auto label is removed for flutter/engine, pr: 43972, due to - The status or check suite Linux mac_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

…e Pointer Movement (flutter/engine#43972)

…131950) flutter/engine@a7bea86...c254deb 2023-08-04 skia-flutter-autoroll@skia.org Roll Skia from 85938bb68e75 to 45c0a830d805 (3 revisions) (flutter/engine#44397) 2023-08-04 55360120+Matt2D@users.noreply.github.com Flutter iOS Interactive Keyboard: Take Screenshot and Handle Pointer Movement (flutter/engine#43972) 2023-08-04 skia-flutter-autoroll@skia.org Roll Dart SDK from b3372378e487 to a0b59bac20fc (1 revision) (flutter/engine#44393) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jacksongardner@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…able (flutter#44303) The existing `screenIfViewLoaded` and `windowSceneIfLoaded` functions are private helpers of `FlutterViewController` class. This PR moves the logic to a category of the `UIViewController`, so it can be reused for any `UIViewController`s. *List which issues are fixed by this PR. You must list at least one issue.* flutter#43972 (comment) *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…Movement (flutter#43972) This PR address the movement aspect of the flutter interactive keyboard. It handles pointer movement while a scroll view widget is visible, and the interactive behavior is chosen for keyboardDismissBehavior. This is a desired behavior of the keyboard that has not yet been implemented. Design Document: https://docs.google.com/document/d/1-T7_0mSkXzPaWxveeypIzzzAdyo-EEuP5V84161foL4/edit?pli=1 Issues Address: flutter/flutter#57609 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

github-actions bot added the platform-ios label Jul 24, 2023

Matt2D requested review from hellohuanlin and cbracken July 24, 2023 21:10

Matt2D force-pushed the screenshot_pointer_move branch from 3091c86 to fb66944 Compare July 26, 2023 21:29

Matt2D changed the title ~~Screenshot pointer move~~ Flutter iOS Interactive Keyboard: Take Screenshot and Handle Pointer Movement Jul 26, 2023

Matt2D marked this pull request as ready for review July 26, 2023 21:44

hellohuanlin reviewed Jul 26, 2023

View reviewed changes

hellohuanlin requested a review from cyanglaz July 27, 2023 17:53

Matt2D force-pushed the screenshot_pointer_move branch 2 times, most recently from 5a5c269 to a41e685 Compare July 27, 2023 20:26

cyanglaz suggested changes Jul 27, 2023

View reviewed changes

Matt2D force-pushed the screenshot_pointer_move branch from a41e685 to 3e41f37 Compare July 27, 2023 21:48

Matt2D requested review from cyanglaz and hellohuanlin July 27, 2023 21:59

Matt2D force-pushed the screenshot_pointer_move branch from 3e41f37 to 314f3db Compare July 27, 2023 22:49

cyanglaz reviewed Aug 2, 2023

View reviewed changes

cbracken reviewed Aug 2, 2023

View reviewed changes

hellohuanlin mentioned this pull request Aug 2, 2023

[ios]make the screenIfViewLoaded and windowSceneIfLoaded helpers reusable #44303

Merged

8 tasks

Matt2D added 3 commits August 3, 2023 14:12

Keyboard Screenshot Pointer Move

fa51055

Address Feedback

2d8138b

Format

881e7be

Matt2D force-pushed the screenshot_pointer_move branch from 314f3db to 881e7be Compare August 3, 2023 21:23

Rename

f07d337

Matt2D requested a review from cyanglaz August 3, 2023 21:30

Matt2D requested a review from cbracken August 3, 2023 21:30

Renamed to cached first responder

ff6f3d5

cyanglaz approved these changes Aug 3, 2023

View reviewed changes

hellohuanlin approved these changes Aug 3, 2023

View reviewed changes

hellohuanlin added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 3, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 3, 2023

Matt2D added 2 commits August 4, 2023 13:47

Update test to reflect new changes

09a5ef5

Format

f3eadbd

hellohuanlin added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 4, 2023

auto-submit bot merged commit 2ab72fa into flutter:main Aug 4, 2023

engine-flutter-autoroll mentioned this pull request Aug 4, 2023

Roll Flutter Engine from a7bea8621f4b to c254deb8fbda (3 revisions) flutter/flutter#131950

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 4, 2023

2ab72fac8 Flutter iOS Interactive Keyboard: Take Screenshot and Handl…

011e5cf

…e Pointer Movement (flutter/engine#43972)

cbracken mentioned this pull request Dec 7, 2023

Let iOS keyboard dismissal synchronize with scroll flutter/flutter#57609

Open

	// Determines if pointer goes overtop of the keyboard
	// Determines if pointer goes overtop of the keyboard.

	@property(nonatomic, assign) UIView* keyboardViewContainer;
	@property(nonatomic, weak) UIView* keyboardViewContainer;

		[UIApplication.sharedApplication.delegate.window.rootViewController.view
		addSubview:_keyboardViewContainer];

	// Determines if pointer goes overtop of the keyboard.
	// If the pointer is within the bounds of the keyboard.

Flutter iOS Interactive Keyboard: Take Screenshot and Handle Pointer Movement #43972

Flutter iOS Interactive Keyboard: Take Screenshot and Handle Pointer Movement #43972

Uh oh!

Conversation

Matt2D commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

hellohuanlin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Matt2D commented Jul 24, 2023 •

edited

Loading

Matt2D Aug 3, 2023 •

edited

Loading