[web] retrieve hostElement for an implicit view #53296
Conversation
There was a problem hiding this comment.
@p-mazhnik thanks for fixing this, I think this API makes sense (even though we'd like to get rid of the implicit view eventually).
Could you create an issue describing what this is solving, for the readers of the future, and link it with this PR?
| @@ -99,6 +99,10 @@ class FlutterViewManager { | |||
| return _jsViewOptions[viewId]; | |||
| } | |||
|
|
|||
| DomElement? getHostElement(int viewId) { | |||
There was a problem hiding this comment.
This method could use a bit of /// Documentation.
|
@ditman could you please re-review and assign a second reviewer? |
| @@ -99,6 +99,11 @@ class FlutterViewManager { | |||
| return _jsViewOptions[viewId]; | |||
| } | |||
|
|
|||
| /// Returns the host element in which the Flutter view associated to [viewId] is embedded (if any). | |||
| DomElement? getHostElement(int viewId) { | |||
There was a problem hiding this comment.
Do we have a clear definition of "host element"? If not, then the docs seem recursive. The name getHostElement already implies that it returns some "host element". There are two candidates for what people might think "host element" refers to:
- The
<flutter-view>element. - The direct parent of
<flutter-view>.
The docs should explain which one this function returns.
There was a problem hiding this comment.
I'd expect the hostElement to be the parent element of the flutter view.
There was a problem hiding this comment.
That is what we have for embeddingStrategy.hostElement:
Also in the Flutter docs:
HTML Element into which Flutter renders the app.
I think rootElement is used for the <flutter-view>.
There was a problem hiding this comment.
Doc in the code can be more specific though. How about updating doc for the embeddingStrategy.hostElement to be the following:
/// The dom element in which the Flutter view is embedded.
/// This element is the direct parent element of the <flutter-view> element.And then for the current function just leave it like this:
/// Returns the dom element in which the Flutter view associated to [viewId] is embedded (if any).There was a problem hiding this comment.
@p-mazhnik is the "(if any)" true? I think there's always a "parent element of the flutter view" (unless viewId is wrong)
There was a problem hiding this comment.
Ah, I meant that there might not be a Flutter view associated with the provided viewId. I see now how this could be misinterpreted. Let's just omit it:
Returns the dom element in which the Flutter view associated to [viewId] is embedded
There was a problem hiding this comment.
The revised doc comment is clearer. Thank you! However, it is more important to get the docs right in dart:ui_web, which is part of the public API. I have some suggestions there.
| @@ -99,6 +99,11 @@ class FlutterViewManager { | |||
| return _jsViewOptions[viewId]; | |||
| } | |||
|
|
|||
| /// Returns the dom element in which the Flutter view associated to [viewId] is embedded. | |||
| @@ -99,6 +99,11 @@ class FlutterViewManager { | |||
| return _jsViewOptions[viewId]; | |||
| } | |||
|
|
|||
| /// Returns the host element in which the Flutter view associated to [viewId] is embedded (if any). | |||
| DomElement? getHostElement(int viewId) { | |||
There was a problem hiding this comment.
The revised doc comment is clearer. Thank you! However, it is more important to get the docs right in dart:ui_web, which is part of the public API. I have some suggestions there.
| @@ -17,13 +17,15 @@ class FlutterViewManagerProxy { | |||
| // The proxied viewManager instance. | |||
| final FlutterViewManager _viewManager; | |||
|
|
|||
| /// Returns the `hostElement` configuration value passed from JS when `viewId` was added. | |||
| /// Returns the host element for [viewId]. | |||
There was a problem hiding this comment.
Add one empty line between paragraphs.
| /// Returns the `hostElement` configuration value passed from JS when `viewId` was added. | ||
| /// Returns the host element for [viewId]. | ||
| /// In a multi-view app, it is the `hostElement` value passed from JS when [viewId] was added. | ||
| /// In an implicit view app, it is either the provided `hostElement` or the <body> element. |
There was a problem hiding this comment.
The implicitView-based element embedding is superseded by multi-view, so I wouldn't bother documenting it. Instead, we will deprecate it and remove it so we don't have multiple ways to do the same thing.
As for multi-view, we don't want to call it "multi-view app". Instead, we'll have full-page mode and add-to-app mode, and being able to add multiple view in the add-to-app mode is simply one of many capabilities of that mode. I'd reword this paragraph to reflect that, such as:
/// In the full-page mode, the host element is the `<body>` element of the page
/// and the view is the one and only [PlatformDispatcher.implicitView].
///
/// In the add-to-app mode, the host element is the value of `hostElement`
/// provided when creating the view.
There was a problem hiding this comment.
This is much better, updated!
|
Let's go! |
…151674) flutter/engine@36dccf7...6b36113 2024-07-12 [email protected] Roll Skia from 9ea5603242c1 to 923034db7728 (1 revision) (flutter/engine#53841) 2024-07-12 [email protected] Roll Dart SDK from 797d3df745d1 to e986ed9d0bc1 (1 revision) (flutter/engine#53840) 2024-07-12 [email protected] Roll Skia from 7a91f0a4b7a0 to 9ea5603242c1 (1 revision) (flutter/engine#53839) 2024-07-12 [email protected] Roll Skia from 9529b8ad9e45 to 7a91f0a4b7a0 (2 revisions) (flutter/engine#53838) 2024-07-12 [email protected] Roll Skia from 38f355af4f36 to 9529b8ad9e45 (1 revision) (flutter/engine#53837) 2024-07-12 [email protected] Roll Skia from 14c8d318615d to 38f355af4f36 (1 revision) (flutter/engine#53836) 2024-07-12 [email protected] Roll Skia from ddf045505cb9 to 14c8d318615d (1 revision) (flutter/engine#53835) 2024-07-12 [email protected] Roll Fuchsia Linux SDK from 0e47sje8wkJ08sGJ6... to VlZIUknh6dnA23owe... (flutter/engine#53834) 2024-07-12 [email protected] Manual roll Dart SDK from fb546f313557 to 797d3df745d1 (8 revisions) (flutter/engine#53832) 2024-07-11 [email protected] [Impeller] Ensure full transform is applied to text contents (flutter/engine#53819) 2024-07-11 [email protected] Roll ICU from 43953f57b037 to 9408c6fd4a39 (6 revisions) (flutter/engine#53827) 2024-07-11 [email protected] Update Life-of-a-Flutter-Frame.md (flutter/engine#53829) 2024-07-11 [email protected] Update Setting-up-the-Engine-development-environment.md (flutter/engine#53828) 2024-07-11 [email protected] [web] retrieve hostElement for an implicit view (flutter/engine#53296) 2024-07-11 [email protected] [dart:ui] remove expensive index assertion in Vertices. (flutter/engine#53558) 2024-07-11 [email protected] [Impeller] Enable fixed-rate compression support in Vulkan. (flutter/engine#53292) 2024-07-11 [email protected] Roll Skia from 037d5f8a727f to ddf045505cb9 (1 revision) (flutter/engine#53824) 2024-07-11 [email protected] Add instructions for source debugging with Xcode when using RBE. (flutter/engine#53822) 2024-07-11 [email protected] Roll Skia from 004c81523e44 to 037d5f8a727f (1 revision) (flutter/engine#53818) 2024-07-11 [email protected] [Impeller] move more aiks tests to DL. (flutter/engine#53792) 2024-07-11 [email protected] Roll Skia from ec4a1e03f7b0 to 004c81523e44 (23 revisions) (flutter/engine#53813) 2024-07-11 [email protected] Roll Skia from 2783ba54bf8e to ec4a1e03f7b0 (9 revisions) (flutter/engine#53797) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 0e47sje8wkJ0 to VlZIUknh6dnA 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
This PR updates
ui_web.views.getHostElementAPI to also returnhostElementfor implicit view.Resolves flutter/flutter#150075