Follow ups with the List Style feature

[pomek]

TL;DR: Many cases I was able to cover on the model side. However, the conversion from the model to the view is a little buggy (the fourth issue).

---

• Empty undo step when executing commands in order: bulletedList, todoList. Can't be reproduced. Works fine.

In the beginning, I was thinking about introducing an additional attribute for the `listItem` element that would trigger the conversion and handling the list-style for lists. I went this way. Every `listItem` has the `listStyle` attribute with the `"default"` value if not specified.

The value is being read from the list parent element (ul/ol) during the upcast conversion. Executing it with the low priority ensures that the element is always correct. If a list has not specified the list-style-type value, the default one will be used. It works.

But the solution isn't perfect. When you add a todo-list item, the attribute still is present in the model. I used a post-fixer to delete the attribute (listStyle) for all todo-list items.

When creating a bulleted/numbered list item then executing the todoList command, the model state will be correct. Unfortunately, it created an empty undo step. This is my first issue.

• Integration with indent/outdent/bulleted/numbered lists command. See Follow ups with the List Style feature #7879 (comment).

The value of the `list-style-type` style does not have to match the type of list that it's being used. I mean, you can use the unordered list (`

`) with the `list-style-type: decimal` and it will use numbers as list markers.

The same operation you can do in the editor. It means if you used e.g unordered list with the square list-style then switched to a numbered list, the editor doesn't render any changes (only listType has changed for listItems elements).

The issue can be solved by restoring the default value for the listStyle attribute on changed items.

A code snippet:

• At the end of the LinkCommand#exexute() method, emit an event:

   /**
    * Event fired by the {@link #execute} method.
    *
    * It allows to execute an action after executing the {@link ~ListCommand#execute} method, e.g. adjusting
    * attributes of changed blocks.
    *
    * @event executeCleanup
    */
   this.fire( 'executeCleanup', blocks );

• And attach listeners:

   this.listenTo( editor.commands.get( 'bulletedList' ), 'executeCleanup', restoreDefaultListStyle( editor ) );
   this.listenTo( editor.commands.get( 'numberedList' ), 'executeCleanup', restoreDefaultListStyle( editor ) );
   
   function restoreDefaultListStyle( editor ) {
   	return ( evt, changedItems ) => {
   		changedItems = changedItems.filter( item => item.is( 'element', 'listItem' ) );
   
   		editor.model.change( writer => {
   			for ( const item of changedItems ) {
   				writer.removeAttribute( 'listStyle', item ); // setAttribute( 'listStyle', 'default', item );
   			}
   		} );
   	};
   }

It must be done in the same model.change() block that is used by ListCommand#execute() (one undo step for all operations). The same event-mechanism was used when indenting/outdenting the lists. This is the second issue since I am unsure about the solution.

• Integration with the Remove format feature. Moved to another ticket: The listStyle attribute for listItem should be marked as isFormatting #7894.

We could use the `RemoveFormat` feature to remove any applied style for the list.

editor.model.schema.setAttributeProperties( 'listStyle', {
	isFormatting: true
} );

However, the feature removes the attribute from the selection. If the selection is collapsed, only one list item will be changed. If non-collapsed, some items will be restored, some don't.

It won't be unified with the List style feature itself because you can apply the list style at any list item and the change will be applied to all items (that have the same indent, list type, and list style values).

At this point, I'm thinking the Remove format feature should not work with list styles (the third issue).

• Unifying the listStyle attribute while merging two lists.

The last issue is about merging lists. Considering a structure like this:

    <ul style="list-style-type: square;">
      <li>2</li>
      <li>3</li>
    </ul>
    <p>Paragraph</p>
    <ul style="list-style-type: disc;">
        <li>3</li>
        <li>4</li>
    </ul>

After deleting content from the paragraph, then remove the entire element, lists should not merge because they have different styles. The first idea @Reinmar had was overwriting the check whether lists should be merged. Unfortunately, it does not help.

While the first three issues aren't so urgent, they don't break the editor and everything still works and looks good, the last issue must be fixed before I'll start thinking about preparing the PR.

Originally posted by @pomek in #7801 (comment)

[pomek]

We agreed that executeCleanup isn't a perfect way to obtain items that should be adjusted. But, as for now, we can go with that solution. We marked the event as @protected and it was renamed to _executeCleanup, so when we will figure out a better solution, we will be able to replace the existing one without introducing major/minor breaking changes.

[pomek]

A case with the Remove format feature was moved to another ticket => #7894.

[scofalik]

Unifying the listStyle attribute while merging two lists.

Not sure if you are aware, but in this issue, if you undo, you have an incorrect list style in the view even though in the model it is fine.

[pomek]

Unfortunately, we're aware of invalid results in the view layer due to renderer.

[scofalik]

There is one more thing I am worried about.

At the moment, the feature works in a way, that there are two separate commands - list command (for bulleted and numbered lists) and list style command (for list style). If a block is not a list item, first list command is fired (and the block becomes a list item) and then list style command is fired.

This makes sense and it is a nice re-use of the code without copying the already existing and difficult logic.

Unfortunately, this is a little problematic for track changes. At the moment, list style is not integrated with track changes but if we would integrate it as usual, we would have two suggestions because we have two commands executed. This will not look nice, surely.

I have two ideas:

1. How about using list command inside the list style command? (A little worse from the track changes point of view but we could live with it).
2. How about extending list command so that list command handles styles? Style could be set as a parameter of list command call. Or we could even extend it by introducing subType for list commands (would be set in constructor just like the type).