[web] Send `flutterViewId` with platform view messages #136548

mdebbar · 2023-10-13T17:28:50Z

This PR prepares web platform views for the multi-view world.

Depends on flutter/engine#46891
Part of #137287

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

ditman

Minor comments, but in general LGTM!

(this can't land until the engine version does, though, since this is using new message argument names that are currently not understood by the engine)

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

ditman · 2023-10-20T01:46:10Z

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

    PlatformViewCreationParams params,
+    int viewId,


Why isn't viewId part of the PlatformViewCreationParams? (This would be easy to change anyway)

PlatformViewLink and PlatformViewCreationParams are used by all platforms and I didn't want to step on their stuff. I also want to hear from @goderbauer about how other platforms are going to deal with this.

I didn't want to step on their stuff.

It's our stuff. We own this all together. :)

Adding the viewId to PlatformViewCreationParams sounds reasonable to me. I imagine other platforms will likely also need to know what FlutterView a PlatformView is associated with. @cbracken @loic-sharma have you thought about this in the context of your Desktop Platform view work.

One thing to note, though: A PlatformView could move from one view to another. When scrolling through this PR I didn't see anything that would update the view ID associated with the controller when this happens?

A PlatformView could move from one view to another.

Hm, that's an interesting case. In the web at least, since we'd need to move the actual DOM Content for platform_view (because each view has its own DOM shadow root), it'd be equivalent to destroying the view and recreating it from the factory elsewhere (in most cases).

I'm not sure how the framework/engine handles this case.

I hadn't given this much thought I terms of desktop platform views but agreed we'll likely want this for desktop as well. Thanks for cc'ing, will take a look.

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

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

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

goderbauer · 2023-10-20T19:53:46Z

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

    PlatformViewCreationParams params,
+    int viewId,


I didn't want to step on their stuff.

It's our stuff. We own this all together. :)

Adding the viewId to PlatformViewCreationParams sounds reasonable to me. I imagine other platforms will likely also need to know what FlutterView a PlatformView is associated with. @cbracken @loic-sharma have you thought about this in the context of your Desktop Platform view work.

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

goderbauer · 2023-10-20T19:56:14Z

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

    PlatformViewCreationParams params,
+    int viewId,


One thing to note, though: A PlatformView could move from one view to another. When scrolling through this PR I didn't see anything that would update the view ID associated with the controller when this happens?

goderbauer · 2023-10-20T19:57:34Z

packages/flutter/test/widgets/view_utils.dart

+import 'package:flutter/widgets.dart';
+import 'package:flutter_test/flutter_test.dart';
+
+extension WrapperlessWidgetTester on WidgetTester {


https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-using-extension

(I would just define this as a helper method until we're ready to actually implement this on WidgetTester itself)

ditman · 2023-10-25T02:35:01Z

Wow, thanks for adding support for moving the flutter view!

The testing seems to be an analyzer issue:

error â�¢ Missing concrete implementation of 'PlatformViewController.moveToFlutterView' â�¢ packages/flutter/test/services/fake_platform_views.dart:47:7

ditman · 2023-10-25T02:37:37Z

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

+        'platformViewId': platformViewId,
+        'viewId': flutterViewId,
+      };
+      await SystemChannels.platform_views.invokeMethod<void>('moveToFlutterView', args);


Suggested change

await SystemChannels.platform_views.invokeMethod<void>('moveToFlutterView', args);

await SystemChannels.platform_views.invokeMethod<void>('move', args);

👍 I'd call this move just to keep the same brevity of the create/dispose methods, but I don't have a strong opinion.

mdebbar · 2023-10-25T18:00:54Z

I'm still working on tests.. my latest commit broke some tests.

Here's what I think so far: in tests, platform view creation happens synchronously in PlatformViewLink.initState, while in real code, the platform view creation happens asynchronously. With my recent change, platform view creation moved from initState to build, causing the tests to fail. @goderbauer any suggestions on how to deal with this? Or ideas on how to restructure the code to avoid this issue?

Note: The reason for moving platform view creation (or controller creation, really) from initState to build is that we need View.of(context) now. Adding inherited widget dependencies isn't allowed in initState apparently (I'm hitting this error).

ditman · 2023-10-25T19:23:24Z

The reason for moving platform view creation (or controller creation, really) from initState to build is that we need View.of(context) now.

Ahhh, tricky tricky :/

loic-sharma · 2023-10-26T16:37:03Z

packages/flutter/lib/src/services/platform_views.dart

  Future<void> create({Size? size, Offset? position}) async {}

+  /// Moves the platform view to a new [FlutterView].
+  Future<void> moveToFlutterView(int flutterViewId) async {}


Could we update the PlatformViewController.viewId documentation on lines 1451-1457 to clarify that identifier is for the platform view (instead of a Flutter view)?

goderbauer · 2023-11-01T20:58:30Z

Missed this ping last week. Apologies.

Instead of moving it from initState to build could you move it to didChangeDependencies? That one runs right after initState and again whenever the dependencies change. It's the usual place to establish dependencies that you cannot establish in initState. The reason for not being able to establish dependencies in initState is that initState doesn't run again when dependencies change while didChangeDependencies does.

flutter-dashboard · 2023-11-21T21:04:23Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

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

goderbauer · 2023-12-05T23:14:49Z

(triage): @mdebbar said this is still on his radar in the next few weeks.

yjbanov · 2023-12-12T18:24:55Z

Attempting to look at it from 30000 feet: Can viewId be determined by the slot created using SceneBuilder.addPlatformView? Then we don't need extra API on the platform view creation side. A platform view can start as "detached", and it would be "attached" when the framework calls FlutterView.render passing it a Scene containing the platform view ID. To move the platform view to a different FlutterView, simply call render on the new FlutterView and pass the platform view ID there. The engine can assert that the same platform view does not appear in multiple FlutterViews simultaneously.

mdebbar · 2023-12-15T15:05:59Z

Update: this PR may become superseded by flutter/engine#48960

This PR defers the injection of the contents of a Platform View into the DOM of the page, until the Platform View is really needed by a renderer. This effectively means that a `platformView` will be injected into the DOM the first time that its `slot` is injected into the Shadow DOM of the Flutter web app. This makes passing a `(flutter)ViewId` parameter from the framework unnecessary, even in a multi-view app. The only cases in which this change might be breaking is those where an app tries to locate the just-created Platform View by looking into the DOM from the [`onPlatformViewCreated` callback](https://api.flutter.dev/flutter/widgets/HtmlElementView/onPlatformViewCreated.html). In those cases, [a fix like this](flutter/packages#5660) is needed (use the **only [documented way](https://api.flutter.dev/flutter/dart-ui_web/PlatformViewRegistry/getViewById.html)** to obtain the Platform View contents from its `viewId`) ## Issues Fixes flutter/flutter#137287 Closes flutter/flutter#136548 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

[web] Send flutterViewId with platform view messages

d867dc4

mdebbar requested a review from ditman October 13, 2023 17:28

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Oct 13, 2023

mdebbar added 3 commits October 13, 2023 15:14

add flutterViewId expectations in tests

af001e7

renaming

331d2c8

Merge branch 'main' into pv_view_id

e13bf5e

ditman reviewed Oct 19, 2023

View reviewed changes

packages/flutter/lib/src/widgets/_html_element_view_web.dart Show resolved Hide resolved

ditman mentioned this pull request Oct 20, 2023

[web] Support flutterViewId in platform view messages flutter/engine#46891

Merged

ditman reviewed Oct 20, 2023

View reviewed changes

mdebbar requested a review from goderbauer October 20, 2023 16:24

mdebbar added 2 commits October 20, 2023 12:29

minor changes

53a806c

Merge branch 'main' into pv_view_id

509b17c

goderbauer reviewed Oct 20, 2023

View reviewed changes

mdebbar added 3 commits October 24, 2023 11:50

Merge branch 'main' into tmp

fee412e

add flutterViewId to PlatformViewCreationParams

2ea58c1

refactor initialization of PlatformViewLink

21573b5

ditman reviewed Oct 25, 2023

View reviewed changes

loic-sharma reviewed Oct 26, 2023

View reviewed changes

ditman mentioned this pull request Dec 19, 2023

[web] Defer injection of platform views until needed. flutter/engine#48960

Merged

8 tasks

auto-submit bot closed this in flutter/engine#48960 Dec 21, 2023

mdebbar deleted the pv_view_id branch January 12, 2024 17:22

	await SystemChannels.platform_views.invokeMethod<void>('moveToFlutterView', args);
	await SystemChannels.platform_views.invokeMethod<void>('move', args);

[web] Send flutterViewId with platform view messages #136548

[web] Send flutterViewId with platform view messages #136548

Uh oh!

Conversation

mdebbar commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

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!

cbracken Oct 21, 2023 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ditman commented Oct 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdebbar commented Oct 25, 2023

Uh oh!

ditman commented Oct 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Nov 21, 2023

Uh oh!

goderbauer commented Dec 5, 2023

Uh oh!

yjbanov commented Dec 12, 2023

Uh oh!

mdebbar commented Dec 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[web] Send `flutterViewId` with platform view messages #136548

[web] Send `flutterViewId` with platform view messages #136548

mdebbar commented Oct 13, 2023 •

edited

Loading

cbracken Oct 21, 2023 •

edited

Loading

goderbauer commented Nov 1, 2023 •

edited

Loading