From 528f77dc99e43f3eb20698cb683c25705725ced0 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 13 Sep 2021 19:10:15 -0700 Subject: [PATCH] Opacity fix (#90017) * Make sure Opacity widgets/layers do not drop the offset --- packages/flutter/lib/src/rendering/layer.dart | 52 ++++++------ .../flutter/lib/src/rendering/proxy_box.dart | 16 +--- .../lib/src/rendering/proxy_sliver.dart | 8 +- .../flutter/test/rendering/debug_test.dart | 26 ++++++ .../flutter/test/rendering/layers_test.dart | 82 +++++++++++++++++++ .../test/rendering/proxy_box_test.dart | 8 +- .../test/rendering/proxy_sliver_test.dart | 8 +- .../flutter/test/widgets/opacity_test.dart | 31 +++++++ 8 files changed, 175 insertions(+), 56 deletions(-) diff --git a/packages/flutter/lib/src/rendering/layer.dart b/packages/flutter/lib/src/rendering/layer.dart index 085ee1bfbbd..5ae73b0a20f 100644 --- a/packages/flutter/lib/src/rendering/layer.dart +++ b/packages/flutter/lib/src/rendering/layer.dart @@ -1741,7 +1741,7 @@ class TransformLayer extends OffsetLayer { /// /// Try to avoid an [OpacityLayer] with no children. Remove that layer if /// possible to save some tree walks. -class OpacityLayer extends ContainerLayer { +class OpacityLayer extends OffsetLayer { /// Creates an opacity layer. /// /// The [alpha] property must be non-null before the compositing phase of @@ -1750,7 +1750,7 @@ class OpacityLayer extends ContainerLayer { int? alpha, Offset offset = Offset.zero, }) : _alpha = alpha, - _offset = offset; + super(offset: offset); /// The amount to multiply into the alpha channel. /// @@ -1764,55 +1764,53 @@ class OpacityLayer extends ContainerLayer { set alpha(int? value) { assert(value != null); if (value != _alpha) { + if (value == 255 || _alpha == 255) { + engineLayer = null; + } _alpha = value; markNeedsAddToScene(); } } - /// Offset from parent in the parent's coordinate system. - Offset? get offset => _offset; - Offset? _offset; - set offset(Offset? value) { - if (value != _offset) { - _offset = value; - markNeedsAddToScene(); - } - } - - @override - void applyTransform(Layer? child, Matrix4 transform) { - assert(child != null); - assert(transform != null); - transform.translate(offset!.dx, offset!.dy); - } - @override void addToScene(ui.SceneBuilder builder, [ Offset layerOffset = Offset.zero ]) { assert(alpha != null); bool enabled = firstChild != null; // don't add this layer if there's no child + if (!enabled) { + // TODO(dnfield): Remove this if/when we can fix https://github.com/flutter/flutter/issues/90004 + return; + } assert(() { enabled = enabled && !debugDisableOpacityLayers; return true; }()); - if (enabled) + final int realizedAlpha = alpha!; + // The type assertions work because the [alpha] setter nulls out the + // engineLayer if it would have changed type (i.e. changed to or from 255). + if (enabled && realizedAlpha < 255) { + assert(_engineLayer is ui.OpacityEngineLayer?); engineLayer = builder.pushOpacity( - alpha!, - offset: offset! + layerOffset, + realizedAlpha, + offset: offset + layerOffset, oldLayer: _engineLayer as ui.OpacityEngineLayer?, ); - else - engineLayer = null; + } else { + assert(_engineLayer is ui.OffsetEngineLayer?); + engineLayer = builder.pushOffset( + layerOffset.dx + offset.dx, + layerOffset.dy + offset.dy, + oldLayer: _engineLayer as ui.OffsetEngineLayer?, + ); + } addChildrenToScene(builder); - if (enabled) - builder.pop(); + builder.pop(); } @override void debugFillProperties(DiagnosticPropertiesBuilder properties) { super.debugFillProperties(properties); properties.add(IntProperty('alpha', alpha)); - properties.add(DiagnosticsProperty('offset', offset)); } } diff --git a/packages/flutter/lib/src/rendering/proxy_box.dart b/packages/flutter/lib/src/rendering/proxy_box.dart index 8c39deae414..adadf52fab4 100644 --- a/packages/flutter/lib/src/rendering/proxy_box.dart +++ b/packages/flutter/lib/src/rendering/proxy_box.dart @@ -843,7 +843,7 @@ class RenderOpacity extends RenderProxyBox { super(child); @override - bool get alwaysNeedsCompositing => child != null && (_alpha != 0 && _alpha != 255); + bool get alwaysNeedsCompositing => child != null && (_alpha > 0); int _alpha; @@ -897,12 +897,6 @@ class RenderOpacity extends RenderProxyBox { layer = null; return; } - if (_alpha == 255) { - // No need to keep the layer. We'll create a new one if necessary. - layer = null; - context.paintChild(child!, offset); - return; - } assert(needsCompositing); layer = context.pushOpacity(offset, _alpha, super.paint, oldLayer: layer as OpacityLayer?); } @@ -993,7 +987,7 @@ mixin RenderAnimatedOpacityMixin on RenderObjectWithChil _alpha = ui.Color.getAlphaFromOpacity(opacity.value); if (oldAlpha != _alpha) { final bool? didNeedCompositing = _currentlyNeedsCompositing; - _currentlyNeedsCompositing = _alpha! > 0 && _alpha! < 255; + _currentlyNeedsCompositing = _alpha! > 0; if (child != null && didNeedCompositing != _currentlyNeedsCompositing) markNeedsCompositingBitsUpdate(); markNeedsPaint(); @@ -1010,12 +1004,6 @@ mixin RenderAnimatedOpacityMixin on RenderObjectWithChil layer = null; return; } - if (_alpha == 255) { - // No need to keep the layer. We'll create a new one if necessary. - layer = null; - context.paintChild(child!, offset); - return; - } assert(needsCompositing); layer = context.pushOpacity(offset, _alpha!, super.paint, oldLayer: layer as OpacityLayer?); } diff --git a/packages/flutter/lib/src/rendering/proxy_sliver.dart b/packages/flutter/lib/src/rendering/proxy_sliver.dart index 00cf7fd6f79..520e0d1bba2 100644 --- a/packages/flutter/lib/src/rendering/proxy_sliver.dart +++ b/packages/flutter/lib/src/rendering/proxy_sliver.dart @@ -112,7 +112,7 @@ class RenderSliverOpacity extends RenderProxySliver { } @override - bool get alwaysNeedsCompositing => child != null && (_alpha != 0 && _alpha != 255); + bool get alwaysNeedsCompositing => child != null && (_alpha > 0); int _alpha; @@ -166,12 +166,6 @@ class RenderSliverOpacity extends RenderProxySliver { layer = null; return; } - if (_alpha == 255) { - // No need to keep the layer. We'll create a new one if necessary. - layer = null; - context.paintChild(child!, offset); - return; - } assert(needsCompositing); layer = context.pushOpacity( offset, diff --git a/packages/flutter/test/rendering/debug_test.dart b/packages/flutter/test/rendering/debug_test.dart index 3bc405840e4..dce60cc9f25 100644 --- a/packages/flutter/test/rendering/debug_test.dart +++ b/packages/flutter/test/rendering/debug_test.dart @@ -208,4 +208,30 @@ void main() { expect(error, isNull); debugPaintSizeEnabled = false; }); + + test('debugDisableOpacity keeps things in the right spot', () { + debugDisableOpacityLayers = true; + + final RenderDecoratedBox blackBox = RenderDecoratedBox( + decoration: const BoxDecoration(color: Color(0xff000000)), + child: RenderConstrainedBox( + additionalConstraints: BoxConstraints.tight(const Size.square(20.0)), + ), + ); + final RenderOpacity root = RenderOpacity( + opacity: .5, + child: blackBox, + ); + layout(root, phase: EnginePhase.compositingBits); + + final OffsetLayer rootLayer = OffsetLayer(offset: Offset.zero); + final PaintingContext context = PaintingContext( + rootLayer, + const Rect.fromLTWH(0, 0, 500, 500), + ); + root.paint(context, const Offset(40, 40)); + final OpacityLayer opacityLayer = rootLayer.firstChild! as OpacityLayer; + expect(opacityLayer.offset, const Offset(40, 40)); + debugDisableOpacityLayers = false; + }); } diff --git a/packages/flutter/test/rendering/layers_test.dart b/packages/flutter/test/rendering/layers_test.dart index f8845a662a8..bade2a540f9 100644 --- a/packages/flutter/test/rendering/layers_test.dart +++ b/packages/flutter/test/rendering/layers_test.dart @@ -537,6 +537,53 @@ void main() { expect(() => holder.layer = layer, throwsAssertionError); }); + + test('OpacityLayer does not push an OffsetLayer if there are no children', () { + final OpacityLayer layer = OpacityLayer(alpha: 128); + final FakeSceneBuilder builder = FakeSceneBuilder(); + layer.addToScene(builder); + expect(builder.pushedOpacity, false); + expect(builder.pushedOffset, false); + expect(builder.addedPicture, false); + expect(layer.engineLayer, null); + + layer.append(PictureLayer(Rect.largest)..picture = FakePicture()); + + builder.reset(); + layer.addToScene(builder); + + expect(builder.pushedOpacity, true); + expect(builder.pushedOffset, false); + expect(builder.addedPicture, true); + expect(layer.engineLayer, isA()); + + builder.reset(); + + layer.alpha = 200; + expect(layer.engineLayer, isA()); + + layer.alpha = 255; + expect(layer.engineLayer, null); + + builder.reset(); + layer.addToScene(builder); + + expect(builder.pushedOpacity, false); + expect(builder.pushedOffset, true); + expect(builder.addedPicture, true); + expect(layer.engineLayer, isA()); + + layer.alpha = 200; + expect(layer.engineLayer, null); + + builder.reset(); + layer.addToScene(builder); + + expect(builder.pushedOpacity, true); + expect(builder.pushedOffset, false); + expect(builder.addedPicture, true); + expect(layer.engineLayer, isA()); + }); } class FakeEngineLayer extends Fake implements EngineLayer { @@ -568,3 +615,38 @@ class _TestAlwaysNeedsAddToSceneLayer extends ContainerLayer { @override bool get alwaysNeedsAddToScene => true; } + +class FakeSceneBuilder extends Fake implements SceneBuilder { + void reset() { + pushedOpacity = false; + pushedOffset = false; + addedPicture = false; + } + + bool pushedOpacity = false; + @override + OpacityEngineLayer pushOpacity(int alpha, {Offset? offset = Offset.zero, OpacityEngineLayer? oldLayer}) { + pushedOpacity = true; + return FakeOpacityEngineLayer(); + } + + bool pushedOffset = false; + @override + OffsetEngineLayer pushOffset(double x, double y, {OffsetEngineLayer? oldLayer}) { + pushedOffset = true; + return FakeOffsetEngineLayer(); + } + + bool addedPicture = false; + @override + void addPicture(Offset offset, Picture picture, {bool isComplexHint = false, bool willChangeHint = false}) { + addedPicture = true; + } + + @override + void pop() {} +} + +class FakeOpacityEngineLayer extends FakeEngineLayer implements OpacityEngineLayer {} + +class FakeOffsetEngineLayer extends FakeEngineLayer implements OffsetEngineLayer {} \ No newline at end of file diff --git a/packages/flutter/test/rendering/proxy_box_test.dart b/packages/flutter/test/rendering/proxy_box_test.dart index 6693af182b1..6733cce2457 100644 --- a/packages/flutter/test/rendering/proxy_box_test.dart +++ b/packages/flutter/test/rendering/proxy_box_test.dart @@ -274,14 +274,14 @@ void main() { expect(renderOpacity.needsCompositing, false); }); - test('RenderOpacity does not composite if it is opaque', () { + test('RenderOpacity does composite if it is opaque', () { final RenderOpacity renderOpacity = RenderOpacity( opacity: 1.0, child: RenderSizedBox(const Size(1.0, 1.0)), // size doesn't matter ); layout(renderOpacity, phase: EnginePhase.composite); - expect(renderOpacity.needsCompositing, false); + expect(renderOpacity.needsCompositing, true); }); test('RenderOpacity reuses its layer', () { @@ -306,7 +306,7 @@ void main() { expect(renderAnimatedOpacity.needsCompositing, false); }); - test('RenderAnimatedOpacity does not composite if it is opaque', () { + test('RenderAnimatedOpacity does composite if it is opaque', () { final Animation opacityAnimation = AnimationController( vsync: FakeTickerProvider(), )..value = 1.0; @@ -318,7 +318,7 @@ void main() { ); layout(renderAnimatedOpacity, phase: EnginePhase.composite); - expect(renderAnimatedOpacity.needsCompositing, false); + expect(renderAnimatedOpacity.needsCompositing, true); }); test('RenderAnimatedOpacity reuses its layer', () { diff --git a/packages/flutter/test/rendering/proxy_sliver_test.dart b/packages/flutter/test/rendering/proxy_sliver_test.dart index 85169d2d5d3..a197f8292f2 100644 --- a/packages/flutter/test/rendering/proxy_sliver_test.dart +++ b/packages/flutter/test/rendering/proxy_sliver_test.dart @@ -29,7 +29,7 @@ void main() { expect(renderSliverOpacity.needsCompositing, false); }); - test('RenderSliverOpacity does not composite if it is opaque', () { + test('RenderSliverOpacity does composite if it is opaque', () { final RenderSliverOpacity renderSliverOpacity = RenderSliverOpacity( opacity: 1.0, sliver: RenderSliverToBoxAdapter( @@ -46,7 +46,7 @@ void main() { ); layout(root, phase: EnginePhase.composite); - expect(renderSliverOpacity.needsCompositing, false); + expect(renderSliverOpacity.needsCompositing, true); }); test('RenderSliverOpacity reuses its layer', () { final RenderSliverOpacity renderSliverOpacity = RenderSliverOpacity( @@ -102,7 +102,7 @@ void main() { expect(renderSliverAnimatedOpacity.needsCompositing, false); }); - test('RenderSliverAnimatedOpacity does not composite if it is opaque', () { + test('RenderSliverAnimatedOpacity does composite if it is opaque', () { final Animation opacityAnimation = AnimationController( vsync: FakeTickerProvider(), )..value = 1.0; @@ -124,7 +124,7 @@ void main() { ); layout(root, phase: EnginePhase.composite); - expect(renderSliverAnimatedOpacity.needsCompositing, false); + expect(renderSliverAnimatedOpacity.needsCompositing, true); }); test('RenderSliverAnimatedOpacity reuses its layer', () { diff --git a/packages/flutter/test/widgets/opacity_test.dart b/packages/flutter/test/widgets/opacity_test.dart index f3225c92a4a..23af74d472c 100644 --- a/packages/flutter/test/widgets/opacity_test.dart +++ b/packages/flutter/test/widgets/opacity_test.dart @@ -195,4 +195,35 @@ void main() { final OffsetLayer offsetLayer = element.renderObject!.debugLayer! as OffsetLayer; await offsetLayer.toImage(const Rect.fromLTRB(0.0, 0.0, 1.0, 1.0)); }, skip: isBrowser); // https://github.com/flutter/flutter/issues/49857 + + testWidgets('Child shows up in the right spot when opacity is disabled', (WidgetTester tester) async { + debugDisableOpacityLayers = true; + final GlobalKey key = GlobalKey(); + await tester.pumpWidget( + RepaintBoundary( + key: key, + child: Directionality( + textDirection: TextDirection.ltr, + child: Stack( + children: [ + Positioned( + top: 40, + left: 140, + child: Opacity( + opacity: .5, + child: Container(height: 100, width: 100, color: Colors.red), + ), + ), + ], + ), + ), + ), + ); + + await expectLater( + find.byKey(key), + matchesGoldenFile('opacity_disabled_with_child.png'), + ); + debugDisableOpacityLayers = false; + }); }