Push SliverToBoxAdapter down to SingleColumnDocumentLayout

[knopp]

Background: The way SuperEditor currently handles document widgets where all the widgets in the document are instantiated at all times is presenting a significant bottleneck to us. We aim to implement an alternative document layout based on super_sliver_list, but in order to accommodate a sliver inside layout some adjustments needs to be made to SuperEditor infrastructure.

This PR ensures that when SuperEditor is placed into a CustomScrollView, it propagates the sliver geometry all the way down to the layout, instead of relying to be wrapped inside a SliverToBoxAdapter.

Breaking use-facing changes introduced in this PR:

• To use SuperEditor inside a custom scroll view, it must no longer be wrapped in a SliverToBoxAdapter. That's because the SuperEditor itself acts like a sliver. When SuperEditor is used as "standalone" widget, no changes are necessary.
• It is no longer possible to use IntrinsicHeight around SuperEditor, as this does not work with CustomScrollView, which is now used instead of SingleChildScrollView in DocumentScrollable. Instead of that, shrinkWrap argument has been added to SuperEditor and SuperReader to accomplish similar behavior.
• Getting the document layout state and casting it to RenderBox will fail, because the layout is a RenderSliver.

Some implementation details:

SliverHybridStack widget was introduced to allow combining slivers and render boxes. Exactly on of the children in the stack must produce a RenderSliver, while other children produce RenderObjects. The render objects are laid-out and painted in a way where they cover the entire sliver scrollExtent.

ContentLayers has been modified to work similar to SliverHybridStack, where the content is a sliver while the overlays and underlays are render boxes. I think this is acceptable solution as the underlays and overlays are expected to be much sparsely populated then the content itself.

ViewportBoundsReporter and RenderViewportBoundsReplicator are gone. The viewport bounds are part of sliver constraints. SliverHybridStack has an optional mode which ensures that the renderboxes fill up entire viewport, which is enabled for gesture system RenderBox in case the entire works in standalone mode (not in user provided scrollable).

[matthew-carroll]

I haven't looked at the source code yet, but I have some thoughts based on the write-up. Mostly, I want to make sure that we don't over correct in the direction of slivers, to the detriment of those who would like to operate on boxes.

Getting the document layout state and casting it to RenderBox will fail, because the layout is a RenderSliver.

Unless I'm misunderstanding, this seems like a problem for those who want to work with boxes and not slivers. Boxes make it trivial to calculate offsets and assemble visual rectangle regions, so my first thought is that we don't want to eliminate the ability to treat a document as a box, if the user is actually using it as a box.

ContentLayers has been modified to work similar to SliverHybridStack, where the content is a sliver while the overlays and underlays are render boxes. I think this is acceptable solution as the underlays and overlays are expected to be much sparsely populated then the content itself.

I've long planned to move ContentLayers into its own package because I think it's generally useful. In so doing, I'm sure people will want to use boxes as the content. Should we instead look into the options to have two different content layer implementations - one for boxes and one for slivers? We'd need to chat about the optimal API for such a thing, but I thought I'd introduce this point at the start of the review.

[knopp]

I haven't looked at the source code yet, but I have some thoughts based on the write-up. Mostly, I want to make sure that we don't over correct in the direction of slivers, to the detriment of those who would like to operate on boxes.

Slivers are a superset of boxes. Whatever you want to do with boxes you can do with slivers.

Getting the document layout state and casting it to RenderBox will fail, because the layout is a RenderSliver.

Unless I'm misunderstanding, this seems like a problem for those who want to work with boxes and not slivers. Boxes make it trivial to calculate offsets and assemble visual rectangle regions, so my first thought is that we don't want to eliminate the ability to treat a document as a box, if the user is actually using it as a box.

We can expose the underlying box from the document if needed. But you can calculate those from slivers too if needed.

ContentLayers has been modified to work similar to SliverHybridStack, where the content is a sliver while the overlays and underlays are render boxes. I think this is acceptable solution as the underlays and overlays are expected to be much sparsely populated then the content itself.

I've long planned to move ContentLayers into its own package because I think it's generally useful. In so doing, I'm sure people will want to use boxes as the content. Should we instead look into the options to have two different content layer implementations - one for boxes and one for slivers? We'd need to chat about the optimal API for such a thing, but I thought I'd introduce this point at the start of the review.

This would make things much more complicated. Basically to be able to use sliver as layout, every render object produced by super editor all the way down to the layout must be a RenderSliver. If you want to use both boxes and slivers throughout to super editor you'll need to duplicate all the render objects and then pass along a flag to know which one to produce.

[matthew-carroll]

Slivers are a superset of boxes. Whatever you want to do with boxes you can do with slivers.

Let's dig into this claim a bit. Certainly, from an API standpoint, this isn't the case. RenderSliver extends RenderObject and RenderBox extends RenderObject. It's not the case that RenderSliver extends RenderBox. For easy reader reference, here are some API links:

https://api.flutter.dev/flutter/rendering/RenderObject-class.html

https://api.flutter.dev/flutter/rendering/RenderBox-class.html

https://api.flutter.dev/flutter/rendering/RenderSliver-class.html

---

Looking beyond the inheritance differences, let's look at a few methods and properties.

It's very common to want to call localToGlobal and globalToLocal. These methods don't exist on RenderSliver. Perhaps that's an equally simple way of achieving the same result, but I'm not sure what it is. Can you point that out?

RenderBox exposes its size, which, again, is very useful. Given the localToGlobal and size, it's trivial to find the global bounding rect for a given box. However, RenderSliver doesn't have a size property. It has a getAbsoluteSize() method, but that's marked "protected". RenderSliver has a geometry, but that geometry doesn't have a size property. Instead, the geometry reports things like paintExtent, layoutExtent, cacheExtent, etc. Is there a combination of these properties that would recover the expected return value of size on a RenderBox?

We've already talked a little bit about intrinsic width and height. While it may be acceptable for a specific chosen SuperEditor layout to use slivers, and therefore not support intrinsic measurement, I think it's reasonable that a developer might want a SuperEditor layout that does support intrinsic measurement. Is there a workaround for this with slivers? If the choice between boxes and slivers is provided, then this point doesn't matter. But if we're essentially migrating exclusively to slivers, then this does matter.

---

I have some concerns in other areas as well, but let's start by aligning on what the relationship really looks like between slivers and boxes. I want to make sure we have a crystal clear picture for everyone involved as to what it means to eliminate the guarantee of boxes and force people to either work with slivers, or force people to work only with generic RenderObjects.

[matthew-carroll]

Let me also loop in some other stakeholders here for visibility. CC @Jethro87 @jmatth

[knopp]

@matthew-carroll, please look at render_object_ext inside the PR. It implements size, hasSize, globalToLocal and localToGlobal on RenderObject, which can be either a RenderSliver or RenderBox.

[knopp]

The alternative for intrinsic height is shrinkWrap. As is common for other scrollable widgets.

[matthew-carroll]

The alternative for intrinsic height is shrinkWrap. As is common for other scrollable widgets.

But shrinkWrap is a request, not a query, right? Whereas a RenderBox getMaxIntrinsicWidth is a query, rather than a request?

[knopp]

The alternative for intrinsic height is shrinkWrap. As is common for other scrollable widgets.

But shrinkWrap is a request, not a query, right? Whereas a RenderBox getMaxIntrinsicWidth is a query, rather than a request?

Yes. ShrinkWrap will cause the scrollview to size itself to content, but does not allow querying intrinsic size. The question is, whether this difference is significant enough in practice to warrant supporting both slivers and boxes through-out super editor, which would increase the complexity significantly and add pretty big surface for runtime errors in cases where sliver and boxes would be mixed by mistake.

[matthew-carroll]

The question is, whether this difference is significant enough in practice to warrant supporting both slivers and boxes through-out super editor, which would increase the complexity significantly and add pretty big surface for runtime errors in cases where sliver and boxes would be mixed by mistake.

I agree that's the final question. I'm trying to start by putting all the tradeoffs on the table.

I'll take a look at your implementation of box stuff in the sliver extensions.

[knopp]

I tried running Superlist with this branch, and apart from making sure there are no render boxes outside of editor (which is a given) the only change I needed was to not make assumption about the layout object render type:

That meant changing code that positions overlay from

    final docBox =
        _documentLayoutKey.currentContext?.findRenderObject() as RenderBox?;
    if (docBox == null) return;

    final position = Rect.fromPoints(
      docBox.localToGlobal(selectionBoundingBox.topLeft),
      docBox.localToGlobal(selectionBoundingBox.bottomRight),
    );

Into

    final layout = _documentLayoutKey.currentState as DocumentLayout;
    final position = Rect.fromPoints(
      layout.getGlobalOffsetFromDocumentOffset(selectionBoundingBox.topLeft),
      layout.getGlobalOffsetFromDocumentOffset(selectionBoundingBox.bottomRight),
    );

[matthew-carroll]

@knopp I think the most reasonable path forward is to just land this and see how it shakes out. If we need to bring more box stuff back in, we can.

Can you address the few minor comments that were originally made?

[matthew-carroll]

@knopp ping on this

[knopp]

Apologies for the delay. Right now trying to dogfooding this in superlist to figure out if there are some loose ends that need to be addressed in this PR.

[knopp]

The failing test is because of https://github.com/superlistapp/super_editor/issues/2056. By a coincidence the existing tests didn't catch the regression in main, but did catch it with changes in this PR.

[matthew-carroll]

@knopp should I go ahead and merge this? I think the best we can do on my side is to see what happens - I'm not sure there's much I can provide that's useful on the review front.

[matthew-carroll]

I think we said on our call that the sliver padding should be reverted because it's now available on stable. After that I think we can merge in.

[knopp]

@matthew-carroll, can you approve the PR?

[matthew-carroll]

@angelosilvestre can you cherry pick this?

[KevinBrendel]

For us (in QuikFlow) this change is very problematic, because we rely on being able to compute the dry layout for sizing our nodes correctly. We have a custom layout for this using boxy. We do not use any traditional scrolling (we transform the whole canvas including all widgets).

As far as I can tell, there is no way to compute the dry layout now?

[matthew-carroll]

@knopp can you address @KevinBrendel's use-case? We'll want to find a way to re-enable their use-case.

[knopp]

@KevinBrendel, can you be more specific about the use-case? Do you calculate dry layout multiple times with different constraints or do you just need super_editor to take required height for given width?

Ultimately the layout here is still a RenderBox, so it should be possible to skip all the slivers get the dry layout from the RenderBox.

[KevinBrendel]

@knopp Yes, we first calculate the dry layout of SuperEditor and some other widgets with one set of constraints, then, based on the resulting sizes, we define new constraints and perform a layout again.

Exactly, in our case, we would like to just skip past the slivers and get to the RenderBox directly. We are fine with losing all of the optimization that comes from slivers in that case. Is this already possible somehow?

[knopp]

@KevinBrendel, I think something like this should work for your use case:

class FakeViewport extends SingleChildRenderObjectWidget {
  FakeViewport({required super.child});

  @override
  RenderObject createRenderObject(BuildContext context) {
    return _RenderFakeViewport();
  }
}

class _RenderFakeViewport extends RenderBox
    with RenderObjectWithChildMixin<RenderSliver>
    implements RenderAbstractViewport {
  @override
  void debugAssertDoesMeetConstraints() {}

  @override
  RevealedOffset getOffsetToReveal(RenderObject target, double alignment, {Rect? rect, Axis? axis}) {
    return RevealedOffset(offset: 0, rect: Rect.zero);
  }

  @override
  void setupParentData(RenderObject child) {}

  @override
  Rect get paintBounds => Rect.zero;

  @override
  void performLayout() {
    final childConstraints = SliverConstraints(
      axisDirection: AxisDirection.down,
      growthDirection: GrowthDirection.forward,
      userScrollDirection: ScrollDirection.forward,
      scrollOffset: 0,
      precedingScrollExtent: 0,
      overlap: 0,
      remainingPaintExtent: constraints.maxHeight,
      crossAxisExtent: constraints.maxWidth,
      crossAxisDirection: AxisDirection.right,
      viewportMainAxisExtent: constraints.maxHeight,
      remainingCacheExtent: double.infinity,
      cacheOrigin: 0,
    );
    child!.layout(childConstraints, parentUsesSize: true);
    final geometry = child!.geometry;
    size = Size(constraints.maxWidth, geometry!.scrollExtent);
  }

  RenderBox _getBox(RenderSliver sliver) {
    RenderSliver? firstSliver;
    RenderBox? firstBox;
    sliver.visitChildren((child) {
      if (child is RenderSliver && firstSliver == null) {
        firstSliver = child;
      }
      if (child is RenderBox && firstBox == null) {
        firstBox = child;
      }
    });
    return firstSliver != null ? _getBox(firstSliver!) : firstBox!;
  }

  @override
  Size computeDryLayout(covariant BoxConstraints constraints) {
    final layoutBox = _getBox(child!);
    return layoutBox.computeDryLayout(constraints);
  }

  @override
  void applyPaintTransform(RenderObject child, Matrix4 transform) {}

  @override
  bool hitTestChildren(BoxHitTestResult result, {required Offset position}) {
    return child!.hitTest(
      SliverHitTestResult.wrap(result),
      mainAxisPosition: position.dy,
      crossAxisPosition: position.dx,
    );
  }

  @override
  void paint(PaintingContext context, Offset offset) {
    context.paintChild(child!, offset);
  }

  @override
  void performResize() {}

  @override
  Rect get semanticBounds => Offset.zero & size;
}

Wrap SuperEditor inside FakeViewport and the render object should forward dry layout request to the actual layout render box. This assumes that there is an actual scrollable somewhere above so that super editor doesn't try to create it's own scrollable. I'd assume that you already have a scrollable in hierarchy in your case, right?

[KevinBrendel]

@knopp Thanks! This definitely goes into the right direction.

We actually do not have a scrollable in our hierarchy. Our app is canvas-based and we have it all in an InteractiveViewer with caching of the widgets to keep it performant. Some of our users have hundreds of nodes, each with their own SuperEditor instance.

I had to wrap it in a SingleChildScrollView to get it to compile. This kind of works, but we lose the intrinsic width of the editor, which is necessary to size the nodes. Essentially, only fixed-width nodes are possible.

Here is a small video to better demonstrate the use-case: https://github.com/user-attachments/assets/bcb4148e-3a82-4643-ab51-4abd5c96f987

But with this workaround, auto-width nodes seem no longer possible: https://github.com/user-attachments/assets/1e75ae05-37e9-4958-8d75-7131dac5f50e

We just released an initial version of QuikFlow with SuperEditor integration. If you want to play around with it yourself, just tell me your OS and I can send you a free key for the Pro version.

[knopp]

If you don't have scrollable in hierarchy, SuperEditor will create a single child scroll view. I think maybe you'd need a way to disable this behavior completely since you don't want to have any kind of scrolling there.

Intrinsic width is doable same way dry layout is. You just implement computeMax/MinIntrinsicWidth and forward the call to the box obtained by _getBox (same way dryLayout does it).

[KevinBrendel]

Ahh, thanks!

Indeed the behavior seems correct now that I have implemented the four intrinsic functions. Probably it would make sense to have an example somewhere with this widget for other people with similar issues.

Will have to test for a noticeable performance impact, but indeed it would be great to be able to disable the scrolling to avoid having to use the ScrollView workarounds.