DRAFT: Layout assertion on Overlay and Ink (#147452) - #158918

Hey @Gustl22 do you want to continue working on this change? If so, please mark it ready for review when it is (it is currently a draft). It looks like there are a bunch of unrelated changes included here as well.

Thank you for reaching out @Piinks
I would love to continue on this PR.
Therefore I already asked twice in the hackers channel on Discord how the layouting is supposed to be, but I got no defintive answer:
https://discord.com/channels/608014603317936148/608018585025118217/1305842290295832577
https://discord.com/channels/608014603317936148/608018585025118217/1319590012832186390

As explained in this issue description, I would like to know whether Widgets should be laid out, if they are hidden behind some sort of overlay (offstaged). This is handled differently in the two widgets Overlay and Offstage.

The unrelated code changes are just some code to try out my stuff and will get transitioned to proper tests, once the solution is applied.

Thank you for any further information in that direction.

I think the correct approach to actually always lay it out regardless if its offstaged (how its currently handled in offstage widget). I suppose technically it's already like this, because the underlaying Overlay is almost always implicitely layed out before the second overlay is pushed on top. Except in this case for web, where both overlays are injected simultaneously, so this bug occurs.

Thanks for the update!

The unrelated code changes are just some code to try out my stuff and will get transitioned to proper tests, once the solution is applied.

Unfortunately the unrelated code makes it difficult to discern what this change is. Can you remove the unrelated code so we can better understand what you are trying to change here?

@Piinks thank you for getting back.

I updated the PR to the latest main state, kept in the reproduction example (but removed every unnecessary stuff).

Approach 1: Avoid painting Ink RenderObjects, which are not visible: This is what I have tried so far in this PR, but I think that's wrong (it's not done like that in e.g. Offstage widget).

Approach 2: Just lay out every overlay in the widget tree (I don't mean render). This is how its done in the Offstage widget. But in Overlay we have some custom logic to exclude that: https://github.com/flutter/flutter/pull/158918/files#diff-6ea34306ad31c88d41a39d24aaad65a2ef227ebbee8919697b3411ff2ef0f897R1438
For a solution I would even propose to use the Offstage widget in this Overlay widget, to not duplicate any logic.
I just haven't implemented that, as it means a lot of code changes, which might not be desired. That's why I asked this yes or no question in the first place.

Hope the problem get's more clearly. It's hard to explain unless one executes the steps in the reproduciton sample (Web only). But the general question keeps the same (even without looking into the code).

Thanks for the update! Hmm, I wonder if @LongCatIsLooong knows why the Overlay behaves differently than Offstage when children are not visible. It does seem like an intentional behavior.

Do you mean Overlay skipping laying out obstructed OverlayEntries? Yes that's intended, we don't want to keep doing layout computations for offscreen routes because that could be expensive and it scales as the number of offscreen routes increases. Typically we only want to do work for what is onscreen (we do rebuild the widget tree even for offscreen widgets though).

Drive-by comment: can you add a simple explanation to help reviewers understand why this issue is happening?

Also the original issue is created one year ago. There are some recent changes made to LayoutBuilder so I'm not sure the original issue still exists / is going to hit the same assertion.

// We should either layout them, or disallow unlayouted widgets, to paint.

The issue is about a layout assert so why do we need to make changes to the painting logic? Aslo IIRC Ink stuff already checks paintsChild.

Hi there @LongCatIsLooong,

Do you mean Overlay skipping laying out obstructed OverlayEntries? Yes that's intended, we don't want to keep doing layout computations for offscreen routes because that could be expensive and it scales as the number of offscreen routes increases. Typically we only want to do work for what is onscreen (we do rebuild the widget tree even for offscreen widgets though).

Yes, that is kind of confusing, it should be the case for Offstage, too. Or at least they should handle it the same way (?)

Also the original issue is created one year ago. There are some recent changes made to LayoutBuilder so I'm not sure the original issue still exists / is going to hit the same assertion.

That was my hope, too, but it still is a problem with the most recent main.

can you add a simple explanation to help reviewers understand why this issue is happening?

I now could further shrink down the code to only framework components:

import 'package:flutter/material.dart';

/// Flutter code sample for [Overlay].

void main() {
  // debugRepaintRainbowEnabled = true;
  // debugPrintMarkNeedsLayoutStacks = true;
  // debugPrintMarkNeedsPaintStacks = true;
  // debugPrintLayouts = true;
  runApp(const OverlayApp());
}

class OverlayApp extends StatelessWidget {
  const OverlayApp({super.key});

  @override
  Widget build(BuildContext context) {
    return const MaterialApp(home: OverlayExample());
  }
}

class OverlayExample extends StatefulWidget {
  const OverlayExample({super.key});

  @override
  State<OverlayExample> createState() => _OverlayExampleState();
}

class _OverlayExampleState extends State<OverlayExample> {
  @override
  void dispose() {
    super.dispose();
  }

  @override
  Widget build(BuildContext context) {
    return Material( // <- InkDecoration is registered here
      child: Overlay(
        initialEntries: [
          OverlayEntry( // Offstaged and therefore not layouted
            builder: (BuildContext context) {
              return Ink(child: Container(width: 111, height: 112, color: Colors.red));
            },
            opaque: true,
            canSizeOverlay: true,
            maintainState: true,
          ),
          OverlayEntry(
            builder: (BuildContext context) {
              return Container(width: 222, height: 223, color: Colors.blue);
            },
            opaque: true,
            canSizeOverlay: true,
            maintainState: true,
          ),
        ],
      ),
    );
  }
}

Typically one uses a ShellRoute of GoRouter which wraps the Scaffold or Material which leads to the same outcome.
The problem is the following:

• We have an Ink widget, which is offstaged, because it is overlaid by another Overlay
• The Ink widget registers its InkDecoration at the next Material in the widget tree
• The Material widget is used for both Overlays, so if any page is build (e.g. the top most, also in case of a ShellRoute) its paint method get's called
• The Material widget tries to render all registered InkDecorations even if the offstaged widget of the underlying Inks RenderObject isn't even layouted.

Options:

1. Wrap subroutes of GoRouter in Material, but that doesn't fix the underlaying problem.
2. Always layout offstaged widget's (like done in Offstage widget). I suspect that also implicitly that was always the case, as the underlying widget most likely was layouted before some other route is pushed on top. So this error basically never occured in real world scenario, except for initialRoutes. It's not layouting offstaged widget is an optimization which almost never is occuring, or applied, at least that's what I think. Not sure about other cases though.
3. Don't paint InkDecorations, whose renderObject doesn't have a size (is not layouted yet).
   • Just ignore the assertion
   • Or decouple Ink from Material and render it, when its referenced renderObject is rendered.

@Gustl22, what are you missing for this PR?

Also, I'm having trouble understanding what you are doing. It looks like you are only printing and creating _getFirstLayoutedParent which is never used

@ValentinVignal I hadn't had time to evaluate which fix is the most appropriate one, but I guess it is the one to just ignore the assertion. The code in this PR is not fixing anything (yet). I will transfer the examples to proper tests and then propose a fix and unmark as DRAFT.

What would help me though, is which of these options described above you would prefer (although the solution might differ for SelectionArea):

1. Wrap subroutes of GoRouter in Material, but that doesn't fix the underlaying problem.
2. Always layout offstaged widget's (like done in Offstage widget). I suspect that also implicitly that was always the case, as the underlying widget most likely was layouted before some other route is pushed on top. So this error basically never occured in real world scenario, except for initialRoutes. It's not layouting offstaged widget is an optimization which almost never is occuring, or applied, at least that's what I think. Not sure about other cases though.
3. Don't paint InkDecorations, whose renderObject doesn't have a size (is not layouted yet).
   - Just ignore the assertion
   - Or decouple Ink from Material and render it, when its referenced renderObject is rendered.

The other thing that came into consideration is that other widgets with different behavior such as SelectionArea might need a different fix (depending on the implementation).

TL;DR: I need more time to provide a proper fix.

Hi @Gustl22 is this a change you plan to continue working on?

I would love to, but need a deeper understanding of the second case of failure. Maybe one can clarify my question above. In the mean time, I will close the PR until I have enough time and I can properly provide a fix.