visual-space / visual-editor

Rich text editor for Flutter based on Delta format (Quill fork)
MIT License
290 stars 47 forks source link

Blocks - Size of Embed elements differs if alone or not #43

Closed apalala-dev closed 9 months ago

apalala-dev commented 2 years ago

There seem to be a difference between the size of Embed if they are alone on their line or not:

image

How I insert an Embed

I just added a FloatingActionButton to the example project with the below onPressed code:

final index = _controller?.selection.baseOffset ?? 0;
// final length = ()_controller?.selection.extentOffset ?? 0 - index;
setState(() {
  _controller?.document
      .insert(index, BlockEmbed('specialKey', 'someValue'));
});

The embedBuilder:

  Widget customEmbedBuilder(BuildContext context, EditorController controller,
      Embed node, bool readOnly) {
    switch (node.value.type) {
      case "specialKey":
        return Container(
            child: Text(node.value.data.toString()), color: Colors.green);
    }
    return Container(width: 50, height: 50, color: Colors.purple);
  }

Is this a bug?

Giving a fixed height to the Container doesn't fix it.

adrian-moisa commented 2 years ago

Hi @apalala-dev Thank you for reporting this detail. Never had to deal with this particular scenario. I'll have a stab at it once the burning refactoring topics are covered. If it's a massive blocker for you I can speed it up. Otherwise I'll let it flow for a while in the backlog.

apalala-dev commented 2 years ago

I only use this package in an hobby project and the issue is not really a blocker for me. However, I think the ability to insert Widgets in the text editor is one of the best addition from these kind of packages and atm it can lead to strange behaviours. I might work on it once you accept PRs.

kairan77 commented 2 years ago

hi, this has something to do with fully understanding flutter's render mechanic, and it does not have anything to do with quill.

in particular remember container go up, sizedbox go down, what you need is to wrap the container in with a center or align, so that the container 'knows' it should not occcupy the full row but just the space it naturally need. of course when you have two or more items in a row, the container sort of already knows it should not expand to take all the space.

adrian-moisa commented 2 years ago

I think there's no way you can change this behaviour from outside. What i recall seeing is that TextLine and TextBlock are rendered in a MultiChildRenderObjectWidget. So whatever you wrap on top of the editor wont make a diff. But maybe I don't understand what you suggest.

kairan77 commented 2 years ago

hi @apalala-dev , I suppose you mean width rather than height? could you please kindly check whether the following works in anyway? If it does not, I will take a look at it in my workspace.

 Widget customEmbedBuilder(BuildContext context, EditorController controller,
      Embed node, bool readOnly) {
    switch (node.value.type) {
      case "specialKey":
        return Row(
children: [
Container(
            child: Text(node.value.data.toString()), color: Colors.green), 
const Spacer(),
]
);
    }
    return Container(width: 50, height: 50, color: Colors.purple);
  }
apalala-dev commented 2 years ago

@kairan77 With your proposition, this is what we get:

image

The spacer just takes the whole place.

If I wrap the content with an Align, I can't write anything on the same line as the content (the later seems to take the whole place available).

image

I don't think it's related to the Container goes up etc. Even if it was, it's just not intuitive from an user perspective in a rich text editor. If I had a button, I expect it to take the same space wether it's on a new line or not.

Example with an ElevatedButton:

image

I made an example repo. You can replace the embed builder by one of the methods available in the code to see the different examples above.

kairan77 commented 2 years ago

@apalala-dev Tested this should work

Row(
        mainAxisSize: MainAxisSize.min,
        children: [
          Container(
            color: Colors.green,
            child: Text(node.value.data.toString()),
          ),
        ],
      )

just set the row to take up only spaces needed instead of being greedy by setting mainAxisSize to min, container when placed in row like widget alone would often expand to take up the full width, this is frequently the case dealing with things like sliver list builders and custom list builders, and in this case quillEmbed as well, you will see this throughout flutter once you come around to build complex nested tabs and list views. but again, in flutter generally speaking, there are always at least 3 ways to fix these issues and they are equally valid.

1 use size to constrain the collection holding foreign widget 2 modify the foreign widget 3 constrain the child passed to the foreign widget

let me know if you need more help

Screenshot_2022-07-16-17-41-17-058_ink guyi llll

apalala-dev commented 2 years ago

Just tested on my side, apparently t works. However, I think it is still strange to have to put everything in a Row to make it work on the client side. The behaviour should be the default one in my opinion. Adding a custom embed on a filled line or on a new line should give the same output for the embed. Maybe wrap any custom embed with this if it's the only solution?

kairan77 commented 2 years ago

Just tested on my side, apparently t works. However, I think it is still strange to have to put everything in a Row to make it work on the client side.

the design decision is really made by the flutter team, in which case when a single container is placed in a collection-like-holder, it always expand to take up all the space. They do give you the option to insert your own wrapper in between.

The behaviour should be the default one in my opinion.

It has something to do with listview being intuitive, which is much more common use case for these problem, imagine you have a column of cards, say each holding a book cover and some description, obviously the books tiltle would be of different length, and with your proposed general behavior , the cards would be of different width, and forcing the developer to specify a size for the card to make them all appear uniformly sized would be cumbersome when dev also have to deal with fitting their app to many different devices of different sizes, it's really the lesser of the two evil. overall flutter's current design on container / holder relationship result in lesser boiler plate code from my own experience.

yes, it's ok to wrap rows in rows in this case.

kairan77 commented 2 years ago

@apalala-dev @adrian-moisa maybe this can be closed?

adrian-moisa commented 2 years ago

I haven't had a chance to study this one. I'm stuck in other higher prio tasks. Will keep it open until I can double check. Unlike the original quill repo, I don't want to push tickets under the rug unless they are fully dealt with one way or the other.

apalala-dev commented 2 years ago

While I understand that the reason is how Flutter works, I still think that it's not natural for a rich text editor to have its elements take a different size wether they are alone on a new line or not.

In my opinion, Visual Editor should take care of wrapping embeds within a Row with mainAxisSize set to MainAxisSize.min if it's the only workaround available.

If you prefer to keep it like this, I think that it should be well highlighted in the doc.