[platform_view]Send platform message when platform view is focused #105050

hellohuanlin · 2022-05-31T19:45:35Z

This PR actually sends message channel to the engine to tell the TextInputPlugin that a platform view is focused.

This PR should wait till the engine roll is done (engine PR here)

We probably want to add integration test too. I am looking into how it's done now.

List which issues are fixed by this PR. You must list at least one issue.
#34187
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

jmagman · 2022-05-31T21:11:07Z

This PR should wait till the engine roll is done (engine PR here)

It was rolled into the framework a few days ago #104885

jmagman

I think including integration tests in the same PR is a good idea, while it's fresh how this works.

Code LGTM.

cyanglaz

~~LGTM % nits~~

Sorry have one question, see: #105050 (comment)

cyanglaz · 2022-05-31T21:10:21Z

packages/flutter/lib/src/widgets/platform_view.dart

@@ -660,7 +660,16 @@ class _UiKitViewState extends State<UiKitView> {
  }

  void _onFocusChange(bool isFocused) {
-    // TODO(hellohuanlin): send 'TextInput.setPlatformViewClient' channel message to engine after the engine is updated to handle this message.
+    if (!isFocused) {


nits:
Maybe this would be more readable if we check if (isFocused) first?

if (isFocused) { SystemChannels.textInput.invokeMethod<void>( 'TextInput.setPlatformViewClient', <String, dynamic>{'platformViewId': _controller!.id}, }

Or we can escape early instead of if-else

if (!isfocused) { return; } SystemChannels.textInput.invokeMethod<void>( 'TextInput.setPlatformViewClient', <String, dynamic>{'platformViewId': _controller!.id},

cyanglaz · 2022-05-31T21:17:13Z

packages/flutter/lib/src/widgets/platform_view.dart

+    } else {
+      SystemChannels.textInput.invokeMethod<void>(
+        'TextInput.setPlatformViewClient',
+        <String, dynamic>{'platformViewId': _controller!.id},


How do we ensure _controller! will not crash?

Oh because _controller and _focusNode are set together:

setState(() { _controller = controller; _focusNode = FocusNode(debugLabel: 'UiKitView(id: $id)'); });

And this _onFocusChange function is triggered by the _focusNode, which must be present, so _controller must also be present.

hellohuanlin · 2022-06-01T21:51:08Z

As discussed since we can't trigger a tap on platform view from the framework integration test (more details here)

jmagman

This LGTM, up to you whether you want to write the integration test in this PR or another, if you can get the native testing working.

hellohuanlin · 2022-06-02T03:10:14Z

~~I think i will keep this PR small for now, since the testing may take some time learning~~

Since it's not stamped yet, I will add integration test to this PR.

hellohuanlin · 2022-06-06T23:12:45Z

@cyanglaz @jmagman I added the XCUITests. it turns out that i may have to create a new flutter project because existing ios_platform_view_test calls enableFlutterDriverExtension in order to support flutter_driver, which disables keyboard for XCUITest.

jmagman

ios_platform_view_xcuitests isn't a good name, what if we wanted to add macOS or non-xcuitests to it?
Is there any reason not to add the tests to ios_platform_view_tests and reuse that project? There's so much boilerplate for every new project we add...

GitHub doesn't let us comment on binaries, but we shouldn't check in any of the app icons.

Also the analyzer isn't happy:

dev/integration_tests/ios_platform_view_xcuitests/ios/RunnerUITests/RunnerUITests.m:50: trailing blank line

jmagman · 2022-06-09T22:27:13Z

dev/integration_tests/ios_platform_view_xcuitests/.gitignore

@@ -0,0 +1,44 @@
+# Miscellaneous


It's inconsistent but I think we generally don't check in gitignores for projects in this repo since there's already a top-level gitignore. See if anything unexpected gets tracked if you remove this.

jmagman · 2022-06-09T22:29:51Z

dev/devicelab/bin/tasks/ios_platform_view_xcuitests.dart

@@ -0,0 +1,49 @@
+// Copyright 2014 The Flutter Authors. All rights reserved.


native_ui_platform_view_tests_ios to better match native_ui_tests_macos?

You'll need to add an entry to .ci.yaml to get this to run anywhere:

- name: Mac_ios native_ui_platform_view_tests_ios bringup: true recipe: devicelab/devicelab_drone presubmit: false timeout: 60 properties: tags: > ["devicelab", "ios", "mac"] task_name: native_ui_platform_view_tests_ios scheduler: luci

jmagman · 2022-06-09T22:35:27Z

dev/integration_tests/ios_platform_view_xcuitests/ios/Runner.xcodeproj/project.pbxproj

+			buildSettings = {
+				ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon;
+				CURRENT_PROJECT_VERSION = "$(FLUTTER_BUILD_NUMBER)";
+				DEVELOPMENT_TEAM = S8QB4VV633;


Remove the Flutter team.

jmagman · 2022-06-09T22:35:43Z

dev/integration_tests/ios_platform_view_xcuitests/ios/Runner.xcodeproj/project.pbxproj

+			buildSettings = {
+				ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon;
+				CURRENT_PROJECT_VERSION = "$(FLUTTER_BUILD_NUMBER)";
+				DEVELOPMENT_TEAM = S8QB4VV633;


jmagman

Changes for the integration test

hellohuanlin · 2022-06-09T23:34:08Z

Is there any reason not to add the tests to ios_platform_view_tests and reuse that project?

@jmagman The existing ios_platform_view_test calls enableFlutterDriverExtension (code here) in order to support flutter_driver, which disables keyboard for XCUITest.

I can look into if there's a way to host 2 Apps in the same project (maybe that's what a "flavor" can do?)

jmagman · 2022-06-10T17:26:14Z

Is there any reason not to add the tests to ios_platform_view_tests and reuse that project?

@jmagman The existing ios_platform_view_test calls enableFlutterDriverExtension (code here) in order to support flutter_driver, which disables keyboard for XCUITest.

I can look into if there's a way to host 2 Apps in the same project (maybe that's what a "flavor" can do?)

Oh sorry I see your comment about this from above. What if you used dart defines? Does this work?

      await flutter(
        'build',
        options: <String>[
          'ios',
          '-v',
          '--release',
          '--dart-define',
          'SKIP_DRIVER_EXTENSION=true',
          '--config-only',
        ],
      );

void main() {
  if (!bool.fromEnvironment('SKIP_DRIVER_EXTENSION')) {
    enableFlutterDriverExtension();
  }
  runApp(const MyApp());
}

jmagman · 2022-06-10T17:27:55Z

Or avoiding double negatives:

      await flutter(
        'build',
        options: <String>[
          'ios',
          '-v',
          '--release',
          '--dart-define',
          'ENABLE_DRIVER_EXTENSION=false',
          '--config-only',
        ],
      );

void main() {
  if (bool.fromEnvironment('ENABLE_DRIVER_EXTENSION'), defaultValue: true) {
    enableFlutterDriverExtension();
  }
  runApp(const MyApp());
}

keyonghan

Please add an ownership for this test here: https://github.com/flutter/flutter/blob/master/TESTOWNERS#L139

hellohuanlin · 2022-06-10T21:47:20Z

TESTOWNERS

@@ -195,7 +195,7 @@
 /dev/devicelab/bin/tasks/module_custom_host_app_name_test.dart @zanderso @flutter/tool
 /dev/devicelab/bin/tasks/module_host_with_custom_build_test.dart @zanderso @flutter/tool
 /dev/devicelab/bin/tasks/module_test.dart @zanderso @flutter/tool
-/dev/devicelab/bin/tasks/native_ui_tests_ios.dart @jmagman @flutter/engine


https://github.com/flutter/flutter/pull/103666/files

jmagman · 2022-06-11T00:27:32Z

.ci.yaml

+    presubmit: false
+    timeout: 60
+    properties:
+      tags: >
+        ["devicelab", "ios", "mac"]


I led you astray, this isn't a devicelab test, which means it has a tethered iOS device, it should be "hostonly" since it only runs in the simulator.

Suggested change

presubmit: false

timeout: 60

properties:

tags: >

["devicelab", "ios", "mac"]

timeout: 60

properties:

tags: >

["devicelab", "hostonly"]

do you mean ["hostonly", "ios", "mac"]?

No, I don't think so, I think that will make it require a machine with a tethered iOS device.
Like this:

flutter/.ci.yaml

Line 2394 in 712860d

["devicelab", "hostonly"]

If this is a host only test, the target name should be updated as Mac native_platform_view_ui_tests_ios, which controls what test bed to run.

The tags here should also be updated as ["devicelab", "hostonly"] as @jmagman suggested. Though this doesn't control the test bed, it affects performance metrics tags and build stats in bigquery. It would be good to reflect the truth of the test.

Thanks @keyonghan!

Interesting, I don't think any of the functionalities require code signing. (@cyanglaz to confirm).

This is the device lab testing so I guess it will require code signing for the app to run on devices, but if you just want to test the functionalities on simulator you can bypass code signing.

I think the Mac name before the test name indicates that it is to be tested on the simulator? @keyonghan is it correct?

Here the devicelab means tasks located under dev/devicelab/bin/tasks, which all share the same devicelab_drone recipes. It includes two types: the ones that need a devicelab testbed (which needs a physical phone connected), the other ones that run on a host only bot.

The Mac name will route all tests to the second type, whereas the Mac_ios will route tests to the first type.

The test added in this PR does run on a host only bot. You may want to take a look at the log to see if any findings: https://logs.chromium.org/logs/flutter/led/keyonghan_google.com/943f3399cb63bc7e1dc12ccdfcf310d85888f7de9f910789f46af2ed8a9f93cf/+/u/run_native_platform_view_ui_tests_ios/test_stdout

From the bottom of this link, it looks like the error is in Android:

Task result: { "success": false, "reason": "Task failed: DeviceException: The ANDROID_SDK_ROOT environment variable is missing. The variable must point to the Android SDK directory containing platform-tools." }

It also mention about code signing tho, but not part of the failure reason.

jmagman · 2022-06-11T00:28:33Z

dev/devicelab/bin/tasks/native_platform_view_ui_tests_ios.dart

+  deviceOperatingSystem = DeviceOperatingSystem.ios;
+


And since this is "hostonly" and doesn't need a tethered device, you should be able to delete this line.

Suggested change

deviceOperatingSystem = DeviceOperatingSystem.ios;

dev/integration_tests/ios_platform_view_tests/ios/PlatformViewUITests/PlatformViewUITests.m

jmagman · 2022-06-11T00:29:32Z

dev/integration_tests/ios_platform_view_tests/ios/Runner.xcodeproj/project.pbxproj

@@ -54,6 +73,13 @@
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 		};
+		E09898E32853E9F000064317 /* Frameworks */ = {


Was this empty frameworks build phase added accidentally? If Xcode did it automatically it's fine.

it's automatically added by Xcode when I add the test target.

jmagman

LGTM

jmagman · 2022-06-14T01:24:57Z

Re: the test failures, rebase past #105927

…cused (flutter/flutter#105050)

…focused (flutter/flutter#105050)

…lutter#105050)

…focused (flutter/flutter#105050)

…cused (flutter/flutter#105050)

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label May 31, 2022

hellohuanlin force-pushed the huan_platform_view_send_channel_message_in_framework branch from 849fcd0 to 37eba75 Compare May 31, 2022 20:58

hellohuanlin requested review from jmagman and cyanglaz May 31, 2022 21:04

hellohuanlin marked this pull request as ready for review May 31, 2022 21:04

jmagman reviewed May 31, 2022

View reviewed changes

cyanglaz approved these changes May 31, 2022

View reviewed changes

cyanglaz suggested changes May 31, 2022

View reviewed changes

hellohuanlin requested review from jmagman and cyanglaz June 1, 2022 21:51

jmagman approved these changes Jun 2, 2022

View reviewed changes

hellohuanlin requested a review from jmagman June 6, 2022 23:11

jmagman reviewed Jun 9, 2022

View reviewed changes

jmagman requested changes Jun 9, 2022

View reviewed changes

hellohuanlin force-pushed the huan_platform_view_send_channel_message_in_framework branch from 68758f9 to 79b49e6 Compare June 10, 2022 21:33

hellohuanlin requested a review from keyonghan as a code owner June 10, 2022 21:33

keyonghan reviewed Jun 10, 2022

View reviewed changes

hellohuanlin force-pushed the huan_platform_view_send_channel_message_in_framework branch from 81596d5 to 1682d5e Compare June 10, 2022 21:42

hellohuanlin commented Jun 10, 2022

View reviewed changes

jmagman reviewed Jun 11, 2022

View reviewed changes

jmagman approved these changes Jun 14, 2022

View reviewed changes

hellohuanlin force-pushed the huan_platform_view_send_channel_message_in_framework branch from 9fd6451 to fa67e7c Compare June 14, 2022 19:11

hellohuanlin added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 16, 2022

hellohuanlin added 16 commits June 22, 2022 11:50

[platform_view]Send platform message when platform view is focused

7c96ac0

nits

0cf4af5

add command line option for driver tests

d6ad615

add fliutter page for focus test

d818763

add text field platform view

9a9cabd

add xcuitest

9a2df74

update license

14bd33c

add ui test for focus

babaa34

add ci task runner

ee38b98

nit

d9ee27a

update test owners

5810072

remove the deleted test owner info

8199f35

update test ci code

7633816

update macios to mac in ci

72e1537

null check for _controller

da8faf3

pass controller in focus callback

d7667a4

hellohuanlin force-pushed the huan_platform_view_send_channel_message_in_framework branch from be0e5e1 to d7667a4 Compare June 22, 2022 18:50

auto-submit bot merged commit 0dd0c2e into flutter:master Jun 23, 2022

engine-flutter-autoroll mentioned this pull request Jun 23, 2022

Roll Flutter from 60f30e5d3ed4 to a30012b27505 (8 revisions) flutter/plugins#6033

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 23, 2022

0dd0c2e [platform_view]Send platform message when platform view is fo…

c8b9697

…cused (flutter/flutter#105050)

engine-flutter-autoroll mentioned this pull request Jun 23, 2022

Roll Flutter from 2540547f06d6 to a30012b27505 (11 revisions) flutter/packages#2273

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 23, 2022

0dd0c2edc [platform_view]Send platform message when platform view is …

80ee570

…focused (flutter/flutter#105050)

engine-flutter-autoroll mentioned this pull request Jun 23, 2022

Roll Flutter from 2540547f06d6 to fe41b6abcd53 (12 revisions) flutter/packages#2274

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 23, 2022

0dd0c2edc [platform_view]Send platform message when platform view is …

8f96a6b

…focused (flutter/flutter#105050)

jmagman mentioned this pull request Jul 7, 2022

Mac native_platform_view_ui_tests_ios has been failing consistently #106745

Closed

camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022

[platform_view]Send platform message when platform view is focused (f…

3c37a27

…lutter#105050)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022

0dd0c2edc [platform_view]Send platform message when platform view is …

4daaa9a

…focused (flutter/flutter#105050)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022

0dd0c2e [platform_view]Send platform message when platform view is fo…

f9d8d4b

…cused (flutter/flutter#105050)

jmagman mentioned this pull request Sep 21, 2022

UiKitView should participate in the Flutter focus tree #34187

Closed

		@@ -0,0 +1,49 @@
		// Copyright 2014 The Flutter Authors. All rights reserved.

[platform_view]Send platform message when platform view is focused #105050

[platform_view]Send platform message when platform view is focused #105050

Conversation

hellohuanlin commented May 31, 2022 • edited Loading

Pre-launch Checklist

jmagman commented May 31, 2022

jmagman left a comment

Choose a reason for hiding this comment

cyanglaz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellohuanlin commented Jun 1, 2022 • edited Loading

jmagman left a comment

Choose a reason for hiding this comment

hellohuanlin commented Jun 2, 2022 • edited Loading

hellohuanlin commented Jun 6, 2022

jmagman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagman left a comment

Choose a reason for hiding this comment

hellohuanlin commented Jun 9, 2022

jmagman commented Jun 10, 2022

jmagman commented Jun 10, 2022

keyonghan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagman left a comment

Choose a reason for hiding this comment

jmagman commented Jun 14, 2022

hellohuanlin commented May 31, 2022 •

edited

Loading

cyanglaz left a comment •

edited

Loading

hellohuanlin commented Jun 1, 2022 •

edited

Loading

hellohuanlin commented Jun 2, 2022 •

edited

Loading