Preserve correct order of block elements inside list items during view to model conversion #113
Conversation
|
I have removed some unnecessary code which was part of the logic of the first approach to this issue (see 76f50d0). |
src/converters.js
Outdated
| // Check all children of the converted `<li>`. At this point we assume there are no "whitespace" view text nodes | ||
| // in view list, between view list items. This should be handled by `<ul>` and `<ol>` converters. | ||
| for ( const child of viewChildren ) { | ||
| // If this is a view list element, we will convert it after last `listItem` model element. |
There was a problem hiding this comment.
I feel that some comments are outdated now. They might have been written by me (I don't remember) and then, after a few refactorings, they lost their meaning a bit. I'd change it to: "If this is a view list element, we will insert its conversion result after currently handled listItem.".
src/converters.js
Outdated
| if ( child.name == 'ul' || child.name == 'ol' ) { | ||
| nextPosition = conversionApi.convertItem( child, nextPosition ).modelCursor; | ||
| } | ||
| // If it was not a list view element it was a "regular" list item content. Convert it to a `listItem`. |
There was a problem hiding this comment.
Same here. In this function we convert children of listItem, not the listItem itself... So it doesn't make sense?
There was a problem hiding this comment.
🤔 The first part (if ( child.name == 'ul'...) refers to list view element:
If this is a view list element, we will insert its conversion result after currently handled
listItem.
and else refers to any element which is not a view list element, what the comment describes:
If it was not a list view element it was a "regular" list item content.
Only second part of it:
Convert it to a
listItem.
seems to be confusing as children are not converted to listItem (it was already inserted) but to listItem children. I will update this part.
src/converters.js
Outdated
| const result = conversionApi.convertItem( child, ModelPosition.createAt( lastListItem, 'end' ) ); | ||
| const convertedChild = Array.from( result.modelRange )[ 0 ]; | ||
|
|
||
| nextPosition = ModelPosition.createAt( lastListItem, 'end' ); |
There was a problem hiding this comment.
Isn't it the same as result.modelCursor? I honestly don't know, I think I haven't yet written a view-to-model converter after the refactor :P.
src/converters.js
Outdated
| // If it was not a list view element it was a "regular" list item content. Convert it to a `listItem`. | ||
| else { | ||
| const result = conversionApi.convertItem( child, ModelPosition.createAt( lastListItem, 'end' ) ); | ||
| const convertedChild = Array.from( result.modelRange )[ 0 ]; |
There was a problem hiding this comment.
Ugh, I hate that variable name! You don't obtain a child here, instead you obtain a tree walker value. The name is very misleading. Later in the code I was like "convertedChild.type === 'elementStart'? What's going on?". Then I got back and realized that above is not returning a child.
If you don't need tree walker value but just a node, you can use result.modelRange.start.nodeAfter. This should return you a node unless the range for some reason starts inside a text node, but that would mean that we had two view.Text nodes next to each other, which shouldn't happen.
Later in the code I see that you check convertedChild.type === 'elementStart' -- is it really needed? Won't it always be elementStart or text? If so, isn't it enough to check .is()?
src/converters.js
Outdated
| // so we need to update reference to `lastListItem` and `nextPosition`. | ||
| if ( convertedChild.type === 'elementStart' && convertedChild.item.is( 'element' ) ) { | ||
| nextPosition = result.modelCursor; | ||
| lastListItem = nextPosition.getAncestors().pop(); |
tests/listediting.js
Outdated
| @@ -325,8 +325,8 @@ describe( 'ListEditing', () => { | |||
|
|
|||
| describe( 'flat lists', () => { | |||
| describe( 'setting data', () => { | |||
| function test( testName, string, expectedString = null ) { | |||
| it( testName, () => { | |||
| function test( testName, string, expectedString = null, skip = false ) { | |||
There was a problem hiding this comment.
Instead of this you could just comment out a test or wrap it in describe.skip().
There was a problem hiding this comment.
BTW. it reminds that something similar was somewhere... probably undo tests in lists were skipped this way... But there at least half of a test was performed.
src/converters.js
Outdated
| // <listItem></listItem><paragraph>Foo</paragraph><listItem></listItem> | ||
| // | ||
| // so we need to update reference to `lastListItem` and `nextPosition`. | ||
| if ( convertedChild.type === 'elementStart' && convertedChild.item.is( 'element' ) ) { |
There was a problem hiding this comment.
One thing that I am not sure I like is future-proof of this if. Let's say that we will allow some kind of element in lists, or we introduce "document lists" (blocks allowed in lists). Maybe we could use Schema somehow to check if given model element is allowed at given position? But I am not sure now if conversionApi has reference to Schema.
|
@scofalik I have refactored the code according to your suggestions. It is ready for another review. |
Suggested merge commit message (convention)
Fix: Preserve correct order of block elements inside list items during view to model conversion. Closes ckeditor/ckeditor5#1263.
Additional information
The cause of the issue is described in ckeditor/ckeditor5#1263 (comment) and ckeditor/ckeditor5#1263 (comment). and more complex mechanics is explained in the comments inside the introduced function.