Check for invalid elevations by dnfield · Pull Request #30215 · flutter/flutter

dnfield · 2019-03-29T21:14:52Z

Description

Today, Android and iOS render in painting order regaredless of elevation. Fuchsia renders in elevation order regardless of painting order. This can lead to vastly different rendering output between the two platforms, particularly when stacks are involved.

This test expects that overlapping layers at the same z-index will be handled in some other way, and are valid.

This patch creates a check at the layer tree level for invalid elevation with regard to painting order in PhysicalModelLayers. This is a recording with the check turned on in some parts of the Flutter Gallery that currently violate this.

The key to turn this on from the console is z. Alternatively, it can be programatically enabled via the debugCheckElevationsEnabled service extension, or by setting debugCheckElevationsEnabled to true from rendering/debug.dart.

/cc @mklim who is working on related efforts for z-fighting.
/cc @jacob314 and @DanTup - at some point there may be a desire to enable toggling this from the IDE plugins.

Related Issues

Fixes #25673
#27891 - which enabled physical layer compositing on all platforms, making this check possible.
#27137 - which was an initial attempt at this that fails we can't trust a call to paint on the widgets being tested.

Tests

I added the following tests:

Test for depth first iterating layers
Tests for this at the layer level
Tests for this at the widget level

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

packages/flutter/lib/src/rendering/layer.dart

jonahwilliams · 2019-03-30T23:25:25Z

packages/flutter/lib/src/rendering/layer.dart

  }

+  /// Returns the descendants of this layer in depth first order.
+  Iterable<Layer> depthFirstIterateChildren() sync* {


In general sync* has very bad performance, it is better to avoid it.

Is there some way to inspect the generated code here? I strongly suspect it will be at least as efficient as writing our own iterable implementation for this in the current usage...

Although this is making me realize that I'm modifying the tree while iterating it which is a bad idea. I can fix that though.

packages/flutter/lib/src/rendering/layer.dart

dnfield · 2019-04-09T16:23:01Z

packages/flutter/lib/src/rendering/layer.dart

+  List<PictureLayer> _debugCheckElevations() {
+    final List<PhysicalModelLayer> physicalModelLayers = depthFirstIterateChildren().whereType<PhysicalModelLayer>().toList();
+    final List<PictureLayer> addedLayers = <PictureLayer>[];
+    bool _predecessorIsNotDirectAncestor(PhysicalModelLayer predecessor, Layer child) {


This should be _predecessorIsAncestor

packages/flutter/lib/src/rendering/layer.dart

goderbauer

There's also the one elevation case I just showed you that didn't seem to work. Probably needs elevation addition?

packages/flutter/lib/src/rendering/debug.dart

packages/flutter/test/foundation/service_extensions_test.dart

goderbauer · 2019-04-09T17:08:32Z

packages/flutter/lib/src/rendering/layer.dart

  void applyTransform(Layer child, Matrix4 transform) {
    assert(child != null);
    assert(transform != null);
+    if (_lastEffectiveTransform == null && this.transform != null) {


What if transform is also null? Wouldn't line 1210 throw then? If it is guaranteed to not be null, maybe add an assert instead?

packages/flutter/lib/src/rendering/layer.dart

goderbauer · 2019-04-09T17:12:37Z

packages/flutter/lib/src/rendering/layer.dart

+    ];
+  }
+
+  /// Checks that no [PhysicalModelLayer] would paint after another


Should this maybe mention that this condition is only problematic if they are overlapping?

packages/flutter/lib/src/rendering/layer.dart

goderbauer · 2019-04-09T17:18:37Z

packages/flutter/lib/src/rendering/layer.dart

+
+  List<PictureLayer> _processConflictingPhysicalLayers(PhysicalModelLayer predecessor, PhysicalModelLayer child) {
+    FlutterError.reportError(FlutterErrorDetails(
+      exception: FlutterError('Painting order is out of order with respect to elevation.'),


This could be more informative. Can we give any call to action to resolve the issue? Where can a developer learn more?

The full error ends up looking like this:

I/flutter (15065): ══╡ EXCEPTION CAUGHT BY RENDERING LIBRARY ╞═════════════════════════════════════════════════════════ I/flutter (15065): The following assertion was thrown during compositing: I/flutter (15065): Painting order is out of order with respect to elevation. I/flutter (15065): I/flutter (15065): Attempted to composite layer: I/flutter (15065): PhysicalModelLayer#c1c68(creator: PhysicalModel ← AnimatedPhysicalModel ← Material ← ConstrainedBox I/flutter (15065): ← Container ← Stack ← Column ← Directionality ← [root], elevation: 1.0, color: MaterialColor(primary I/flutter (15065): value: Color(0xff2196f3))) I/flutter (15065): after layer: I/flutter (15065): PhysicalModelLayer#d7ff6(creator: PhysicalModel ← AnimatedPhysicalModel ← Material ← ConstrainedBox I/flutter (15065): ← Container ← Stack ← Column ← Directionality ← [root], elevation: 1.2, color: MaterialColor(primary I/flutter (15065): value: Color(0xff4caf50))) I/flutter (15065): which occupies the same area at a higher elevation. I/flutter (15065): ════════════════════════════════════════════════════════════════════════════════════════════════════

Although unfortunately only for the first one. Is there some way you think I could highlight that this will be added with more information elsewhere?

Maybe we need a doc page on flutter.dev going over this concept?

Linking to somewhere with more information would probable be good. For now maybe just link back to the docs of debugCheckElevationsEnabled which have a nice example?

packages/flutter/lib/src/rendering/layer.dart

packages/flutter/lib/src/rendering/debug.dart

goderbauer · 2019-04-09T19:23:42Z

packages/flutter/lib/src/rendering/debug.dart

 bool debugRepaintTextRainbowEnabled = false;

+/// Causes [PhysicalModelLayer]s to paint a red rectangle around themselves if
+/// they are painted out of order with regard to their elevation.


Should this summary maybe already include the overlapping bit?

goderbauer · 2019-04-09T19:24:29Z

packages/flutter/lib/src/rendering/layer.dart

+
+  List<PictureLayer> _processConflictingPhysicalLayers(PhysicalModelLayer predecessor, PhysicalModelLayer child) {
+    FlutterError.reportError(FlutterErrorDetails(
+      exception: FlutterError('Painting order is out of order with respect to elevation.'),


Linking to somewhere with more information would probable be good. For now maybe just link back to the docs of debugCheckElevationsEnabled which have a nice example?

packages/flutter/test/rendering/layers_test.dart

packages/flutter/test/widgets/physical_model_test.dart

goderbauer · 2019-04-09T19:36:29Z

packages/flutter/test/widgets/physical_model_test.dart

+      debugDisableShadows = true;
+    }
+
+    testWidgets('entirely overlapping, direct child', (WidgetTester tester) async {


Future me who may have to debug these in the future wishes there were diagrams on these too

goderbauer

LGTM

goderbauer · 2019-04-10T19:26:10Z

packages/flutter/lib/src/rendering/debug.dart

+/// 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):
+/// ┌───────────────────┐


nit: should these be enclosed in ``` to ensure they render with a fixed-width font in dartdocs?

jacob314

Thanks for the heads up! I've filed flutter/devtools#508
to track adding this service extension to devtools.

* master: (209 commits) Allow mouse hover to only respond to some mouse events but not all. (flutter#30886) Fix issue 23527: Exception: RenderViewport exceeded its maximum number of layout cycles (flutter#30809) Keep hover annotation layers in sync with the mouse detector. (flutter#30829) Use identical instead of '==' in a constant expression. (flutter#30921) Add toggle for debugProfileWidgetBuilds (flutter#30867) Revert "Manual engine roll with disabled service authentication codes (flutter#30919)" (flutter#30930) Manual engine roll with disabled service authentication codes (flutter#30919) Baseline Aligned Row (flutter#30746) [Material] Fix showDialog crasher caused by old contexts (flutter#30754) Let `sliver.dart` `_createErrorWidget` work with other Widgets (flutter#30880) Add more dialog doc cross-reference (flutter#30887) Allow downloading of desktop embedding artifacts (flutter#30648) CupertinoDatePicker initialDateTime accounts for minuteInterval (flutter#30862) add golden tests for CupertinoDatePicker (flutter#30828) Simplify toImage future handling (flutter#30876) Fixed Table flex column layout error flutter#30437 (flutter#30470) Fix iTunes Transporter quirk (flutter#30883) Bump Android build tools to 28.0.3 in Dockerfile (flutter#30832) Update the upload key which seems to have trouble for some reason (flutter#30871) Check for invalid elevations (flutter#30215) ...

mwk24 · 2021-02-14T19:17:40Z

Apologies if mentioned above - are there plans to allow iOS/Android to render in elevation order?

If not, is there a best practice for bringing a widget out of a multi child? I've tried a few things, but the only way I've found to avoid typing the widget code out in 2 places is GlobalKey.currentWidget

dnfield added 6 commits March 28, 2019 10:35

Check for invalid elevation usage in the layer tree

d2aa134

Add service extension, debug guard

504c9ab

Merge remote-tracking branch 'upstream/master' into elevation_checker

4e41d1e

tests

195cdd6

Merge remote-tracking branch 'upstream/master' into elevation_checker

98021ba

tests

46ea686

dnfield added customer: fuchsia framework flutter/packages/flutter repository. See also f: labels. platform-fuchsia Fuchsia code specifically severe: customer critical customer: dream (g3) labels Mar 29, 2019

dnfield requested review from goderbauer and mklim March 29, 2019 21:14

googlebot added the cla: yes label Mar 29, 2019

add test

6a973ad

jonahwilliams reviewed Mar 30, 2019

View reviewed changes

dnfield added 3 commits March 30, 2019 19:49

Merge remote-tracking branch 'upstream/master' into elevation_checker

b8e7424

clean up

fd7412a

avoid modification while iterating

5b41e04

dnfield commented Apr 9, 2019

View reviewed changes

packages/flutter/lib/src/rendering/layer.dart Outdated Show resolved Hide resolved

review

ce9fcf4

goderbauer reviewed Apr 9, 2019

View reviewed changes

doc, tests

6a46542

goderbauer reviewed Apr 9, 2019

View reviewed changes

dnfield added 2 commits April 9, 2019 13:44

doc and parameters

490fd8d

remove setter

5d818e6

goderbauer approved these changes Apr 10, 2019

View reviewed changes

wrap diagram in backtics

209bdb7

dnfield merged commit d2790bd into flutter:master Apr 10, 2019

dnfield deleted the elevation_checker branch April 10, 2019 21:57

jacob314 reviewed Apr 10, 2019

View reviewed changes

Conversation

dnfield commented Mar 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dnfield Apr 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob314 left a comment

Choose a reason for hiding this comment

Uh oh!

mwk24 commented Feb 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dnfield commented Mar 29, 2019 •

edited

Loading

dnfield Apr 9, 2019 •

edited

Loading