From 9db5457746a127e93738b4757737fe6d030f5f57 Mon Sep 17 00:00:00 2001 From: Jackson Gardner Date: Fri, 12 Jul 2024 12:37:18 -0700 Subject: [PATCH] [skwasm] Fix platform view placement. Previously, each platform view contained styling/placement information from the entire stack of the layer builder. This caused issues when using `addRetained`, since it would contain stale styling/placement information from its old parent layers. I have changed it so that platform views only contain local styling information, and that styling is combined with the parent only when the layers are merged. --- lib/web_ui/lib/src/engine/layers.dart | 43 +++++++------------ lib/web_ui/lib/src/engine/scene_view.dart | 29 +++++++------ .../test/engine/scene_builder_test.dart | 18 +++----- lib/web_ui/test/engine/scene_view_test.dart | 6 +-- lib/web_ui/test/ui/platform_view_test.dart | 43 ++++++++++++++++++- 5 files changed, 81 insertions(+), 58 deletions(-) diff --git a/lib/web_ui/lib/src/engine/layers.dart b/lib/web_ui/lib/src/engine/layers.dart index 7d6940be76be5..ecccd02e7845c 100644 --- a/lib/web_ui/lib/src/engine/layers.dart +++ b/lib/web_ui/lib/src/engine/layers.dart @@ -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 { @@ -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) { @@ -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) { @@ -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)); + } } } } diff --git a/lib/web_ui/lib/src/engine/scene_view.dart b/lib/web_ui/lib/src/engine/scene_view.dart index 6f3031ae919df..65ee7480cba43 100644 --- a/lib/web_ui/lib/src/engine/scene_view.dart +++ b/lib/web_ui/lib/src/engine/scene_view.dart @@ -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); @@ -281,7 +281,7 @@ final class PlatformViewContainer extends SliceContainer { final int viewId; PlatformViewStyling? _styling; - ui.Size? _size; + ui.Rect? _bounds; bool _dirty = false; @override @@ -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; } } @@ -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( + _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; 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 diff --git a/lib/web_ui/test/engine/scene_builder_test.dart b/lib/web_ui/test/engine/scene_builder_test.dart index 0a80cb2cad987..69088f897e440 100644 --- a/lib/web_ui/test/engine/scene_builder_test.dart +++ b/lib/web_ui/test/engine/scene_builder_test.dart @@ -72,9 +72,7 @@ void testMain() { expect(slices.length, 2); expect(slices[0], pictureSliceWithRect(pictureRect)); expect(slices[1], platformViewSliceWithViews([ - PlatformView(1, platformViewRect.size, PlatformViewStyling( - position: PlatformViewPosition.offset(platformViewRect.topLeft) - )) + PlatformView(1, platformViewRect, const PlatformViewStyling()) ])); }); @@ -95,9 +93,7 @@ void testMain() { final List slices = scene.rootLayer.slices; expect(slices.length, 2); expect(slices[0], platformViewSliceWithViews([ - PlatformView(1, platformViewRect.size, PlatformViewStyling( - position: PlatformViewPosition.offset(platformViewRect.topLeft) - )) + PlatformView(1, platformViewRect, const PlatformViewStyling()) ])); expect(slices[1], pictureSliceWithRect(pictureRect)); }); @@ -122,9 +118,7 @@ void testMain() { expect(slices.length, 3); expect(slices[0], pictureSliceWithRect(pictureRect1)); expect(slices[1], platformViewSliceWithViews([ - PlatformView(1, platformViewRect.size, PlatformViewStyling( - position: PlatformViewPosition.offset(platformViewRect.topLeft) - )) + PlatformView(1, platformViewRect, const PlatformViewStyling()) ])); expect(slices[2], pictureSliceWithRect(pictureRect2)); }); @@ -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(1, platformViewRect.size, PlatformViewStyling( - position: PlatformViewPosition.offset(platformViewRect.topLeft) - )) + PlatformView(1, platformViewRect, const PlatformViewStyling()) ])); }); }); @@ -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) { diff --git a/lib/web_ui/test/engine/scene_view_test.dart b/lib/web_ui/test/engine/scene_view_test.dart index 38c88f5049e21..64229dfb01742 100644 --- a/lib/web_ui/test/engine/scene_view_test.dart +++ b/lib/web_ui/test/engine/scene_view_test.dart @@ -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], null)); final EngineScene scene = EngineScene(rootLayer); diff --git a/lib/web_ui/test/ui/platform_view_test.dart b/lib/web_ui/test/ui/platform_view_test.dart index 84ab3d1741701..321cf4bea32cb 100644 --- a/lib/web_ui/test/ui/platform_view_test.dart +++ b/lib/web_ui/test/ui/platform_view_test.dart @@ -132,6 +132,47 @@ Future 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); @@ -149,7 +190,7 @@ Future 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),