Nested lists support #42
Conversation
…m. They will be removed in post fixer.
…l test for nested lists.
|
I'm afraid some tests started failing after we closed ckeditor/ckeditor5-engine#873. I restarted the build to check that it's not only my local problem. I had a quick look at the code and... WOW. That's a test suite :) I checked that some lines of code are called 6-8k times ;) |
|
Yep, some tests are failing now. And the old list tests were green on master according to https://travis-ci.org/ckeditor/ckeditor5/builds/212070784 so some of the new ones are failing. |
|
I've also encountered this:
This behaviour I got on this branch. On list#master the behaviour was a bit different – the middle list item totally vanished on undo. |
|
It was difficult for me to test integration with non-existing feature ;). As far as failing tests are concerned, all of them are for move change. I'll check it, it's probably connected with the recent changes we did to move conversion. |
|
Fixes #44. |
|
Is this one of these ignored cases? |
|
One more broken thing – select multiple items and press Tab. Only the first of the selected items get indented. Other than that... I've been able to achieve some "out of bounds index" errors couple of times, but I'm not able to repeat any of these scenarios. It happened in the nested lists manual test which contains that odd data (see https://github.com/ckeditor/ckeditor5-list/issues/45) and I haven't seen any bug if those items weren't present somewhere, so perhaps it doesn't make sense to try to investigate this further at the moment. EDIT, it happened once on paste (from gazeta.pl) and selection move. Maybe this screenshot will help you somehow:
So, this is really impressive how this feature works. It's the first version and you really need to push it hard to get some errors :). Believe me – this is a really big achievement 🥇 |
|
The bug with empty list is very sad, as it was working for most part of the time. One of the most recent commits must had broken it. Indenting just one item is a feature, actually :P. I mean it was designed to work like this. I don't know what I based it on, but if it looks like it's broken, then it needs to be fixed. As far as the error is concerned, you are right that it looks like something with selection. It's a pity that you haven't saved other error messages. Is there a possibility that the editor threw on undo/redo? Restoring selection is quite unstable ATM and might throw such errors (when model range is incorrectly restored and is then mapped to view).
It's kinda second version but I appreciate that you try to keep my morale up :P |
|
The bug with empty list is caused by imperfect model-to-view position mapping and the recent change in model-to-view remove converter (using |
|
Upon further research it appears that model-to-view position is ... heavily flawed. I haven't noticed it because it appeared that almost nothing in conversion uses model-to-view position mapping -- converters tried to use element mapping whenever is needed. Now we used position mapping in remove converter and actually a lot of cases are falling. I'll look at unit tests too. I am not sure whether it will be possible to fix it considering how |
I think it needs to be fixed. |
…test. Changed: Added conditional unbinding in model-to-view remove converter.
|
I've pushed a commit but it fails because it needs ckeditor/ckeditor5-engine#885 . The commit handles position mapping. I also expanded the position mapping test. This bug is fixed:
|
… items are selected.
|
Added indenting/outdenting of multiple items at once. |
…instead of it's own algorithm. Removed src/utils.js.
…items in that nested list.
|
Fixed #45. |
…elected list items affected by list item type change.
|
I found one quite important issue: https://github.com/ckeditor/ckeditor5-list/issues/51 and a scenario which I guess may throw at some point – https://github.com/ckeditor/ckeditor5-list/issues/48#issuecomment-289065138. Other than that, all worked fine. |
Suggested merge commit message (convention)
Feature: Nested lists support. Closes ckeditor/ckeditor5#2943. Closes ckeditor/ckeditor5#2942. Closes ckeditor/ckeditor5#2959. Closes ckeditor/ckeditor5#2963. Closes ckeditor/ckeditor5#2962. Closes ckeditor/ckeditor5#2965. Closes ckeditor/ckeditor5#2964. Closes ckeditor/ckeditor5#2967. Closes ckeditor/ckeditor5#2966. Closes ckeditor/ckeditor5#2968. Closes ckeditor/ckeditor5#2971.