Skip to content
This repository has been archived by the owner on Feb 25, 2025. It is now read-only.

[web] retrieve hostElement for an implicit view #53296

Merged
merged 7 commits into from
Jul 11, 2024
Merged

[web] retrieve hostElement for an implicit view #53296

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
98f0685
[web] retrieve hostElement for an implicit view
p-mazhnik Jun 9, 2024
adb379b
add tests
p-mazhnik Jun 9, 2024
e48806c
update doc comments
p-mazhnik Jun 11, 2024
cdb4678
Merge branch 'main' into implicit-view-host-element
p-mazhnik Jun 12, 2024
812a40e
update doc comments
p-mazhnik Jul 10, 2024
b22fcf1
Merge branch 'main' into implicit-view-host-element
p-mazhnik Jul 10, 2024
6045c9f
update comments
p-mazhnik Jul 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/web_ui/lib/src/engine/view_embedder/flutter_view_manager.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,10 @@ class FlutterViewManager {
return _jsViewOptions[viewId];
}

DomElement? getHostElement(int viewId) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method could use a bit of /// Documentation.

Copy link
Contributor

@yjbanov yjbanov Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect the hostElement to be the parent element of the flutter view.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is what we have for embeddingStrategy.hostElement:

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

Lines 29 to 30 in 928d34f

/// The host element in which the Flutter view is embedded.
DomElement get hostElement;

Also in the Flutter docs:

HTML Element into which Flutter renders the app.

I think rootElement is used for the <flutter-view>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@p-mazhnik is the "(if any)" true? I think there's always a "parent element of the flutter view" (unless viewId is wrong)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return _viewData[viewId]?.embeddingStrategy.hostElement;
}

EngineFlutterView? findViewForElement(DomElement? element) {
const String viewRootSelector =
'${DomManager.flutterViewTagName}[${GlobalHtmlAttributes.flutterViewIdAttributeName}]';
Expand Down
2 changes: 1 addition & 1 deletion lib/web_ui/lib/ui_web/src/ui_web/flutter_views_proxy.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class FlutterViewManagerProxy {
/// can add their own custom HTML elements (for example: file inputs for the
/// file_selector plugin).
JSAny? getHostElement(int viewId) {
return _viewManager.getOptions(viewId)?.hostElement as JSAny?;
return _viewManager.getHostElement(viewId) as JSAny?;
}

/// Returns the `initialData` configuration value passed from JS when `viewId` was added.
Expand Down
26 changes: 24 additions & 2 deletions lib/web_ui/test/engine/view_embedder/flutter_views_proxy_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ Future<void> doTests() async {
}

setUp(() {
view = EngineFlutterView(platformDispatcher, createDomElement('div'));
viewId = view.viewId;
hostElement = createDomElement('div');
view = EngineFlutterView(platformDispatcher, hostElement);
viewId = view.viewId;
});

tearDown(() {
Expand All @@ -60,6 +60,28 @@ Future<void> doTests() async {

expect(element, hostElement);
});

test('can retrieve hostElement for an implicit view with default host element', () {
final view = EngineFlutterView.implicit(platformDispatcher, null);
final viewId = view.viewId;
viewManager.registerView(view);
addTearDown(() => viewManager.unregisterView(viewId));

final JSAny? element = views.getHostElement(viewId);

expect(element, domDocument.body);
});

test('can retrieve hostElement for an implicit view with custom host element', () {
final view = EngineFlutterView.implicit(platformDispatcher, hostElement);
final viewId = view.viewId;
viewManager.registerView(view);
addTearDown(() => viewManager.unregisterView(viewId));

final JSAny? element = views.getHostElement(viewId);

expect(element, hostElement);
});
});

group('getInitialData', () {
Expand Down