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

ditman · 2023-12-13T01:01:37Z

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. In those cases, a fix like this is needed (use the only documented way to obtain the Platform View contents from its viewId)

Issues

Fixes flutter/flutter#137287
Closes flutter/flutter#136548

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

The private `FlexHtmlElementView` widget that backs the `renderButton` method currently relies on internal web engine knowledge (a CSS selector) to work. This PR reimplements that bit of the `FlexHtmlElementView` so it uses standard framework APIs, and removes its reliance in undocumented behavior. ## Issues * Related to: flutter/engine#48960

mdebbar

You also need to change engine/platform_dispatcher.dart. It grabs platformViewMessageHandler from the implicit view.

Maybe we should make the PlatformViewMessageHandler a singleton? (since creating and disposing a platform view has nothing to do with a FlutterView).

lib/web_ui/lib/src/engine/platform_views/message_handler.dart

lib/web_ui/lib/src/engine/platform_views/content_manager.dart

mdebbar · 2023-12-14T18:42:05Z

That said, I like the this approach of deferring the insertion of platform views!

…renderer, right before injecting its slot.

* Make the PlatformViewsContentManager a singleton. * Modify the DOM Manager of a view so it can inject platformViews into the right spot. * Use the view.dom.injectPlatformView from renderers so naming is less iffy. * Use a slightly faster way to determine if a platform view is injected in a platformViewHostNode or not. * Break many more tests.

ditman · 2023-12-15T04:11:15Z

PTAL @mdebbar, I think I updated everything we discussed in the scrum today!

mdebbar

Looks good!! Ship it!

lib/web_ui/lib/src/engine/html/platform_view.dart

lib/web_ui/lib/src/engine/platform_views/message_handler.dart

lib/web_ui/lib/src/engine/view_embedder/dom_manager.dart

Co-authored-by: Mouad Debbar <[email protected]>

ditman · 2023-12-15T20:04:51Z

I'll update the tests, and land this. I tested all the example apps we have that use platform views and all seem to work normally.

ditman · 2023-12-19T03:35:27Z

lib/web_ui/lib/src/engine/view_embedder/dom_manager.dart

+    } catch (e) {
+      // Do not fail, many tests expect this to not crash!
+      domWindow.console.warn(e);
+    }


Not super thrilled about this, but it fixes everything... opinions @mdebbar/@yjbanov?

What exactly is throwing here? The only thing I can think of is the assertion in getSlottedContent, but I would expect that to work if the tests are setting everything up correctly.

It is exactly the getSlottedContent assertion (or an unexpected null value if we don't assert). There's a bunch of tests in the framework that are not setting everything up correctly indeed.

flutter-dashboard · 2023-12-19T03:39:39Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #48960 at sha a07a5d9

ditman · 2023-12-19T04:01:46Z

Gold tests are legitimately broken by this change.

flutter-dashboard · 2023-12-19T19:46:04Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #48960 at sha 62c0023

ditman · 2023-12-19T23:49:50Z

All right, I'm more confident now that I've fixed the goldens. The problem was the html and skwasm renderers not injecting the slotted contents at the right time/place.

ditman · 2023-12-19T23:57:31Z

PTAL people, I think this is somewhat ready to land no! LMK about the locations of the view.dom.injectPlatformView(id) calls, those should happen always that are needed, but as few times as possible!

harryterkelsen

LGTM

mdebbar

LGTM with a minor suggestion for the test.

lib/web_ui/test/engine/platform_dispatcher/platform_dispatcher_test.dart

PR comments.

ditman · 2023-12-21T04:15:12Z

If CI is happy, we're all happy. Go on auto-submit!

…ter/engine#48960)

…140519) Roll Flutter Engine from c70f0a495ace to 1b1b2a12a597 (32 revisions) flutter/engine@c70f0a4...1b1b2a1 2023-12-21 [email protected] Roll Dart SDK from 4a10533a4dc8 to bcf68d22f0fa (1 revision) (flutter/engine#49329) 2023-12-21 [email protected] Roll Dart SDK from 1d0c3ecd1349 to 4a10533a4dc8 (1 revision) (flutter/engine#49326) 2023-12-21 [email protected] Ensure sorted rects in ui.Canvas for legacy compatibility (flutter/engine#49309) 2023-12-21 [email protected] Roll Dart SDK from 63a452b45026 to 1d0c3ecd1349 (1 revision) (flutter/engine#49318) 2023-12-21 [email protected] [Impeller] Make IPLR files multi-platform (flutter/engine#49253) 2023-12-21 [email protected] [web] Defer injection of platform views until needed. (flutter/engine#48960) 2023-12-21 [email protected] Roll Dart SDK from 1aef027ec953 to 63a452b45026 (2 revisions) (flutter/engine#49311) 2023-12-21 [email protected] Roll Skia from 29917d8c97ca to 4b16117e94b2 (4 revisions) (flutter/engine#49310) 2023-12-21 [email protected] Reland "[Windows] Move to FlutterCompositor for rendering" (flutter/engine#49262) 2023-12-21 [email protected] Reland `[Impeller] new blur: refactored math and fixed expanded padding size` (flutter/engine#49302) 2023-12-20 [email protected] Multiview pipeline Pt. 1: Skip illegal render calls (flutter/engine#49266) 2023-12-20 [email protected] SemanticsUpdateBuilder: make all args non-null (flutter/engine#49148) 2023-12-20 [email protected] [Impeller] fixed Rect::Contains (flutter/engine#49294) 2023-12-20 [email protected] Roll Dart SDK from 97c3b7e1885a to 1aef027ec953 (1 revision) (flutter/engine#49295) 2023-12-20 [email protected] Revert "[Impeller] new blur: refactored math and fixed expanded padding size" (flutter/engine#49298) 2023-12-20 [email protected] [Impeller] new blur: refactored math and fixed expanded padding size (flutter/engine#49206) 2023-12-20 [email protected] Multi-view pointer event (flutter/engine#46213) 2023-12-20 [email protected] [web:multiview] Only call `Renderer.clearFragmentProgramCache` on hot restart (flutter/engine#48758) 2023-12-20 [email protected] Roll Skia from 9cb1bb1164ea to 29917d8c97ca (1 revision) (flutter/engine#49289) 2023-12-20 [email protected] [Impeller] Add interactive Blur+Clip AiksTest. (flutter/engine#49283) 2023-12-20 [email protected] [macos] FlutterKeyboardManager memory leak fix (flutter/engine#48824) 2023-12-20 [email protected] Don't guard Windows arm64 Dart SDK download on the release candidate flag (flutter/engine#49244) 2023-12-20 [email protected] Fix testAppExtensionLaunching for Xcode 15/iOS 17 (flutter/engine#49242) 2023-12-20 [email protected] Roll Skia from 8060d6b36066 to 9cb1bb1164ea (2 revisions) (flutter/engine#49288) 2023-12-20 [email protected] Roll Dart SDK from ed415d966d8a to 97c3b7e1885a (1 revision) (flutter/engine#49287) 2023-12-20 [email protected] Roll Skia from d0f09ad481f7 to 8060d6b36066 (1 revision) (flutter/engine#49285) 2023-12-20 [email protected] [release] Update release config (flutter/engine#49254) 2023-12-20 [email protected] Roll Dart SDK from 1732c4c92ccd to ed415d966d8a (1 revision) (flutter/engine#49274) 2023-12-20 [email protected] Roll Dart SDK from 4c59838945d9 to 1732c4c92ccd (1 revision) (flutter/engine#49269) 2023-12-20 [email protected] Sync lints with flutter/flutter (flutter/engine#49192) 2023-12-19 [email protected] [web] Enforce onDrawFrame/onBeginFrame render rule (flutter/engine#49214) 2023-12-19 [email protected] [Docs] Add more info about running tests on iOS (flutter/engine#48859) 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 [email protected],[email protected],[email protected] 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md ...

…lutter#140519) Roll Flutter Engine from c70f0a495ace to 1b1b2a12a597 (32 revisions) flutter/engine@c70f0a4...1b1b2a1 2023-12-21 [email protected] Roll Dart SDK from 4a10533a4dc8 to bcf68d22f0fa (1 revision) (flutter/engine#49329) 2023-12-21 [email protected] Roll Dart SDK from 1d0c3ecd1349 to 4a10533a4dc8 (1 revision) (flutter/engine#49326) 2023-12-21 [email protected] Ensure sorted rects in ui.Canvas for legacy compatibility (flutter/engine#49309) 2023-12-21 [email protected] Roll Dart SDK from 63a452b45026 to 1d0c3ecd1349 (1 revision) (flutter/engine#49318) 2023-12-21 [email protected] [Impeller] Make IPLR files multi-platform (flutter/engine#49253) 2023-12-21 [email protected] [web] Defer injection of platform views until needed. (flutter/engine#48960) 2023-12-21 [email protected] Roll Dart SDK from 1aef027ec953 to 63a452b45026 (2 revisions) (flutter/engine#49311) 2023-12-21 [email protected] Roll Skia from 29917d8c97ca to 4b16117e94b2 (4 revisions) (flutter/engine#49310) 2023-12-21 [email protected] Reland "[Windows] Move to FlutterCompositor for rendering" (flutter/engine#49262) 2023-12-21 [email protected] Reland `[Impeller] new blur: refactored math and fixed expanded padding size` (flutter/engine#49302) 2023-12-20 [email protected] Multiview pipeline Pt. 1: Skip illegal render calls (flutter/engine#49266) 2023-12-20 [email protected] SemanticsUpdateBuilder: make all args non-null (flutter/engine#49148) 2023-12-20 [email protected] [Impeller] fixed Rect::Contains (flutter/engine#49294) 2023-12-20 [email protected] Roll Dart SDK from 97c3b7e1885a to 1aef027ec953 (1 revision) (flutter/engine#49295) 2023-12-20 [email protected] Revert "[Impeller] new blur: refactored math and fixed expanded padding size" (flutter/engine#49298) 2023-12-20 [email protected] [Impeller] new blur: refactored math and fixed expanded padding size (flutter/engine#49206) 2023-12-20 [email protected] Multi-view pointer event (flutter/engine#46213) 2023-12-20 [email protected] [web:multiview] Only call `Renderer.clearFragmentProgramCache` on hot restart (flutter/engine#48758) 2023-12-20 [email protected] Roll Skia from 9cb1bb1164ea to 29917d8c97ca (1 revision) (flutter/engine#49289) 2023-12-20 [email protected] [Impeller] Add interactive Blur+Clip AiksTest. (flutter/engine#49283) 2023-12-20 [email protected] [macos] FlutterKeyboardManager memory leak fix (flutter/engine#48824) 2023-12-20 [email protected] Don't guard Windows arm64 Dart SDK download on the release candidate flag (flutter/engine#49244) 2023-12-20 [email protected] Fix testAppExtensionLaunching for Xcode 15/iOS 17 (flutter/engine#49242) 2023-12-20 [email protected] Roll Skia from 8060d6b36066 to 9cb1bb1164ea (2 revisions) (flutter/engine#49288) 2023-12-20 [email protected] Roll Dart SDK from ed415d966d8a to 97c3b7e1885a (1 revision) (flutter/engine#49287) 2023-12-20 [email protected] Roll Skia from d0f09ad481f7 to 8060d6b36066 (1 revision) (flutter/engine#49285) 2023-12-20 [email protected] [release] Update release config (flutter/engine#49254) 2023-12-20 [email protected] Roll Dart SDK from 1732c4c92ccd to ed415d966d8a (1 revision) (flutter/engine#49274) 2023-12-20 [email protected] Roll Dart SDK from 4c59838945d9 to 1732c4c92ccd (1 revision) (flutter/engine#49269) 2023-12-20 [email protected] Sync lints with flutter/flutter (flutter/engine#49192) 2023-12-19 [email protected] [web] Enforce onDrawFrame/onBeginFrame render rule (flutter/engine#49214) 2023-12-19 [email protected] [Docs] Add more info about running tests on iOS (flutter/engine#48859) 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 [email protected],[email protected],[email protected] 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md ...

ditman requested review from mdebbar and yjbanov December 13, 2023 01:01

github-actions bot added the platform-web Code specifically for the web engine label Dec 13, 2023

ditman changed the title ~~[web] Defer injection of platform views until really needed.~~ [web] Defer injection of platform views until needed. Dec 13, 2023

ditman mentioned this pull request Dec 13, 2023

[google_sign_in_web] Stop relying on framework internals. flutter/packages#5660

Merged

11 tasks

mdebbar reviewed Dec 14, 2023

View reviewed changes

lib/web_ui/lib/src/engine/platform_views/message_handler.dart Outdated Show resolved Hide resolved

lib/web_ui/lib/src/engine/platform_views/content_manager.dart Outdated Show resolved Hide resolved

ditman force-pushed the deferred-platform-view-injection branch from a46bdc2 to 60b7649 Compare December 15, 2023 01:27

ditman added 2 commits December 14, 2023 19:27

Inject contents of a platform view in the FlutterView of the current …

0ecf2ff

…renderer, right before injecting its slot.

ditman force-pushed the deferred-platform-view-injection branch from aeb3278 to 5b00e5a Compare December 15, 2023 03:28

Don't be so smart.

cc9c3a3

ditman requested a review from mdebbar December 15, 2023 04:11

mdebbar mentioned this pull request Dec 15, 2023

[web] Send flutterViewId with platform view messages flutter/flutter#136548

Closed

mdebbar approved these changes Dec 15, 2023

View reviewed changes

lib/web_ui/lib/src/engine/html/platform_view.dart Show resolved Hide resolved

lib/web_ui/lib/src/engine/platform_views/message_handler.dart Show resolved Hide resolved

lib/web_ui/lib/src/engine/view_embedder/dom_manager.dart Outdated Show resolved Hide resolved

Update lib/web_ui/lib/src/engine/view_embedder/dom_manager.dart

60549bf

Co-authored-by: Mouad Debbar <[email protected]>

ditman added 2 commits December 18, 2023 17:54

Get engine tests to compile at least.

e8af2c0

Swallow+log exception in dom.injectPlatformView

a07a5d9

ditman commented Dec 19, 2023

View reviewed changes

ditman marked this pull request as ready for review December 19, 2023 03:36

flutter-dashboard bot added the will affect goldens label Dec 19, 2023

ditman marked this pull request as draft December 19, 2023 04:13

ditman removed the will affect goldens label Dec 19, 2023

Attempt to cure goldens.

526e186

flutter-dashboard bot added the will affect goldens label Dec 19, 2023

ditman added 3 commits December 19, 2023 14:20

Fix html renderer.

73952d0

Fix skwasm renderer.

6a7fea9

Skwasm comments

fed4781

ditman requested review from mdebbar and harryterkelsen December 19, 2023 23:50

harryterkelsen approved these changes Dec 20, 2023

View reviewed changes

ditman added 2 commits December 20, 2023 11:55

Connect view dispose to metricsChanged.

df73550

Add test for view disposal sending events to the framework.

76e4b9d

ditman removed the will affect goldens label Dec 20, 2023

mdebbar approved these changes Dec 20, 2023

View reviewed changes

lib/web_ui/test/engine/platform_dispatcher/platform_dispatcher_test.dart Outdated Show resolved Hide resolved

ditman added 2 commits December 20, 2023 12:38

Do not use try/catch for injectPlatformView flow.

deba54d

PR comments.

Better error message.

e210595

ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 21, 2023

auto-submit bot merged commit 3837195 into flutter:main Dec 21, 2023

ditman deleted the deferred-platform-view-injection branch December 21, 2023 04:25

engine-flutter-autoroll mentioned this pull request Dec 21, 2023

Roll Flutter Engine from c70f0a495ace to 1b1b2a12a597 (32 revisions) flutter/flutter#140519

Merged

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

383719539 [web] Defer injection of platform views until needed. (flut…

3ffbcff

…ter/engine#48960)

ditman mentioned this pull request Jan 2, 2024

[pointer_interceptor_web] Breaking the packages roll. flutter/flutter#140834

Closed

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

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

Uh oh!

Conversation

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

Uh oh!

Issues

Pre-launch Checklist

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mdebbar commented Dec 14, 2023

Uh oh!

ditman commented Dec 15, 2023

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ditman commented Dec 15, 2023

Uh oh!

ditman Dec 19, 2023

Choose a reason for hiding this comment

Uh oh!

mdebbar Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

ditman Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Dec 19, 2023

Uh oh!

ditman commented Dec 19, 2023

Uh oh!

flutter-dashboard bot commented Dec 19, 2023

Uh oh!

ditman commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ditman commented Dec 19, 2023

Uh oh!

harryterkelsen left a comment

Choose a reason for hiding this comment

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ditman commented Dec 21, 2023

Uh oh!

Uh oh!

ditman commented Dec 13, 2023 •

edited

Loading

ditman commented Dec 19, 2023 •

edited

Loading