Unverified Commit d2790bd2 authored by Dan Field's avatar Dan Field Committed by GitHub

Check for invalid elevations (#30215)

* Check for invalid elevation usage in the layer tree
parent 83ddd988
......@@ -87,6 +87,17 @@ mixin RendererBinding on BindingBase, ServicesBinding, SchedulerBinding, Gesture
return Future<void>.value();
},
);
registerBoolServiceExtension(
name: 'debugCheckElevationsEnabled',
getter: () async => debugCheckElevationsEnabled,
setter: (bool value) {
if (debugCheckElevationsEnabled == value) {
return Future<void>.value();
}
debugCheckElevationsEnabled = value;
return _forceRepaint();
}
);
registerSignalServiceExtension(
name: 'debugDumpLayerTree',
callback: () {
......
......@@ -50,6 +50,44 @@ bool debugRepaintRainbowEnabled = false;
/// Overlay a rotating set of colors when repainting text in checked mode.
bool debugRepaintTextRainbowEnabled = false;
/// Causes [PhysicalModelLayer]s to paint a red rectangle around themselves if
/// they are overlapping and painted out of order with regard to their elevation.
///
/// Android and iOS will show the last painted layer on top, whereas Fuchsia
/// will show the layer with the highest elevation on top.
///
/// For example, a rectangular elevation at 3.0 that is painted before an
/// overlapping rectangular elevation at 2.0 would render this way on Android
/// and iOS (with fake shadows):
/// ```
/// ┌───────────────────┐
/// │ │
/// │ 3.0 │
/// │ ┌───────────────────┐
/// │ │ │
/// └────────────│ │
/// │ 2.0 │
/// │ │
/// └───────────────────┘
/// ```
///
/// But this way on Fuchsia (with real shadows):
/// ```
/// ┌───────────────────┐
/// │ │
/// │ 3.0 │
/// │ │────────────┐
/// │ │ │
/// └───────────────────┘ │
/// │ 2.0 │
/// │ │
/// └───────────────────┘
/// ```
///
/// This check helps developers that want a consistent look and feel detect
/// where this inconsistency would occur.
bool debugCheckElevationsEnabled = false;
/// The current color to overlay when repainting a layer.
///
/// This is used by painting debug code that implements
......
......@@ -4,7 +4,8 @@
import 'dart:async';
import 'dart:collection';
import 'dart:ui' as ui show EngineLayer, Image, ImageFilter, Picture, Scene, SceneBuilder;
import 'dart:ui' as ui show EngineLayer, Image, ImageFilter, PathMetric,
Picture, PictureRecorder, Scene, SceneBuilder;
import 'package:flutter/foundation.dart';
import 'package:flutter/painting.dart';
......@@ -503,6 +504,100 @@ class ContainerLayer extends Layer {
return child == equals;
}
PictureLayer _highlightConflictingLayer(PhysicalModelLayer child) {
final ui.PictureRecorder recorder = ui.PictureRecorder();
final Canvas canvas = Canvas(recorder);
canvas.drawPath(
child.clipPath,
Paint()
..color = const Color(0xFFAA0000)
..style = PaintingStyle.stroke
// The elevation may be 0 or otherwise too small to notice.
// Adding 10 to it makes it more visually obvious.
..strokeWidth = child.elevation + 10.0,
);
final PictureLayer pictureLayer = PictureLayer(child.clipPath.getBounds())
..picture = recorder.endRecording()
..debugCreator = child;
child.append(pictureLayer);
return pictureLayer;
}
List<PictureLayer> _processConflictingPhysicalLayers(PhysicalModelLayer predecessor, PhysicalModelLayer child) {
FlutterError.reportError(FlutterErrorDetails(
exception: FlutterError('Painting order is out of order with respect to elevation.\n'
'See https://api.flutter.dev/flutter/rendering/debugCheckElevations.html '
'for more details.'),
library: 'rendering library',
context: 'during compositing',
informationCollector: (StringBuffer buffer) {
buffer.writeln('Attempted to composite layer:');
buffer.writeln(child);
buffer.writeln('after layer:');
buffer.writeln(predecessor);
buffer.writeln('which occupies the same area at a higher elevation.');
}
));
return <PictureLayer>[
_highlightConflictingLayer(predecessor),
_highlightConflictingLayer(child),
];
}
/// Checks that no [PhysicalModelLayer] would paint after another overlapping
/// [PhysicalModelLayer] that has a higher elevation.
///
/// Returns a list of [PictureLayer] objects it added to the tree to highlight
/// bad nodes. These layers should be removed from the tree after building the
/// [Scene].
List<PictureLayer> _debugCheckElevations() {
final List<PhysicalModelLayer> physicalModelLayers = depthFirstIterateChildren().whereType<PhysicalModelLayer>().toList();
final List<PictureLayer> addedLayers = <PictureLayer>[];
for (int i = 0; i < physicalModelLayers.length; i++) {
final PhysicalModelLayer physicalModelLayer = physicalModelLayers[i];
assert(
physicalModelLayer.lastChild?.debugCreator != physicalModelLayer,
'debugCheckElevations has either already visited this layer or failed '
'to remove the added picture from it.',
);
double accumulatedElevation = physicalModelLayer.elevation;
Layer ancestor = physicalModelLayer.parent;
while (ancestor != null) {
if (ancestor is PhysicalModelLayer) {
accumulatedElevation += ancestor.elevation;
}
ancestor = ancestor.parent;
}
for (int j = 0; j <= i; j++) {
final PhysicalModelLayer predecessor = physicalModelLayers[j];
double predecessorAccumulatedElevation = predecessor.elevation;
ancestor = predecessor.parent;
while (ancestor != null) {
if (ancestor == predecessor) {
continue;
}
if (ancestor is PhysicalModelLayer) {
predecessorAccumulatedElevation += ancestor.elevation;
}
ancestor = ancestor.parent;
}
if (predecessorAccumulatedElevation <= accumulatedElevation) {
continue;
}
final Path intersection = Path.combine(
PathOperation.intersect,
predecessor._debugTransformedClipPath,
physicalModelLayer._debugTransformedClipPath,
);
if (intersection != null && intersection.computeMetrics().any((ui.PathMetric metric) => metric.length > 0)) {
addedLayers.addAll(_processConflictingPhysicalLayers(predecessor, physicalModelLayer));
}
}
}
return addedLayers;
}
@override
void updateSubtreeNeedsAddToScene() {
super.updateSubtreeNeedsAddToScene();
......@@ -679,6 +774,23 @@ class ContainerLayer extends Layer {
assert(transform != null);
}
/// Returns the descendants of this layer in depth first order.
@visibleForTesting
List<Layer> depthFirstIterateChildren() {
if (firstChild == null)
return <Layer>[];
final List<Layer> children = <Layer>[];
Layer child = firstChild;
while(child != null) {
children.add(child);
if (child is ContainerLayer) {
children.addAll(child.depthFirstIterateChildren());
}
child = child.nextSibling;
}
return children;
}
@override
List<DiagnosticsNode> debugDescribeChildren() {
final List<DiagnosticsNode> children = <DiagnosticsNode>[];
......@@ -744,9 +856,29 @@ class OffsetLayer extends ContainerLayer {
/// Consider this layer as the root and build a scene (a tree of layers)
/// in the engine.
ui.Scene buildScene(ui.SceneBuilder builder) {
List<PictureLayer> temporaryLayers;
assert(() {
if (debugCheckElevationsEnabled) {
temporaryLayers = _debugCheckElevations();
}
return true;
}());
updateSubtreeNeedsAddToScene();
addToScene(builder);
return builder.build();
final ui.Scene scene = builder.build();
assert(() {
// We should remove any layers that got added to highlight the incorrect
// PhysicalModelLayers. If we don't, we'll end up adding duplicate layers
// or potentially leaving a physical model that is now correct highlighted
// in red.
if (temporaryLayers != null) {
for (PictureLayer temporaryLayer in temporaryLayers) {
temporaryLayer.remove();
}
}
return true;
}());
return scene;
}
@override
......@@ -1090,7 +1222,12 @@ class TransformLayer extends OffsetLayer {
void applyTransform(Layer child, Matrix4 transform) {
assert(child != null);
assert(transform != null);
transform.multiply(_lastEffectiveTransform);
assert(_lastEffectiveTransform != null || this.transform != null);
if (_lastEffectiveTransform == null) {
transform.multiply(this.transform);
} else {
transform.multiply(_lastEffectiveTransform);
}
}
@override
......@@ -1309,6 +1446,16 @@ class PhysicalModelLayer extends ContainerLayer {
}
}
Path get _debugTransformedClipPath {
ContainerLayer ancestor = parent;
final Matrix4 matrix = Matrix4.identity();
while (ancestor != null && ancestor.parent != null) {
ancestor.applyTransform(this, matrix);
ancestor = ancestor.parent;
}
return clipPath.transform(matrix.storage);
}
/// {@macro flutter.widgets.Clip}
Clip get clipBehavior => _clipBehavior;
Clip _clipBehavior;
......
......@@ -1709,6 +1709,10 @@ class RenderPhysicalModel extends _RenderPhysicalModelBase<RRect> {
color: color,
shadowColor: shadowColor,
);
assert(() {
physicalModel.debugCreator = debugCreator;
return true;
}());
context.pushLayer(physicalModel, super.paint, offset, childPaintBounds: offsetBounds);
}
}
......@@ -1799,6 +1803,10 @@ class RenderPhysicalShape extends _RenderPhysicalModelBase<Path> {
color: color,
shadowColor: shadowColor,
);
assert(() {
physicalModel.debugCreator = debugCreator;
return true;
}());
context.pushLayer(physicalModel, super.paint, offset, childPaintBounds: offsetBounds);
}
}
......
......@@ -117,7 +117,7 @@ Future<Map<String, dynamic>> hasReassemble(Future<Map<String, dynamic>> pendingR
void main() {
final List<String> console = <String>[];
test('Service extensions - pretest', () async {
setUpAll(() async {
binding = TestServiceExtensionsBinding();
expect(binding.frameScheduled, isTrue);
......@@ -142,10 +142,25 @@ void main() {
};
});
tearDownAll(() async {
// See widget_inspector_test.dart for tests of the ext.flutter.inspector
// service extensions included in this count.
int widgetInspectorExtensionCount = 15;
if (WidgetInspectorService.instance.isWidgetCreationTracked()) {
// Some inspector extensions are only exposed if widget creation locations
// are tracked.
widgetInspectorExtensionCount += 2;
}
// If you add a service extension... TEST IT! :-)
// ...then increment this number.
expect(binding.extensions.length, 26 + widgetInspectorExtensionCount);
expect(console, isEmpty);
debugPrint = debugPrintThrottled;
});
// The following list is alphabetical, one test per extension.
//
// The order doesn't really matter except that the pretest and posttest tests
// must be first and last respectively.
test('Service extensions - debugAllowBanner', () async {
Map<String, dynamic> result;
......@@ -170,6 +185,34 @@ void main() {
expect(binding.frameScheduled, isFalse);
});
test('Service extensions - debugCheckElevationsEnabled', () async {
expect(binding.frameScheduled, isFalse);
expect(debugCheckElevationsEnabled, false);
bool lastValue = false;
Future<void> _updateAndCheck(bool newValue) async {
Map<String, dynamic> result;
binding.testExtension(
'debugCheckElevationsEnabled',
<String, String>{'enabled': '$newValue'}
).then((Map<String, dynamic> answer) => result = answer);
await binding.flushMicrotasks();
expect(binding.frameScheduled, lastValue != newValue);
await binding.doFrame();
await binding.flushMicrotasks();
expect(result, <String, String>{'enabled': '$newValue'});
expect(debugCheckElevationsEnabled, newValue);
lastValue = newValue;
}
await _updateAndCheck(false);
await _updateAndCheck(true);
await _updateAndCheck(true);
await _updateAndCheck(false);
await _updateAndCheck(false);
expect(binding.frameScheduled, isFalse);
});
test('Service extensions - debugDumpApp', () async {
Map<String, dynamic> result;
......@@ -617,22 +660,4 @@ void main() {
expect(trace, contains('package:test_api/test_api.dart,::,test\n'));
expect(trace, contains('service_extensions_test.dart,::,main\n'));
});
test('Service extensions - posttest', () async {
// See widget_inspector_test.dart for tests of the ext.flutter.inspector
// service extensions included in this count.
int widgetInspectorExtensionCount = 15;
if (WidgetInspectorService.instance.isWidgetCreationTracked()) {
// Some inspector extensions are only exposed if widget creation locations
// are tracked.
widgetInspectorExtensionCount += 2;
}
// If you add a service extension... TEST IT! :-)
// ...then increment this number.
expect(binding.extensions.length, 25 + widgetInspectorExtensionCount);
expect(console, isEmpty);
debugPrint = debugPrintThrottled;
});
}
......@@ -4,6 +4,7 @@
import 'dart:ui';
import 'package:flutter/foundation.dart';
import 'package:flutter/rendering.dart';
import 'package:flutter_test/flutter_test.dart';
......@@ -112,6 +113,56 @@ void main() {
expect(followerLayer.debugSubtreeNeedsAddToScene, true);
});
test('depthFirstIterateChildren', () {
final ContainerLayer a = ContainerLayer();
final ContainerLayer b = ContainerLayer();
final ContainerLayer c = ContainerLayer();
final ContainerLayer d = ContainerLayer();
final ContainerLayer e = ContainerLayer();
final ContainerLayer f = ContainerLayer();
final ContainerLayer g = ContainerLayer();
final PictureLayer h = PictureLayer(Rect.zero);
final PictureLayer i = PictureLayer(Rect.zero);
final PictureLayer j = PictureLayer(Rect.zero);
// The tree is like the following:
// a____
// / \
// b___ c
// / \ \ |
// d e f g
// / \ |
// h i j
a.append(b);
a.append(c);
b.append(d);
b.append(e);
b.append(f);
d.append(h);
d.append(i);
c.append(g);
g.append(j);
expect(
a.depthFirstIterateChildren(),
<Layer>[b, d, h, i, e, f, c, g, j],
);
d.remove();
// a____
// / \
// b___ c
// \ \ |
// e f g
// |
// j
expect(
a.depthFirstIterateChildren(),
<Layer>[b, e, f, c, g, j],
);
});
void checkNeedsAddToScene(Layer layer, void mutateCallback()) {
layer.debugMarkClean();
layer.updateSubtreeNeedsAddToScene();
......@@ -239,4 +290,170 @@ void main() {
layer.shadowColor = const Color(1);
});
});
group('PhysicalModelLayer checks elevations', () {
/// Adds the layers to a container where A paints before B.
///
/// Expects there to be `expectedErrorCount` errors. Checking elevations is
/// enabled by default.
void _testConflicts(
PhysicalModelLayer layerA,
PhysicalModelLayer layerB, {
@required int expectedErrorCount,
bool enableCheck = true,
}) {
assert(expectedErrorCount != null);
assert(enableCheck || expectedErrorCount == 0, 'Cannot disable check and expect non-zero error count.');
final OffsetLayer container = OffsetLayer();
container.append(layerA);
container.append(layerB);
debugCheckElevationsEnabled = enableCheck;
debugDisableShadows = false;
int errors = 0;
if (enableCheck) {
FlutterError.onError = (FlutterErrorDetails details) {
errors++;
};
}
container.buildScene(SceneBuilder());
expect(errors, expectedErrorCount);
debugCheckElevationsEnabled = false;
}
// Tests:
//
// ───────────── (LayerA, paints first)
// │ ───────────── (LayerB, paints second)
// │ │
// ───────────────────────────
test('Overlapping layers at wrong elevation', () {
final PhysicalModelLayer layerA = PhysicalModelLayer(
clipPath: Path()..addRect(Rect.fromLTWH(0, 0, 20, 20)),
elevation: 3.0,
color: const Color(0),
shadowColor: const Color(0),
);
final PhysicalModelLayer layerB =PhysicalModelLayer(
clipPath: Path()..addRect(Rect.fromLTWH(10, 10, 20, 20)),
elevation: 2.0,
color: const Color(0),
shadowColor: const Color(0),
);
_testConflicts(layerA, layerB, expectedErrorCount: 1);
});
// Tests:
//
// ───────────── (LayerA, paints first)
// │ ───────────── (LayerB, paints second)
// │ │
// ───────────────────────────
//
// Causes no error if check is disabled.
test('Overlapping layers at wrong elevation, check disabled', () {
final PhysicalModelLayer layerA = PhysicalModelLayer(
clipPath: Path()..addRect(Rect.fromLTWH(0, 0, 20, 20)),
elevation: 3.0,
color: const Color(0),
shadowColor: const Color(0),
);
final PhysicalModelLayer layerB =PhysicalModelLayer(
clipPath: Path()..addRect(Rect.fromLTWH(10, 10, 20, 20)),
elevation: 2.0,
color: const Color(0),
shadowColor: const Color(0),
);
_testConflicts(layerA, layerB, expectedErrorCount: 0, enableCheck: false);
});
// Tests:
//
// ────────── (LayerA, paints first)
// │ ─────────── (LayerB, paints second)
// │ │
// ────────────────────────────
test('Non-overlapping layers at wrong elevation', () {
final PhysicalModelLayer layerA = PhysicalModelLayer(
clipPath: Path()..addRect(Rect.fromLTWH(0, 0, 20, 20)),
elevation: 3.0,
color: const Color(0),
shadowColor: const Color(0),
);
final PhysicalModelLayer layerB =PhysicalModelLayer(
clipPath: Path()..addRect(Rect.fromLTWH(20, 20, 20, 20)),
elevation: 2.0,
color: const Color(0),
shadowColor: const Color(0),
);
_testConflicts(layerA, layerB, expectedErrorCount: 0);
});
// Tests:
//
// ─────── (Child of A, paints second)
// │
// ─────────── (LayerA, paints first)
// │ ──────────── (LayerB, paints third)
// │ │
// ────────────────────────────
test('Non-overlapping layers at wrong elevation, child at lower elevation', () {
final PhysicalModelLayer layerA = PhysicalModelLayer(
clipPath: Path()..addRect(Rect.fromLTWH(0, 0, 20, 20)),
elevation: 3.0,
color: const Color(0),
shadowColor: const Color(0),
);
layerA.append(PhysicalModelLayer(
clipPath: Path()..addRect(Rect.fromLTWH(2, 2, 10, 10)),
elevation: 1.0,
color: const Color(0),
shadowColor: const Color(0),
));
final PhysicalModelLayer layerB =PhysicalModelLayer(
clipPath: Path()..addRect(Rect.fromLTWH(20, 20, 20, 20)),
elevation: 2.0,
color: const Color(0),
shadowColor: const Color(0),
);
_testConflicts(layerA, layerB, expectedErrorCount: 0);
});
// Tests:
//
// ─────────── (Child of A, paints second, overflows)
// │ ──────────── (LayerB, paints third)
// ─────────── │ (LayerA, paints first)
// │ │
// │ │
// ────────────────────────────
//
// Which fails because the overflowing child overlaps something that paints
// after it at a lower elevation.
test('Child overflows parent and overlaps another physical layer', () {
final PhysicalModelLayer layerA = PhysicalModelLayer(
clipPath: Path()..addRect(Rect.fromLTWH(0, 0, 20, 20)),
elevation: 3.0,
color: const Color(0),
shadowColor: const Color(0),
);
layerA.append(PhysicalModelLayer(
clipPath: Path()..addRect(Rect.fromLTWH(15, 15, 25, 25)),
elevation: 2.0,
color: const Color(0),
shadowColor: const Color(0),
));
final PhysicalModelLayer layerB =PhysicalModelLayer(
clipPath: Path()..addRect(Rect.fromLTWH(20, 20, 20, 20)),
elevation: 4.0,
color: const Color(0),
shadowColor: const Color(0),
);
_testConflicts(layerA, layerB, expectedErrorCount: 1);
});
});
}
......@@ -272,6 +272,11 @@ class FlutterDevice {
await view.uiIsolate.flutterToggleDebugPaintSizeEnabled();
}
Future<void> toggleDebugCheckElevationsEnabled() async {
for (FlutterView view in views)
await view.uiIsolate.flutterToggleDebugCheckElevationsEnabled();
}
Future<void> debugTogglePerformanceOverlayOverride() async {
for (FlutterView view in views)
await view.uiIsolate.flutterTogglePerformanceOverlayOverride();
......@@ -620,6 +625,12 @@ abstract class ResidentRunner {
await device.toggleDebugPaintSizeEnabled();
}
Future<void> _debugToggleDebugCheckElevationsEnabled() async {
await refreshViews();
for (FlutterDevice device in flutterDevices)
await device.toggleDebugCheckElevationsEnabled();
}
Future<void> _debugTogglePerformanceOverlayOverride() async {
await refreshViews();
for (FlutterDevice device in flutterDevices)
......@@ -871,6 +882,9 @@ abstract class ResidentRunner {
} else if (lower == 'd') {
await detach();
return true;
} else if (lower == 'z') {
await _debugToggleDebugCheckElevationsEnabled();
return true;
}
return false;
......@@ -962,6 +976,7 @@ abstract class ResidentRunner {
printStatus('To toggle the widget inspector (WidgetsApp.showWidgetInspectorOverride), press "i".');
printStatus('To toggle the display of construction lines (debugPaintSizeEnabled), press "p".');
printStatus('To simulate different operating systems, (defaultTargetPlatform), press "o".');
printStatus('To toggle the elevation checker, press "z".');
} else {
printStatus('To dump the accessibility tree (debugDumpSemantics), press "S" (for traversal order) or "U" (for inverse hit test order).');
}
......
......@@ -1283,6 +1283,8 @@ class Isolate extends ServiceObjectOwner {
Future<Map<String, dynamic>> flutterToggleDebugPaintSizeEnabled() => _flutterToggle('debugPaint');
Future<Map<String, dynamic>> flutterToggleDebugCheckElevationsEnabled() => _flutterToggle('debugCheckElevationsEnabled');
Future<Map<String, dynamic>> flutterTogglePerformanceOverlayOverride() => _flutterToggle('showPerformanceOverlay');
Future<Map<String, dynamic>> flutterToggleWidgetInspector() => _flutterToggle('inspector.show');
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment