Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for invalid elevations #30215

Merged
merged 15 commits into from Apr 10, 2019
Merged

Check for invalid elevations #30215

merged 15 commits into from Apr 10, 2019

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Mar 29, 2019

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.

@dnfield 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
packages/flutter/lib/src/rendering/layer.dart Outdated Show resolved Hide resolved
@@ -679,6 +765,20 @@ class ContainerLayer extends Layer {
assert(transform != null);
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
packages/flutter/lib/src/rendering/layer.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/rendering/layer.dart Outdated Show resolved Hide resolved
List<PictureLayer> _debugCheckElevations() {
final List<PhysicalModelLayer> physicalModelLayers = depthFirstIterateChildren().whereType<PhysicalModelLayer>().toList();
final List<PictureLayer> addedLayers = <PictureLayer>[];
bool _predecessorIsNotDirectAncestor(PhysicalModelLayer predecessor, Layer child) {
Copy link
Contributor Author

@dnfield dnfield Apr 9, 2019

Choose a reason for hiding this comment

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

This should be _predecessorIsAncestor

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@@ -1090,6 +1204,9 @@ class TransformLayer extends OffsetLayer {
void applyTransform(Layer child, Matrix4 transform) {
assert(child != null);
assert(transform != null);
if (_lastEffectiveTransform == null && this.transform != null) {
Copy link
Member

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
packages/flutter/lib/src/rendering/layer.dart Show resolved Hide resolved
packages/flutter/lib/src/rendering/layer.dart Show resolved Hide resolved
];
}

/// Checks that no [PhysicalModelLayer] would paint after another
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
packages/flutter/lib/src/rendering/layer.dart Show resolved Hide resolved
packages/flutter/lib/src/rendering/debug.dart Show resolved Hide resolved
@@ -50,6 +50,40 @@ 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 painted out of order with regard to their elevation.
Copy link
Member

Choose a reason for hiding this comment

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

Should this summary maybe already include the overlapping bit?


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

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
packages/flutter/test/widgets/physical_model_test.dart Outdated Show resolved Hide resolved
debugDisableShadows = true;
}

testWidgets('entirely overlapping, direct child', (WidgetTester tester) async {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

/// 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):
/// ┌───────────────────┐
Copy link
Member

Choose a reason for hiding this comment

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

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

@dnfield dnfield merged commit d2790bd into flutter:master Apr 10, 2019
@dnfield dnfield deleted the elevation_checker branch April 10, 2019 21:57
Copy link
Contributor

@jacob314 jacob314 left a comment

Choose a reason for hiding this comment

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

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

jiisd added a commit to jiisd/flutter that referenced this pull request Apr 12, 2019
* 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
Copy link

mwk24 commented Feb 14, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer: dream (g3) customer: fuchsia framework flutter/packages/flutter repository. See also f: labels. platform-fuchsia Fuchsia code specifically
Projects
None yet
6 participants