An error thrown when setting a data containing a list element with an inline font-weight (regression)

[oleq]

Is this a bug report or feature request? (choose one)

🐞 Bug report

💻 Version of CKEditor

11.2.0

📋 Steps to reproduce

1. https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/examples/builds/classic-editor.html
2. editor.setData( '<ol><li style="font-weight: bold">foo</li></ol>' )

✅ Expected result

Data is set. In 11.1.1 it wasn't quite valid but at least there was no error:

❎ Actual result

An error is thrown

converters.js:827 Uncaught TypeError: Cannot read property 'start' of null
    at converters.js:827
    at Ia.Jm (converters.js:369)
    at Ia.fire (emittermixin.js:196)
    at Ia._convertItem (upcastdispatcher.js:218)
    at Ia._convertChildren (upcastdispatcher.js:249)
    at Ia.La.upcastDispatcher.on (upcast-converters.js:583)
    at Ia.fire (emittermixin.js:196)
    at Ia._convertItem (upcastdispatcher.js:218)
    at Ia._convertChildren (upcastdispatcher.js:249)
    at Ia.La.upcastDispatcher.on (upcast-converters.js:583)

📃 Other details that might be useful

It's a regression of 11.2.0.

[scofalik]

Could be related to changes done by @f1ames, related with lists pasting?

[f1ames]

Yes, it seems the issue was introduced together with ckeditor/ckeditor5-list#113 PR. However, the cause is more general as @scofalik pointed out. What happens is:

1. The font-weight converter (from basic-styles plugin) is executed on <li> element.
2. The text itself is not yet converted.
3. Text gets converted to <p>foo</p> (looks like autoparagraphing mechanism kicks in).
4. Bold is set on text.
5. Now, the list converter is executed on the same <li> element.
6. Since the above converter also tries to convert <li> children and the text was already consumed/converted the error is thrown (as conversion result for the text is empty).

Now, there are few things to consider:

• Why attribute conversion is executed before element conversion?
• Is the bold converter in basic-styles plugin created/configured correctly or maybe it can be adjusted to solve this issue?

[f1ames]

When editor.setData( '<ol><li style="font-weight: bold">foo</li></ol>' ) is run the conversions order is as follows (--- convertItem:* indicates that conversion event was fired for the given item and doesn't mean that any converter was actually executed):

01 editor.setData( '<ol><li style="font-weight: bold">foo</li></ol>' )
02 --- convertItem:documentFragment {viewItem: DocumentFragment, modelCursor: Position, modelRange: null} DocumentFragment {_children: Array(1)}
03 --- convertItem:element ol Element {parent: DocumentFragment, name: "ol", _attrs: Map(0), _children: Array(1), _classes: Set(0), …}
04 OL/UL conversion - cleanList
05 --- convertItem:element li Element {parent: Element, name: "li", _attrs: Map(0), _children: Array(1), _classes: Set(0), …}
06 LI conversion - cleanListItem
07 AttributeConversion li {view: {…}, model: {…}, priority: undefined}
08 --- convertItem:text {viewItem: Text, modelCursor: Position, modelRange: null} Text {parent: Element, _textData: "foo"}
09 TEXT conversion - paragraph plugin autoparagraphing
10 --- convertItem:text {viewItem: Text, modelCursor: Position, modelRange: null} Text {parent: Element, _textData: "foo"}
11 LI conversion - viewModelConverter
12 --- convertItem:text {viewItem: Text, modelCursor: Position, modelRange: null} Text {parent: Element, _textData: "foo"}
13 Uncaught TypeError: Cannot read property 'start' of null

So in line 07, the font-weight from <li> is converted which then causes autoparagraphing the <li>'s text (line 09). One important thing to keep in mind is that both OL/UL conversion - cleanList and LI conversion - cleanListItem have high priority which is the reason they are executed before <li>'s AttributeConversion.

When font-weight converter from basic-styles plugin is disabled, lines 07-10 are not executed and everything converts fine (which means attribute conversion on <li> somehow triggers autoparagraphing - needs to be checked):

01 editor.setData( '<ol><li style="font-weight: bold">foo</li></ol>' )
02 --- convertItem:documentFragment {viewItem: DocumentFragment, modelCursor: Position, modelRange: null} DocumentFragment {_children: Array(1)}
03 --- convertItem:element ol Element {parent: DocumentFragment, name: "ol", _attrs: Map(0), _children: Array(1), _classes: Set(0), …}
04 OL/UL conversion - cleanList
05 --- convertItem:element li Element {parent: Element, name: "li", _attrs: Map(0), _children: Array(1), _classes: Set(0), …}
06 LI conversion - cleanListItem
07 LI conversion - viewModelConverter
08 --- convertItem:text {viewItem: Text, modelCursor: Position, modelRange: null} Text {parent: Element, _textData: "foo"}

and the same happens when li:viewModelConverter has priority higher then default one (e.g. { priority: 'high' }).

---

Another thing I have noticed is that autoparagraphing converter has the lowest priority, still it executes before the li one (LI conversion - viewModelConverter) which seems strange TBH.

[scofalik]

Isn't it because li converter explicitly converts it's children first and then it creates an element for itself? I am not looking into the code so this may be totally wrong :P.

[f1ames]

Isn't it because li converter explicitly converts it's children first and then it creates an element for itself? I am not looking into the code so this may be totally wrong :P.

I was thinking about it, but from what I see li is converted first - basically it is marked as consumed and then processed (see https://github.com/ckeditor/ckeditor5-list/blob/f5315c25a03a24d777a48e8079cb8926042ee1ca/src/converters.js#L340). And after that it's children are converted.

I think there are at least two issues here:

• The <li> children converter doesn't check if child can be consumed (however I'm not sure if that's mandatory during such conversions).
• The unexpected order of list children/paragraph converters.

☝️ I was wondering if the order is not caused by the fact that priority is respected only for the same type of converters. So when li font-weight is converted (attribute conversion) it also converts text (because it also converts children) inside which goes through all text converters and then element conversion on li occurs.

[f1ames]

• The <li> children converter doesn't check if child can be consumed (however I'm not sure if that's mandatory during such conversions).
• The unexpected order of list children/paragraph converters.

@scofalik Do you think we should treat it as two separate issues and go with the 1 point here (which restores the previous behaviour of setting such data). And then in another issue try to improve converters (investigate the execution order) so setting content as above will result in correct list?

[scofalik]

In upcast the situation is a little stinky cause we have the same event for elements upcasting and attributes upcasting. For this reason, we need to manage it using priorities. Attributes upcasting should happen always after elements upcasting. AFAICS the default priority for elementToAttribute upcast is normal. We should try changing it to low.