[GHS] Applying attributes to existing features - lists

[jacekbogdanski]

📝 Provide detailed reproduction steps (if any)

Due to the complex model structure of the Lists feature, it's not possible to easily extend it with additional attributes. We will need to create separate integration for it.

For now, it's possible to extend li elements. ul and ol elements are removed during upcasting to the model. We could probably preserve them on e.g. the first list item of the list and restore them during downcast conversion.

---

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

[Mgsy]

In order to handle this task, research will be required - #10073.

[jacekbogdanski]

Currently, we are blocked by #8829 and #8734

The issue is complex because the current lists merging logic is tied with the same converter as for wrapping list items with list elements.

Currently, most logic responsible for downcasting model list items to the data is done by modelViewInsertion method:

1. Every listItem model item results in generated ul->li view structure, so in the case of the model like:

<listItem>1</listItem>
<listItem>2</listItem>
<listItem>3</listItem>

We will get:

<ul><li>1</li></ul>
<ul><li>2</li></ul>
<ul><li>3</li></ul>

2. Depending on the context (if it's the first list item in the list with the same indent or not), the list view element will be inserted into the editor as it is, or merged with the previous list view element if it's of the same type (the same element name and class).

In case if it's the first list item with the same indentation in the surrounding content, HTML like:
<p>foo</p>[]<p>bar</p>
will be compiled to:
<p>foo</p>[<ul><li>1</li></ul>]<p>bar</p>

If it's the next item with the expected indentation confirming that the items should be merged:
<p>foo</p><ul><li>1</li></ul>[]<p>bar</p>
Lists will be merged:
<p>foo</p><ul><li>1</li><li>2</li></ul>[]<p>bar</p>

However, if lists are of different type or have different class name, they won't be merged:
<p>foo</p><ul><li>1</li></ul>[]<p>bar</p>
<p>foo</p><ul><li>1</li></ul><ol><li>2</li></ol>[]<p>bar</p>

Unfortunatelly, the above logic is hardcoded in a single conversion. It works well only to some extent, because list items are wrapped with lists and later merged based on the content context with no open window for any modification to view element attributes. When we are comparing lists in merging method:

ckeditor5/packages/ckeditor5-list/src/utils.js

Lines 174 to 176 in 6abebb6

	if ( !firstList || !secondList || ( firstList.name != 'ul' && firstList.name != 'ol' ) ) {
	return null;
	}

We are comparing fully converted firstList view element with freshly created secondList view element by

ckeditor5/packages/ckeditor5-list/src/utils.js

Lines 36 to 49 in 6abebb6

	export function generateLiInUl( modelItem, conversionApi ) {
	const mapper = conversionApi.mapper;
	const viewWriter = conversionApi.writer;
	const listType = modelItem.getAttribute( 'listType' ) == 'numbered' ? 'ol' : 'ul';
	const viewItem = createViewListItemElement( viewWriter );
	const viewList = viewWriter.createContainerElement( listType, null );
	viewWriter.insert( viewWriter.createPositionAt( viewList, 0 ), viewItem );
	mapper.bindElements( modelItem, viewItem );
	return viewItem;
	}

So we are not able to correctly compare attributes (only element names) as at this point, secondList was not fully processed.

The fix should cover 2 issues:

• Separating merging logic from the list items wrapping, so it's possible to inject custom converter before lists are merged which later would allow us to merge lists with actual attributes
• Distinguish list types based on element name, attributes, styles, and classes, not only element name and classes

I've already prepared POC for the fix, although many tests are failing so I could do some mistakes during refactoring or we have other converters (like for styling lists) which also require some additional changes. See POC on https://github.com/ckeditor/ckeditor5/tree/ck/8829-lists-merging branch.

[mmichaelis]

Thanks for your effort on this! Up to now GHS is a real benefit at many places. But, we also struggle with the current state as of 31.0.0:

For now, it's possible to extend li elements. ul and ol elements are removed during upcasting to the model. We could probably preserve them on e.g. the first list item of the list and restore them during downcast conversion.

You may already know, but just for reference, which must work for us, this following failed processing will occur as can be tested in the 31.0.0 Demo:

Input in source view:

<ol class="ol" lang="en" dir="ltr">
    <li class="li li1" lang="de" dir="rtl">ACME 1</li>
    <li class="li li2" lang="fr" dir="rtl">ACME 2</li>
</ol>

will end up as (after switching from source to view back and forth):

<ol>
    <li class="ol" dir="ltr" lang="en">ACME 1</li>
    <li class="ol" dir="ltr" lang="en">ACME 2</li>
</ol>

And as already mentioned above, we will get a false-merge for this:

<ol class="ol ol--first">
    <li class="li li--first">ACME</li>
</ol>
<ol class="ol ol--second">
    <li class="li li--second">ACME</li>
</ol>

to:

<ol>
    <li class="ol ol--first">
        ACME
    </li>
    <li class="ol ol--second">
        ACME
    </li>
</ol>

I really hope, you get this fixed with this non-existing ol/ul model representation.

[mmichaelis]

Just an additional thought: Make it a known behavior. Let implementors decide on how to deal with it.

Having the current 31.0.0 state, I could think of a workaround for us, which is adapting data-processing in a way, that data are presented to CKEditor, so that it can deal with it and resolve the workaround afterwards:

Data (similar to HTML):

<ol class="ol--first"><li>ACME</li></ol>
<ol class="ol--second"><li>ACME</li></ol>

Data View (thus, within CKEditor):

(after telling GHS, that <li> has a valid attribute data-ol-class)

<ol>
<li data-ol-class="ol--first">ACME</li>
<li data-ol-class="ol--second">ACME</li>
</ol>

Having this, we can transform data to data-view without loss (but with some effort in data-processing, though).

[Mgsy]

Support for GHS attributes in lists has been introduced in the Document list feature, so we decided to close this issue, as the integration is already done.