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

[skwasm] Fix platform view placement. #53845

Merged
merged 1 commit into from
Jul 12, 2024
Merged

[skwasm] Fix platform view placement. #53845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 15 additions & 28 deletions lib/web_ui/lib/src/engine/layers.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,14 +385,14 @@ class ShaderMaskOperation implements LayerOperation {
}

class PlatformView {
PlatformView(this.viewId, this.size, this.styling);
PlatformView(this.viewId, this.bounds, this.styling);

int viewId;
final int viewId;

// The bounds of this platform view, in the layer's local coordinate space.
ui.Size size;
final ui.Rect bounds;

PlatformViewStyling styling;
final PlatformViewStyling styling;
}

sealed class LayerSlice {
Expand Down Expand Up @@ -606,20 +606,8 @@ class LayerBuilder {

PlatformViewStyling? _memoizedPlatformViewStyling;

PlatformViewStyling _createPlatformViewStyling() {
final PlatformViewStyling? innerStyling = operation?.createPlatformViewStyling();
final PlatformViewStyling? outerStyling = parent?.platformViewStyling;
if (innerStyling == null) {
return outerStyling ?? const PlatformViewStyling();
}
if (outerStyling == null) {
return innerStyling;
}
return PlatformViewStyling.combine(outerStyling, innerStyling);
}

PlatformViewStyling get platformViewStyling {
return _memoizedPlatformViewStyling ??= _createPlatformViewStyling();
return _memoizedPlatformViewStyling ??= operation?.createPlatformViewStyling() ?? const PlatformViewStyling();
}

(ui.PictureRecorder, SceneCanvas) _createRecorder(ui.Rect rect) {
Expand Down Expand Up @@ -713,16 +701,7 @@ class LayerBuilder {
}) {
final ui.Rect bounds = ui.Rect.fromLTWH(offset.dx, offset.dy, width, height);
platformViewRect = platformViewRect?.expandToInclude(bounds) ?? bounds;
final PlatformViewStyling layerStyling = platformViewStyling;
final PlatformViewStyling viewStyling = offset == ui.Offset.zero
? layerStyling
: PlatformViewStyling.combine(
layerStyling,
PlatformViewStyling(
position: PlatformViewPosition.offset(offset),
),
);
pendingPlatformViews.add(PlatformView(viewId, ui.Size(width, height), viewStyling));
pendingPlatformViews.add(PlatformView(viewId, bounds, platformViewStyling));
}

void mergeLayer(PictureEngineLayer layer) {
Expand All @@ -738,7 +717,15 @@ class LayerBuilder {
if (occlusionRect != null) {
platformViewRect = platformViewRect?.expandToInclude(occlusionRect) ?? occlusionRect;
}
pendingPlatformViews.addAll(slice.views);
for (final PlatformView view in slice.views) {
// Merge the platform view styling of this layer with the nested
// platform views.
final PlatformViewStyling styling = PlatformViewStyling.combine(
platformViewStyling,
view.styling,
);
pendingPlatformViews.add(PlatformView(view.viewId, view.bounds, styling));
}
}
}
}
Expand Down
29 changes: 17 additions & 12 deletions lib/web_ui/lib/src/engine/scene_view.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ class EngineSceneView {
}
}
container ??= PlatformViewContainer(view.viewId);
container.size = view.size;
container.bounds = view.bounds;
container.styling = view.styling;
container.updateContents();
newContainers.add(container);
Expand Down Expand Up @@ -281,7 +281,7 @@ final class PlatformViewContainer extends SliceContainer {

final int viewId;
PlatformViewStyling? _styling;
ui.Size? _size;
ui.Rect? _bounds;
bool _dirty = false;

@override
Expand All @@ -294,9 +294,9 @@ final class PlatformViewContainer extends SliceContainer {
}
}

set size(ui.Size size) {
if (_size != size) {
_size = size;
set bounds(ui.Rect bounds) {
if (_bounds != bounds) {
_bounds = bounds;
_dirty = true;
}
}
Expand All @@ -305,23 +305,28 @@ final class PlatformViewContainer extends SliceContainer {
@override
void updateContents() {
assert(_styling != null);
assert(_size != null);
assert(_bounds != null);
if (_dirty) {
final DomCSSStyleDeclaration style = container.style;
final double devicePixelRatio = EngineFlutterDisplay.instance.devicePixelRatio;
final double logicalWidth = _size!.width / devicePixelRatio;
final double logicalHeight = _size!.height / devicePixelRatio;
final double logicalWidth = _bounds!.width / devicePixelRatio;
final double logicalHeight = _bounds!.height / devicePixelRatio;
style.width = '${logicalWidth}px';
style.height = '${logicalHeight}px';
style.position = 'absolute';

final ui.Offset? offset = _styling!.position.offset;
final double logicalLeft = (offset?.dx ?? 0) / devicePixelRatio;
final double logicalTop = (offset?.dy ?? 0) / devicePixelRatio;
final PlatformViewPosition position = PlatformViewPosition.combine(
Copy link
Contributor

Choose a reason for hiding this comment

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

Unable to comment on the internals of this class, so commenting here. Just a nit that we should probably put transform first and offset second in the order of fields, arguments, and various expressions that involve both because transform is applied first and offset is applied second. For example, if you have a transform layer and you do addPlatformView(offset), you first apply the transform and then the offset. Unless I'm misunderstanding what these fields do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an "either-or" class, where either the offset or the transform are specified but never both. If there is an offset and a transform that are combined, they just become a transform only (the offset is applied properly to the transform). As such, there is no ordering implied here within this class.

_styling!.position,
PlatformViewPosition.offset(_bounds!.topLeft),
);

final ui.Offset offset = position.offset ?? ui.Offset.zero;
final double logicalLeft = offset.dx / devicePixelRatio;
final double logicalTop = offset.dy / devicePixelRatio;
Copy link
Contributor

Choose a reason for hiding this comment

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

Up to you, but local variable types are now optional. It might be good to take advantage of it in individual PRs so we don't have to do a huge type rewrite later.

style.left = '${logicalLeft}px';
style.top = '${logicalTop}px';

final Matrix4? transform = _styling!.position.transform;
final Matrix4? transform = position.transform;
style.transform = transform != null ? float64ListToCssTransform3d(transform.storage) : '';
style.opacity = _styling!.opacity != 1.0 ? '${_styling!.opacity}' : '';
// TODO(jacksongardner): Implement clip styling for platform views
Expand Down
18 changes: 5 additions & 13 deletions lib/web_ui/test/engine/scene_builder_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,7 @@ void testMain() {
expect(slices.length, 2);
expect(slices[0], pictureSliceWithRect(pictureRect));
expect(slices[1], platformViewSliceWithViews(<PlatformView>[
PlatformView(1, platformViewRect.size, PlatformViewStyling(
position: PlatformViewPosition.offset(platformViewRect.topLeft)
))
PlatformView(1, platformViewRect, const PlatformViewStyling())
]));
});

Expand All @@ -95,9 +93,7 @@ void testMain() {
final List<LayerSlice> slices = scene.rootLayer.slices;
expect(slices.length, 2);
expect(slices[0], platformViewSliceWithViews(<PlatformView>[
PlatformView(1, platformViewRect.size, PlatformViewStyling(
position: PlatformViewPosition.offset(platformViewRect.topLeft)
))
PlatformView(1, platformViewRect, const PlatformViewStyling())
]));
expect(slices[1], pictureSliceWithRect(pictureRect));
});
Expand All @@ -122,9 +118,7 @@ void testMain() {
expect(slices.length, 3);
expect(slices[0], pictureSliceWithRect(pictureRect1));
expect(slices[1], platformViewSliceWithViews(<PlatformView>[
PlatformView(1, platformViewRect.size, PlatformViewStyling(
position: PlatformViewPosition.offset(platformViewRect.topLeft)
))
PlatformView(1, platformViewRect, const PlatformViewStyling())
]));
expect(slices[2], pictureSliceWithRect(pictureRect2));
});
Expand Down Expand Up @@ -153,9 +147,7 @@ void testMain() {
expect(slices.length, 2);
expect(slices[0], pictureSliceWithRect(const ui.Rect.fromLTRB(50, 50, 200, 200)));
expect(slices[1], platformViewSliceWithViews(<PlatformView>[
PlatformView(1, platformViewRect.size, PlatformViewStyling(
position: PlatformViewPosition.offset(platformViewRect.topLeft)
))
PlatformView(1, platformViewRect, const PlatformViewStyling())
]));
});
});
Expand Down Expand Up @@ -219,7 +211,7 @@ class PlatformViewSliceMatcher extends Matcher {
if (expectedView.viewId != actualView.viewId) {
return false;
}
if (expectedView.size != actualView.size) {
if (expectedView.bounds != actualView.bounds) {
return false;
}
if (expectedView.styling != actualView.styling) {
Expand Down
6 changes: 2 additions & 4 deletions lib/web_ui/test/engine/scene_view_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,10 +152,8 @@ void testMain() {

final PlatformView platformView = PlatformView(
1,
const ui.Size(100, 120),
const PlatformViewStyling(
position: PlatformViewPosition.offset(ui.Offset(50, 80)),
));
const ui.Rect.fromLTWH(50, 80, 100, 120),
const PlatformViewStyling());
final EngineRootLayer rootLayer = EngineRootLayer();
rootLayer.slices.add(PlatformViewSlice(<PlatformView>[platformView], null));
final EngineScene scene = EngineScene(rootLayer);
Expand Down
43 changes: 42 additions & 1 deletion lib/web_ui/test/ui/platform_view_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,47 @@ Future<void> testMain() async {
await matchGoldenFile('platformview_transformed.png', region: region);
});

test('offset platformview', () async {
await _createPlatformView(1, platformViewType);

final ui.PictureRecorder recorder = ui.PictureRecorder();
final ui.Canvas canvas = ui.Canvas(recorder);
canvas.drawCircle(
const ui.Offset(50, 50),
50,
ui.Paint()
..style = ui.PaintingStyle.fill
..color = const ui.Color(0xFFFF0000)
);

final ui.Picture picture = recorder.endRecording();

final ui.SceneBuilder sb = ui.SceneBuilder();
sb.pushOffset(50, 50);
sb.addPicture(const ui.Offset(100, 100), picture);

final ui.EngineLayer retainedPlatformView = sb.pushOffset(50, 50);
sb.addPlatformView(
1,
offset: const ui.Offset(125, 125),
width: 50,
height: 50,
);
await renderScene(sb.build());

await matchGoldenFile('platformview_offset.png', region: region);

final ui.SceneBuilder sb2 = ui.SceneBuilder();
sb2.pushOffset(0, 0);
sb2.addPicture(const ui.Offset(100, 100), picture);

sb2.addRetained(retainedPlatformView);
await renderScene(sb2.build());

await matchGoldenFile('platformview_offset_moved.png', region: region);
});


test('platformview with opacity', () async {
await _createPlatformView(1, platformViewType);

Expand All @@ -149,7 +190,7 @@ Future<void> testMain() async {
sb.pushOffset(0, 0);
sb.addPicture(const ui.Offset(100, 100), recorder.endRecording());

sb.pushOpacity(127);
sb.pushOpacity(127, offset: const ui.Offset(50, 50));
sb.addPlatformView(
1,
offset: const ui.Offset(125, 125),
Expand Down
Loading