[SuperEditor][SuperReader] Widget overflow error for fixed height editor with scrollable content present within a parent scrollable.

[rutvik110]

Issue:

SuperEditor or SuperReader doesn't create an internal scrollable when an ancestor scrollable is present but isn't an direct parent of the editor and constraints passed down to the editor are tight. This is resulting in overflow issue mentioned below when the editor is given a fixed height using something like SizedBox and when editor has scrollable content.

Basic widget tree structure when editor has fixed height within an ancestor scrollable.

|---> Scrollable(e.g. ListView)
     |---> SizedBox(with fixed height)
            |---> SuperEditor/SuperReader
     ......

Reproducible example

void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: MyHomePage(),
    );
  }
}

class MyHomePage extends StatefulWidget {
  @override
  _MyHomePageState createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  late ScrollController _scrollController;
  late MutableDocument _doc;
  late MutableDocumentComposer _composer;
  late Editor _docEditor;

  @override
  void initState() {
    super.initState();
    _scrollController = ScrollController();
    _doc = _createInitialDocument();
    _composer = MutableDocumentComposer();

    _docEditor = createDefaultDocumentEditor(document: _doc, composer: _composer);
  }

  @override
  void dispose() {
    _doc.dispose();
    super.dispose();
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Builder(builder: (context) {
        return ListView(
          children: [
            SizedBox(
              height: MediaQuery.of(context).size.height * 0.5,
              width: double.infinity,
              child: const Placeholder(
                child: Center(
                  child: Text("Content"),
                ),
              ),
            ),
            SizedBox(
              height: 350,
              width: double.infinity,
              child: SuperEditor(
                document: _doc,
                composer: _composer,
                scrollController: _scrollController,
                editor: _docEditor,
              ),
            ),
            SizedBox(
              height: MediaQuery.of(context).size.height,
              width: double.infinity,
              child: const Placeholder(
                child: Center(
                  child: Text("Content"),
                ),
              ),
            ),
          ],
        );
      }),
    );
  }
}

MutableDocument _createInitialDocument() {
  return MutableDocument(
      nodes: List.generate(
    20,
    (index) => ParagraphNode(
        id: Editor.createNodeId(),
        text: AttributedText(
          "${index + 1} Scrollable content",
        )),
  ));
}

Overflow issue ss:

Expected behaviour:

To work with fixed height when present within an ancestor scrollable, SuperEditor or SuperReader should build an internal scrollable. Usually, an internal scrollable is created in absence of an ancestor scrollable, but for the scenario mentioned here this doesn't happen. User need to provide an another scrollable as a direct parent of the editor to resolve the overflow issue.

Proposed solution:

I think at the very top level, we'll need to address two things here. First of all we'll need to take into consideration that the ancestor scrollable if one's present may not be a direct parent of the editor.

Second is, not in all cases we'll need to create an internal scrollable if the ancestor scrollable is present. This may only be needed when the wrappers around editor are altering the constraints passed down to the editor like when using a SizedBox for fixed height on editor and resulting in an unexpected behaviour like the one mentioned here.

[rutvik110]

@matthew-carroll

[angelosilvestre]

I remember discussing this with @matthew-carroll a long time ago in https://github.com/superlistapp/super_editor/issues/462 and we ended up just fixing the app.

At the time, I modified the editor to create an internal scrollable if it has a maxHeight constraint. I might have the branch saved somewhere.

With this change we need to decide how the auto-scrolling should work:

1. Should it scroll only the internal scrollable?
2. Should it scroll the internal scrollable and, after reaching the end, scroll the ancestor scrollable?

If we go with option two, we need to change the auto-scroller to care about two scroll positions: the internal editor scroll position and the parent scroll position.

[rutvik110]

With this change we need to decide how the auto-scrolling should work:

1. Should it scroll only the internal scrollable?
2. Should it scroll the internal scrollable and, after reaching the end, scroll the ancestor scrollable?

If we go with option two, we need to change the auto-scroller to care about two scroll positions: the internal editor scroll position and the parent scroll position.

I think the second options sounds good for auto-scrolling. Currently, with SuperTextField we scroll the ancestor scrollable once we exceeds text field content when we use shortcuts to scroll within the text field. So, similar functionality for editor would be nice and along with auto-scrolling updates.

Though this sounds like a nice to have update rather than something that would be critical change for this issue. We could probably handle it separately once this issue is fixed. Wdyt? @angelosilvestre

[rutvik110]

At the time, I modified the editor to create an internal scrollable if it has a maxHeight constraint. I might have the branch saved somewhere.

Great! I'll have look at it once you can find it. This issue should mostly affect DocumentScrollable as that's where we decide whether we want to add an internal scrollable or not. Beside that everything else should remain unaffected.

[angelosilvestre]

The original PR is https://github.com/superlistapp/super_editor/pull/584. However, we should probably avoid using the LayoutBuilder and find another solution.

Though this sounds like a nice to have update rather than something that would be critical change for this issue. We could probably handle it separately once this issue is fixed. Wdyt? @angelosilvestre

We should wait for Matt's opinion for this ticket. I guess we probably want to do everything in one PR.

[matthew-carroll]

This is resulting in overflow issue mentioned below when the editor is given a fixed height using something like SizedBox and when editor has scrollable content.

How would this situation arise? What would it mean for a SuperEditor to have an explicit height, unrelated to its intrinsic content height, but then also be placed within an ancestor scrollable? What's the expected behavior in such a situation? Where do we see this situation in real life?

[rutvik110]

How would this situation arise?

For a SuperEditor that has explicit height and has content that exceeds that height, and is placed within a scrollable, we can see this issue occurring.

What would it mean for a SuperEditor to have an explicit height, unrelated to its intrinsic content height, but then also be placed within an ancestor scrollable?

First of all, editor creates an internal scrollable if there won't be any internal scrollable created as we have an ancestor scrollable in the widget tree. In that case, if the editors content ever exceeds the explicit height, we'll be facing the overflow issue. So, editor will need to handle this by checking for more than just presence of scrollable in the widget tree. Like, editor could check incoming constraints and decide whether it should create an internal scrollable or not.

What's the expected behaviour in such a situation?

If I'm setting a explicit height for the editor, then I would expect it to handle the exceeding content itself and allowing me to scroll through it as usual.

Where do we see this situation in real life?

It's pretty common to have an explicit height set when building WYSIWYG editors for rich text support in design tools or applications.

[matthew-carroll]

I think most of that response just repeats the same technical details but we still aren't getting to the use-cases. Please include a few real-world examples where one might need this behavior from SuperEditor.

[rutvik110]

ok, I'll gather some examples for this issue.

[matthew-carroll]

Ok, but let's not make that a priority. You can find those when you get a few extra minutes. Do you have tickets currently assigned or should I assign some to you?

[rutvik110]

None atm. You can go ahead and assign some.

[rutvik110]

I think most of that response just repeats the same technical details but we still aren't getting to the use-cases. Please include a few real-world examples where one might need this behavior from SuperEditor.

Few of the examples I can think of right now,

1. SuperEditor website demo

For the site demo, we've fixed this issue inside the site demo itself.

2. Github comment/description editor,

And I think this pattern is common enough when using document editors for us to fix this in the editor.

[matthew-carroll]

We spoke offline. We agreed that @rutvik110 will replicate the code necessary for end-users to fix this problem and place it in this thread. Given there's a workaround that apps can take to fix this, this is not a priority at the moment.

[rutvik110]

For now this issue can be resolved by wrapping the editor within it's own scrollable to fix the overflow issue,

SizedBox(
        height: 350,
        width: double.infinity,
        child: ListView(
          children: [
            SuperEditor(
              document: _doc,
              composer: _composer,
              scrollController: _scrollController,
              editor: _docEditor,
            ),
          ],
        ),
      )

Currently this workaround works fine on web as well mobile platforms, but for desktop we're encountering an issue where the overscroll within the editor won't scroll the page scrollable #1798. This desktop issue will be worked on in near future but isn't priority atm.