-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce the List feature #2936
Comments
First decision to make is how list should be represented in model. 1. Mimic HTML implementation.We would have two elements: list and item. list element would have some attributes like controlling how list is displayed, like type=ordered|unordered, starting number. Indentation would be automatic. This implementation makes lists more encapsulated, it would be easier to write model<->view converters and control settings for given list. However, it could be more difficult to implement some functionalities like merging lists or "unlisting"/changing type of half of list. 2. Item element only.In this approach, we only introduce item element. The model would be simpler and list behavior, like merging lists or changing properties of only some items would be easier. Unfortunately model->view converters would be a bit tougher to write. Also the model would be heavier because each item would have to keep some duplicated data, like indentation or list type. Another drawback is that we would not be able to create two consecutive list of same type. This should not be a big thing, unless someone wants to create a list, make some space and create another list -- we'll need separating paragraph for that. 3. List element only.We could just introduce list element and treat paragraph elements as items. When converting to view, list converter will consume paragraphs inside it. This is variation of first approach. We don't need another model item. What we might gain is that there is already an implementation for how to merge paragraphs, so merging list items inside list would be done automatically. The risk is that, in future, there might be some attributes or functionallities that should work on paragraphs, but not on list items. 4. Attributes for paragraphs.Instead of introducing new elements, we could introduce a few attributes that would be applied straight to paragraphs. What we gain is that many things will work out of the box:
Fourth solution sounds most interesting to me, but I think it may be risky. I would go with second solution and see what behaviors that are now true for paragraphs will also work for list items. I suspect there should not be much difference between second and fourth solution and introducing new element may be more safe. |
How would nested lists behave in each approach? It was a huge pain to implement them in v4 with user–friendly, predictable in(outd)dentation behaviour, to preserve structure and support edge cases. |
Second and fourth approach are more friendly for nested list. All the nesting would be done by indent attribute. Then converters will handle creating/closing ULs/OLs then. They won't be easy, that's for sure, but operating on model will be, in that case. In first and third solution nested lists would be done like in HTML - by nesting lists. Nested list would probably end up straight in it's parent list, not item/paragraph:
|
Editor Recommendations does not specify how button state should behave or what should happen when list button is clicked while being inside list item, or while having some paragraphs/items selected. However I think we should implement same solutions as in CKE4, these are:
The question is how we decide which elements should be converted to list items. We need to make it flexible, so other, possibly 3rd party, plugins are able to co-operate. We have to enable converting, let's say, a couple of paragraphs and block quotes, to list items, to block quotes, back to paragraphs, etc. The other problem are elements that cannot be converted to list item, but can have list inside, i.e table cells. We won't have paragraphs in table cells so we have to know how far we can look-up from current position finding a parent that can be converted to list item. In such elements, we should not change list items back to paragraphs. Or maybe we could? I know that we are far from designing table feature but we have to know what we have to think about. There is also question "what if Paragraph feature is not enabled"... But maybe features like List or Blockquote should require it. Solutions that I see now:
If you have any other, better ideas please share! |
Regarding the model,
This is neglectable.
I've been thinking about that. First, in 99% cases you don't want to create separate lists one after another because there's no sense in that, so I'm strongly for forbidding those. Second, AFAIR Google Docs solved this by adding list ids to list items. If subsequent items have the same id, they are added to the same list. So this can be our solution if anyone will ever need such a future, which I doubt because CKEditor 4 is merging lists in many cases and I don't remember any report about that. You simply don't see whether two lists are separate. It may only matter if you loaded such a list using |
Yes, this is a conclusion that we came with after discussing the ticket with @pjasiun, so it's good that we all agree. I also noticed that CKE4 already behaves like this. Still, it is a nice to have a backup plan if we need it (list id). |
BTW, if we go with the 2nd solution, current implementations of the enter and backspace features should already handle most of the cases. Even many cases with nested lists should work, because in the model they will be represented by a flat list of elements (similar to paragraphs) and nicely merged or split. |
BTW2, regarding encapsulation of the most tricky logic. There's bazillion of cases with indenting one or many items, outdenting one or many items, outdenting by enter, outdenting by backspace, splitting items (enter), creating new items (enter), merging items (delete), removing pieces of tree (delete contents), etc, etc. to implement. In CKEditor 4 we've got a few kLOC which handle these cases and despite fixing many, many bugs, there are still some glitches in edge cases. The reason is that each of these algorithms has to care about details of how lists should be rendered and their complicated structure. Additionally, if you wished to add or change some behaviour of the list feature (and related features such as backspace and enter) you'd need to alter incomprehensible number of cases. Encapsulating this complex logic in converters will be extremely complex, I can imagine. But there's a closed set of operation and structures. We can start with flat lists (highly recommended) which will significantly speed up the initial work. Later on we'll have to spend some significant time on adding nested structures support, but we'll do this in one place, once. There will be bugs, I'm sure, but again – in one place :). |
As for the schema, I'd choose the easiest paths for now, as we'll be working on the schema improvements (https://github.com/ckeditor/ckeditor5-engine/issues/532) and current implementation doesn't have many features. E.g. – which elements can be converted to list items? Those which inherit from Just make sure to keep that logic in some meaningful methods so we can understand and easily update it in the future, once schema will be improved. Also, I propose to post all these cases that you'd like the schema to support under https://github.com/ckeditor/ckeditor5-engine/issues/532 so we've got a material to work on in the future. |
Option 2. looks good to me too. I only hope it will not become too tricky to implement. I'm also OK with the first implementation covering flat lists only and progressive enhancement in the future 👍 Anyway, as for DEL/BACKSPACE behavior, there's a lot of tests in v4, which could (should) be ported to v5 so, at least, the process figuring out all the possible cases would be done. The same with indent/outdent tests and enter tests in lists, which describe various transformations between nested lists, list items and underlying/neighbouring blocks. So there's a lot of stuff to study 😄 |
List item approach with
Let's say that one person outdents |
Another small issue is when you have two |
Aaand another issue, this time converting from view to model. When single list items are converted (say, somebody selected and copied one |
You mean – upon conversion? Yes, I also think this will be tricky. But that's exactly what makes it a perfect candidate for a code that you'd love to keep in one, safe place, not spread across many algorithms (which would need to merge the lists as well). |
This should be handled when copying a list item. In the clipboard it should rather be written together with its |
Hmmmmmmm..... hmm..... This is tricky. I even checked how it works in Google Docs. And we cannot compare with them, because they have an HTML-incompatible lists implementation. E.g you may have this: <listItem indent=0>a</listItem>
<listItem indent=2>b</listItem> So when we tested the case that you described, it didn't blow up – indent simply changed one item and outdent another one. None of them affected sublists. Having HTML/MD as a primary output we can't do stuff like this. I started wondering whether:
<paragraph>Foo</paragraph>
<listItem>1</listItem>
<listItem indent="1">1.1</listItem>
<listItem>1.2</listItem>
<listItem indent="-1">2</listItem> If I'm thinking correctly, if you're changing indentation of a single item it will only change one attribute. Never more, unless you created a non-collapsed selection, but even then you should be safer. We'll never get indent values that differs more than one level, because operation changing the attribute will either set |
I know that the idea is crazy and that the model will not be that intuitive, but it may solve other issues. |
The problem is that I don't know where this code should be put? Maybe in low-priority
But when you copy from outside source you might get Of course, when you copy flat set of list items, indentation does not matter. But you cannot paste a
This idea is not crazy and it is exactly what I've been thinking about after reporting problems with current solution. It solves not only OT problem but also indent problem (unfortunately not type problem). As I was thinking about it yesterday I came up with another problematic scenario: <listItem>1</listItem>
<listItem indent=1>1.1</listItem>
<listItem>1.2</listItem> Consider now user A outdenting item |
I don't know enough about converters to be sure about this, but late
The converters will focus on Besides, if we go with the indent=-1/1 approach, then indent doesn't have to be set. No ident means "follow your predecessor", which is what should actually happen when you paste such an item.
After the OT was resolved we'd get something like: <listItem>1</listItem>
<listItem>1.1</listItem>
<listItem indent=-1>1.2</listItem> Funny thing here is that the last item (1.2.) should actually become a paragraph and that's what the postfixer could do and that's what the model is actually saying. Because if you outdent a list item (when you have a flat list e.g.), then it should become a paragraph too. This looks really nice.
Command should never create an incorrect structure, so it's just a matter of fixing results that become incorrect because of OT. I think that those should be fixed ASAP. E.g. the case that we discuss above with one item at the end which has |
It's we who decide what is a correct model. If we decide that Anyway, I don't think that in described case last item should be converted to paragraph. It was neither users intent to do so. Both users after their changes see it as "level 0" item. |
Maybe you're right. Let's see then – we've got at least two options – ignoring this when converting or postifxing. |
So, we discussed more and came to the conclusion that since we need post-fixing for both solutions, we will try to go with more intuitive, "old" solution, that is that Now for something different. I've been thinking about cases and solving them for insertion converter. I tried (as I always do) to go with all the cases in the world, but then noticed that some are not possible (with only Here is the list, and let's make sure that's complete:
EDIT: There might be other scenarios for pasting, though. EDIT2: Keep in mind that we are only discussing creating new |
For scenario number 4., perhaps merging could be done in This will probably work nicely, if For scenario 2, to make it more general, what we can do is:
Then |
This solution will nicely cover 5. scenario. Newly created I guess I am slowly tearing this apart to small pieces. Hopefully I haven't make any mistake in the process :P |
For pasting, we need to make sure that first We need to handle one special case. In scenarios 2, 4 and 5 we are looking at previous Another thing to decide is what should be expected behavior when a list is pasted inside a list. Should new list items be pasted starting from
Should the result be like on left or like on right:
I think like on left, but this will need some callback that will be fired before/after pasting, because this cannot be handled in coverters. This fixing callback will have to check whether we are pasting inside a list and if so, fix indents of pasted content. Also three more things:
Consider pasting XYZ list in empty list item above. Expected result is:
not
|
Hopefully the last issue concerning pasting is dealing with list items of different types that are inserted inside existing list. This is not a problem for normal This is not true for pasting. You might get one of following cases:
In the first case, we have to map Good news is that after processing first item in pasted list, all the other will work following already described algorithm. |
I stumbled upon another problem with using just <paragraph>Lorem.</paragraph>
[<listItem>A</listItem>]
{<listItem>B</listItem>}
<paragraph>Ipsum.</paragraph> <p>Lorem.</p>
[<ul>
<li>A</li>]
{<li>B</li>
</ul>}
<p>Ipsum.</p> Because of how position mapping works, it's supper annoying to have view container elements that are not mapped to anything. If we interpret position I am sure we will find this problem a big, annoying issue. Right now I have a problem with removing first and last |
I see four solutions, for move/remove converters.
|
+1 for option 2. |
+1 for option 2. Option 3/4 was my original idea, but it appeared not to be doable. There is one action of moving/removing model elements and converters are fired after that action. The range is messy anyway because we need to calculate view range (which was not moved) based on the moved model range. If every node will be processed separately keeping in mind non-trivial mapping (like in this case) it may appear to be too hard to calculate proper ranges for each item. |
As I think about mapping there is another problem. The mapper was designed this way that you always map the position next to one element to the position next to another element. There is a ticket about mapping inner and outer differently (https://github.com/ckeditor/ckeditor5-engine/issues/266), but here we have another case. If you have in model:
And in view:
And the |
What we could do is improving mapper, so it can have fixed mapping for defined position, so the position after the last |
In fact you just map elements from model to elements from view and position is calculated on the fly. Current algorithm guarantees that you always get a position in top-most mapped element - if there is ambiguity. Why can't we treat it as expected behavior? Let's write tests, write it in documentation and call it a day. Instead of complicate it when it is not really needed by the feature. What we could do is pass |
We can try, but this behavior already changed multiple times, for instance, to put position in the text node if it is possible. But in may be the way to go.
I'm against it. First, the mapper is already complicated. Second: it solves nothing because default converter would always behave the same way, anyway. |
Those two behaviors do not affect each other that much. The only way it may happen is when text is directly next to element in model (and the element is "special", <listItem>foo</listItem>
<listItem>bar</listItem>
^<$text>someText</$text> <ul>
<li>foo</li>
<li>foo</li>
</ul>
someText In this case position after second I am well aware that this is not a perfect solution and this may change in future. We might introduce fixed positions in mapper, as you suggested. Or come up with something else. But for now we need something that works. This works and there are no real case scenario where it blows up. So for now I'd like to assume that this is expected behavior. It always behaves the same, so there's no fear it will work differently if there is different model structure. |
Just a small comment, without reading the previous posts – this is a totally legit case which the mapper should handle. Even if we wouldn't decide that lists will be represented in the model in this way, there's a very high chance that there will be other features like blockquote or code blocks or some widgets, in which such a mapping scheme would need to be supported. |
I only mean that we should keep mapper simple. We may try to revert decision about moving position to the text node and decide that mapper will always return the most outer possible position and it is view writer who ensures that position in text works fine. Current solution: "writer has to return the most outer possible position, unless it is position next to the text node, then the position should be the most inner" may works for now but will be terrible when we will add more features to mapper and want to refactor it. |
I'm lucky to have v. little knowledge about how it works so I can write "This can't be that hard." :D |
There is an issue with the approach we took with having only As an introduction to the problem I have to say that mapping model to view in this approach is difficult. Unfortunately, this is not true in our case, because <p>x[xx</p>
<ul>
<li>
aa]a
<ul>
<li>bbb</li>
<li>ccc</li>
</ul>
</li>
</ul> Model: <paragraph>x[xx</paragraph>
{<listItem indent=0>aa]a</listItem>}
<listItem indent=1>bbb</listItem>
<listItem indent=1>ccc</listItem> First of all, take a look at This leads to following problem. Look at selection
However, as we just said, it is impossible to map that range on view. There is another problem: after removing such range we would have incorrect model, because first What can be done?
What now? Honestly, I already hacked engine a bit during all the prototyping I've done. These are not big changes but I am worried that I am already trying too much to prove that we can get by with On the other hand, most stuff works correctly, there are some bugs with outdenting but nothing that big (and it should work on paper so if I haven't missed something this is and implementation bug). There are also small issues with undo but I am not sure if this is undo itself or converters were written without ackonwledging some corner cases that might come up when undoing. At this moment I am not sure whether it is better to scratch the idea and start from the beginning, or try to fix all the issues. If we start from the beginning, we would probably need more time to finish this feature (honestly, most of my work will go to waste, except maybe that I am now more experienced on what edge case scenarios exist). If we decide to continue with |
The question I'd ask is whether we can imagine any other feature which requires similar mapping schemes. E.g. we're pretty sure that widgets will require some tricks. In the view you'll want to have more things than you'll keep in the model. Also, we can imagine some integration scenarios where you may want to render some features differently than we designed it. We may want to implement blockquotes as attributes on headings and paragraphs (to avoid the same problems as with lists). This is just the beginning that we use the engine. Lists aren't some simple feature, so I can't say "if we can't implement lists this way, then the engine is broken", but this situation should definitely alarm us. Because there will be weird scenarios to solve and if engine requires that the model is mapped 1:1 to view (with the exception of widgets internals, where nested editables should be pretty easy to map), then perhaps the engine needs some improvements. Perhaps even drastic in some places, but this happens if a piece of software was written without a full knowledge about possible use case. We should also think from a bit different perspective. How we designed lists in the model sounds great from perspectives like operations on them and collaboration. The model is minimal and e.g. there are less invalid (from text editing perspective) positions than in the respective view. If we'll decide to change the model to align it to the view's structure, then we'll save time on engine corrections, but I guarantee that we'll then waste time on implementing features and we may hit serious troubles with OT, because model operations will be very complex for pretty frequent editing operations. As far as I understand, there are no issues which you consider unsolvable. If that's true, then IMO it's still reasonable to work on this. In fact, it will be totally understandable even if the whole mapper or something needs to be rethought. It will only become unreasonable to dig deeper if we'll understand that the changes in the engine require so much time or will improve complexity so much that time saved on implementing and supporting lists and other features will not exceed that. And I think that this can really be counted in months. |
Such selection is invalid. So we should only talk about this in terms of "if someone wants to delete such a range". This is of course possible, but shifts priorities.
The So don't worry if an existing behaviour doesn't cooperate well with lists. It can and even should be changed. Last but not least, I can imagine that at conversion level all operations on all possible ranges should at least not throw upon their conversion to the view. However, they don't have to have some really brilliant outcome unless these ranges are valid in editability terms. So e.g. a range that ends between I hope this helps :). |
I understand purpose of Moving is mostly connected to drag&drop and I belive there will be a solution similar to I don't understand why For now I will try to change conversion dispatching so that |
I think that you'll use deltas directly only in a very specific cases, where you operate on the content you know and on simple ranges which behaviour you can understand (which gets tricky once the content gets unpredictable). You'll e.g. never delete range of contents that your feature doesn't own. For that you'll use the generic In the future we'll have more of those higher order helpers. There will be a set of functions which represent the most basic operations – inserting, deleting and extracting the content.
D&D will perform extract & insert, because the content will need to go through the whole clipboard pipeline. No one should move a piece of content which isn't own by a controlled feature. E.g. we agreed that move operation's main use cases are other operations like wrap. |
PS. I may be a bit biased, because in CKE4 |
I meant the following selection: <paragraph>xa</paragraph>
[<listItem indent=0></listItem>]
<listItem indent=1>bbb</listItem>
<listItem indent=1>ccc</listItem> In the DOM you can't have such selection. Regardless of how you'll map it to the view, the selection must stay next to a text in those case. It can never be between blocks. OTOH, the position before first list item may be valid if e.g. you have an object that preceeds it: [<image></image>]
<listItem indent=0></listItem>
<listItem indent=1>bbb</listItem>
<listItem indent=1>ccc</listItem> So the image may be selected, so position between it and its sibling is valid. Deleting such image will need to leave the |
I know that
Right now engine requires that top-most element of a structure in view is mapped to model. So widgets are enabled (all widget internals are enclosed in one widget holder which is mapped to a model element). Unfortunately this One other possibility is to wrap <listItem indent=0>aaa</listItem>
<listItem indent=1>bbb</listItem> View: <ul>
<li>
<liContent>aaa</liContent>
<ul>
<li>
<liContent>bbb</liContent>
</li>
</ul>
</li>
</ul> HTML <ul>
<li>
aaa
<ul>
<li>bbb</li>
</ul>
</li>
</ul> This would be much easier to handle in mapping, because Anyway my point is that when you have elements that are siblings in model but children in view you have mess. Position mapping becomes unpredictible. I am really scared of that and I can imagine a lot of bugs coming out of that hole. |
TBH, I don't feel that I'm able to help you with the mapper... You can either bother @pjasiun or we'd need a longer talk so I can understand some limitations first. |
For example take this: <root>
<listItem indent=0>aaa</listItem>
<listItem indent=1>bbb</listItem>
<paragraph>xxx</paragraph>
</root> We will convert it to view. <div>
<ul>
<li>
aaa
<ul>
<li>bbb</li>
</ul>
</li>
</ul>
</div> In model, position for So I wrote a quick upgrade that let me specify those lengths. Now, <div>
<ul>
<li>
aaa
<ul>
<li>bbb</li>
</ul>
</li>
</ul>
<p>xxx</p>
</div> Now, consider mapping such position: <root>
<listItem indent=0>aaa</listItem>
^<listItem indent=1>bbb</listItem>
<paragraph>xxx</paragraph>
</root> This is parent Really, we could hack and smack but this looks like an unsolvable issue or an issue which solving takes too much effort. If anything, I'd try with transparent elements. |
Ok we decided that we will try to solve this problem by upgrading Callbacks will be fired whenever position is mapped from/to model/view. There will be separate callbacks for model->view mapping and for view->model mapping. If mapping request is not handled by callback, current algorithm will apply. The advantage of this solution is that we are not breaking anything that was working before and give maximum flexibility for future uses. However we fear that it may bring some efficiency problems as mapping are very very frequent task. |
Fixed by ckeditor/ckeditor5-list#6. |
Introduce List feature to CKEditor5.
Feature should follow Editor Recommendations:
http://ckeditor.github.io/editor-recommendations/features/numbered-list.html
http://ckeditor.github.io/editor-recommendations/features/bulleted-list.html
The text was updated successfully, but these errors were encountered: