[SuperEditor] Add test to demonstrate that changing layer builders does not re-layout

[angelosilvestre]

[SuperEditor] Add test to demonstrate that changing layer builders does not re-layout

Changing SuperEditor overlay builders doesn't trigger a re-layout. An overlay can change, for example, to switch the caret color depending on the theme.

This was discussed in #2042

This PR adds tests to demonstrate that issue. The first test, which only switches the theme, fails. The second test, that types a character after switching themes, passes.

[matthew-carroll]

Looking at this widget tree, I'm a little confused about the build behavior.

Your description of the problem is that the overlay builders don't rebuild when the brightness changes.

I'm reading through this tree. I see the ValueListenableBuilder, which rebuilds whenever the brightness changes. When the ValueListenableBuilder rebuilds, it should construct a new SuperEditor instance, which then creates a new DefaultCaretOverlayBuilder. If we're rebuilding the ValueListenableBuilder, and rebuilding the SuperEditor, why isn't the DefaultCaretOverlayBuilder being built?

[angelosilvestre]

Because none of the document nodes changed its content. Thus, we are not running another layout phase. We are only repainting the document components, without running a layout phase ContentLayers won't build the overlayers.

[matthew-carroll]

Can you phrase that statement in terms of the chain of events that I described? What did I get wrong in that series of events? It's not clear what "none of the document nodes changed" has to do with my comment. If we're rebuilding SuperEditor then why isn't SuperEditor running build on its internal ContentLayers? Where are those two builds getting disconnected, such that SuperEditor runs widget build, but ContentLayers doesn't?

[angelosilvestre]

This is the sequence of methods being called:

-> INITIAL BUILD
ValueListenableBuilder -> build
SuperEditor -> build
DocumentScaffold -> build
RenderContentLayers -> performLayout
ContentLayersElement -> buildLayers
RenderContentLayers -> performLayout
ContentLayersElement -> buildLayers

-> VALUE NOTIFIER CHANGED 
ValueListenableBuilder -> build
SuperEditor -> build
DocumentScaffold -> build
ContentLayersElement -> update

The critical thing for us is that RenderContentLayers.performLayout is not called. It seems the subtree isn't considered dirty, so the layout does not run again.

[matthew-carroll]

Do we have any comments in ContentLayersElement.update() that gives a specific reason for us not to rebuild the layers there?

I know that ContentLayers is very hacky, and intentionally avoids various rebuilds of the layers. I'm wondering if this is a place where we should re-run build.

One thing is that when update runs, if we haven't already laid out the content, then we don't have geometry info for the layers. So in that case, we'll need to invalidate layout upon update(). However, if the layout of content hasn't changed, then the previous geometry should still apply, and we should, in theory, be able to re-run build for the layers in the Element, outside of performLayout.

[angelosilvestre]

Do we have any comments in ContentLayersElement.update() that gives a specific reason for us not to rebuild the layers there?

We don't. That's the whole implementation:

@override
void update(ContentLayers newWidget) {
  super.update(newWidget);

  final newContent = widget.content(_onContentBuildScheduled);

  assert(widget == newWidget);
  assert(!debugChildrenHaveDuplicateKeys(widget, [newContent]));

  _content = updateChild(_content, newContent, _contentSlot);

  // super.update() and updateChild() is where the framework reparents
  // forgotten children. Therefore, at this point, the framework is
  // done with the concept of forgotten children, so we clear our
  // local cache of them, too.
  _forgottenChildren.clear();
}

So, it seems we are building the content:

final newContent = widget.content(_onContentBuildScheduled);

But since the layers are built during layout, they are not rebuilt.

One thing is that when update runs, if we haven't already laid out the content, then we don't have geometry info for the layers. So in that case, we'll need to invalidate layout upon update()

Calling markNeedsLayout on the RenderObject seems to make it work.

However, if the layout of content hasn't changed, then the previous geometry should still apply, and we should, in theory, be able to re-run build for the layers in the Element, outside of performLayout.

How should we determine that the previous geometry should still apply? Also, I tried calling buildLayers to only rebuild the layers, but then Flutter crashes with the exception:

════════ Exception caught by widgets library ═══════════════════════════════════
The following assertion was thrown building DocumentMouseInteractor(dependencies: [MediaQuery], state: _DocumentMouseInteractorState#59dbd):
'package:flutter/src/widgets/framework.dart': Failed assertion: line 3004 pos 12: '!_debugBuilding': is not true.

The relevant error-causing widget was:
    DocumentMouseInteractor DocumentMouseInteractor:file:///Users/angelosilvestre/dev/super_editor/super_editor/lib/src/default_editor/super_editor.dart:865:16

When the exception was thrown, this was the stack:
#2      BuildOwner.buildScope (package:flutter/src/widgets/framework.dart:3004:12)
framework.dart:3004
#3      ContentLayersElement.buildLayers (package:super_editor/src/infrastructure/content_layers.dart:277:12)
content_layers.dart:277
#4      ContentLayersElement.update (package:super_editor/src/infrastructure/content_layers.dart:358:5)
content_layers.dart:358
#5      Element.updateChild (package:flutter/src/widgets/framework.dart:3949:15)
framework.dart:3949
#6      Element.updateChildren (package:flutter/src/widgets/framework.dart:4098:32)
framework.dart:4098
#7      MultiChildRenderObjectElement.update (package:flutter/src/widgets/framework.dart:7080:17)
framework.dart:7080
#8      Element.updateChild (package:flutter/src/widgets/framework.dart:3949:15)

many more...

Maybe we are calling BuildOwner.buildScope when we are already inside a scope? I tried running the same logic from buildLayers, but without the owner!.buildScope and it seems to work. However, I'm not very familiar with the ContentLayers, so I'm not sure if this could cause any issue.

[matthew-carroll]

Do you know why update() doesn't cause a new layout pass?

In the situation that you're investigating do you know if the content widget is considered to be == in the update method? Specifically, I'm looking at the series of if-statements the begin here: https://github.com/flutter/flutter/blob/a0ba2decab156c88708c4261d40660ab8f60da5f/packages/flutter/lib/src/widgets/framework.dart#L3918

If the content widget is considered to be identical, then I suppose that would explain why layout isn't invalidated. But of content isn't considered to be identical, then I would expect the _content Element to mark its render object dirty, and trigger a layout and paint pass.

[angelosilvestre]

@matthew-carroll

Do you know why update() doesn't cause a new layout pass?

We do end up calling SingleColumnDocumentLayout.didUpdateWidget and SingleColumnDocumentLayout.build during update, but we are not running a layout pass, just a paint pass. What seems to be happening is that, even though we are running build, we are not making any change that causes a RenderObject to call markNeedsLayout. When switching to dark mode, the only thing we change is the font color, this causes the inner RenderParagraph to call markNeedsPaint, but not markNeedsLayout. If we change the dark mode styles to modify, for example, the font weight, then we cause RenderParagraph to call markNeedsLayout and a new layout pass happens.

Looking at LayoutBuilder, that also runs build during layout, it seems we should call markNeedsLayout ourselves:

@override
void update(ConstrainedLayoutBuilder<ConstraintType> newWidget) {
  assert(widget != newWidget);
  final ConstrainedLayoutBuilder<ConstraintType> oldWidget = widget as ConstrainedLayoutBuilder<ConstraintType>;
  super.update(newWidget);
  assert(widget == newWidget);

  renderObject.updateCallback(_rebuildWithConstraints);
  if (newWidget.updateShouldRebuild(oldWidget)) {
    _needsBuild = true;
    renderObject.markNeedsLayout();
  }
}

In the situation that you're investigating do you know if the content widget is considered to be == in the update method? Specifically, I'm looking at the series of if-statements the begin here:

No, we construct a new content widget during update.

[matthew-carroll]

We do end up calling SingleColumnDocumentLayout.didUpdateWidget and SingleColumnDocumentLayout.build during update, but we are not running a layout pass, just a paint pass. What seems to be happening is that, even though we are running build, we are not making any change that causes a RenderObject to call markNeedsLayout.

Ok. What I'm thinking is that in this particular situation we seem to be guaranteed that layout has already run at least once before, and nothing has changed in terms of size and location of layout. Therefore, in theory, there should be no issue with running build() on the overlays and underlays.

The remaining questions become:

• How do we identify this situation, so that we don't try to run build() when the layout is outdated?
• Where exactly should we run the overlay and underlay build()s?

WRT to the LayoutBuilder code:

  if (newWidget.updateShouldRebuild(oldWidget)) {
    _needsBuild = true;
    renderObject.markNeedsLayout();
  }

Do you know what updateShouldRebuild() is doing?

I'm wondering if we can do something like the following (pseudo code):

// Re-build the content.
content = update(content);

if (!content.needsLayout) {
  // Layout has already run. No layout bounds changed. There might be a
  // non-layout change that needs to be painted, e.g., change to theme brightness.
  // Re-build all layers, which is safe to do because no layout constraints changed.
  for (final underlay in _underlays) {
    underlay = update(widget.underlays[i](context, _previousConstraints));
  }
  for (final overlay in _overlays) {
    overlay = update(widget.overlays[i](context, _previousConstraints));
  }
}
// Else, dirty content layout will cause this whole widget to re-layout. The
// layers will be re-built during that layout pass.

// Remaining update code, like _forgottenChildren.clear()

[angelosilvestre]

@matthew-carroll

How do we identify this situation, so that we don't try to run build() when the layout is outdated?

This seems to be the hard question. It seems there isn't a way for us to query this information. I tried calling _isContentDirty after assigning the _content but it always returns false, even when changing the font size. If I print the RenderObject to the console, it prints "NEEDS LAYOUT`, but I found no way to query this information.

Where exactly should we run the overlay and underlay build()s?

Probably the same way we are doing in buildLayers, but without the owner!.buildScope.

Do you know what updateShouldRebuild() is doing?

The default implementation always returns true. It seems to be intended to be used by subclasses.

I guess we will probably need to always call markNeedsLayout...

[matthew-carroll]

it prints "NEEDS LAYOUT`, but I found no way to query this information.

Can you check what the print code is accessing to produce the text "NEEDS LAYOUT"? The answer to that question is what we'll need to query. If it's querying something like _debugNeedsLayout or debugNeedsLayout then please also check what non-debug info is tied to that, which we might try to query.

[angelosilvestre]

Can you check what the print code is accessing to produce the text "NEEDS LAYOUT"? The answer to that question is what we'll need to query. If it's querying something like _debugNeedsLayout or debugNeedsLayout then please also check what non-debug info is tied to that, which we might try to query.

It accesses _needsLayout. It's accessible via debugNeedsLayout, but this throws when called in release builder. I don't know why they need to hide this information in release...

[matthew-carroll]

Does layout invalidation always invalidate the parent? If so, can we create a custom render object that does nothing but expose that property, and then we query our render object that we used to wrap around the content?

[angelosilvestre]

@matthew-carroll It seems RenderContentLayers already exposes that, I updated this PR with a possible fix for the issue.

[matthew-carroll]

LGTM