[Document lists] Make sure that lists isn’t split when making certain changes inside it

[niegowski]

Scenarios (some examples, there's definitely more of those):

• Widget type around, insertParagraph, insertTable, pasting images into empty list items, etc
• Inline image ↔ block image, moving list attributes to e.g. the paragraph is created.
• A scenario in handling Backspace:

* li bar
* li [block image]
* li foo

+ backspace

expected:

* li bar
* li []
* li foo

actual:

* li bar
p []
* li foo

Idea: Make paragraph inherit attributes allowed by the schema of the block/element that was removed.

• This could be implemented directly in deleteContent().
• And apply only in cases where we create some new element (e.g. merging one block into the preceding one doesn’t count).
• Do we need a flag? Maybe it should always be true?
• But then, what about conflicting attributes (allowed on many elements)?
  • GHS: htmlAttributes, and specific ones: htmlImgAttributes
• Introduce copyOnReplace in schema.setAttributeProperties()
  • And mark list* attributes

[CatStrategist]

Case 1 - inserting new elements

Inserting new elements splits a list into two because inserted elements don't have list attributes.
New elements are inserted through insertContent() function which doesn't know to add list attributes or copy them from the element where the selection is.

insertContent() checks if content to insert is allowed in a current selection's parent. Objects can't be inserted to paragraphs so selection is created after or before paragraph and a new element is inserted there. insertContent() is designed to remove empty paragraph after.

If selection is in list item with content it does not delete paragraph and only inserts new content.

Case 2 - inline image ↔ block image

Image plugin has a functionality to preserve attributes after type change, but inline image is inside a paragraph in a list item, so required attributes are on a paragraph and inline image can't pass them to block image.

When swapping block image to inline image it does not work because attributes are placed on inlineImage element instead of on a paragraph.

In both cases insertContent() is executed so solution may be the same as for case 1.

Case 3 - delete content

[Reinmar]

Meeting notes:

• The changes may not only affect deleteContent(), but also insertContent().
• The copyOnReplace may not be a valid name, but we can start with it. Let's check it later once we understand the cases that it needs to handle.
• @CatStrategist, you can start from building a list of scenarios to cover so we can prioritize them.

[CatStrategist]

I reviewed features from https://ckeditor.com/docs/ckeditor5/latest/features/index.html and list is breaking in following cases (still might not be all):

• Inserting table
• Inserting media
• Inserting image
• Inserting HTML block
• Inserting paragraphs by widget type around
• Pasting multiple paragraphs
• Pasting from Office
• Page break? Probably correct behaviour
• Inserting horizontal line
• Changing between inline and block image
• Deleting content that is replaced by paragraph (on delete)

Handling is needed in the deleteContent() and the insertContent() . Also need to review what happens when pasting text to a document list.

[CatStrategist]

Most cases are solved with copying list attributes of parent list item to newly inserted block element in insertContent(). Altough it solved most cases it also introduced a question - should we allow rewriting list attributes for all elements? For example page break might not be desirable in a list. It's not really an issue and doesn't break anything. Users can easly go around it - a list just won't automatically break.

Also rewriting attributes changed behaviour of pasting multiple paragraphs. Now instead of completely breaking a list, it adds the first pasted paragraph as one list item and the rest of paragraphs as second list item. It needs further investigation. 🕵️

[CatStrategist]

Another issue: Inserting page break and horizontal line create a new paragraph after - throwing user out of a list.

[CatStrategist]

Problem with insertContent()

Selectable that we receive in insertContent() might be already moved by getOptimalInsertionRange(). Selection ends up outside element that we want to insert to, losing context and we don't know source element that we should copy attributes from.

[CatStrategist]

Issues and cases that needs to be handled when we want to copy attributes using insertContent()

Case 1 - inserting widgets that use getOptimalInsertionRange()

This function sometimes calculates a range to insert to outside user's current selection. In this case we don't have access to element that we want to copy attributes from.

My current solution is to access an element to copy attributes from by accessing DocumentSelection in `insertContent(). This is the easieast one but after consulting it with @niegowski it may not be ideal and could introduce potential issues with collaboration.

If we won't proceed with the above, current alternative solutions are:
a) change getOptimalInsertionRange() to return Selection with attributes to copy instead of position/range.
This solution would introduce a braking change and would force usage of getOptimalInsertionRange() when inserting content.
b) calculate attributes to copy to outside insertContent() and pass them as parameter.
It would require every integrated feature to take care of retrieving attributes to copy.

Case 2 - widget typearound - no selection

When inserting paragraphs with widget typearound selection could be anywhere. We need to pass context from widget typearound which knows about an element that has attributes to copy.

Case 3 - copying attributes from parent element

Current example: when we change image block to inline image, we need to know to apply attributes to a paragraph surrounding inline image instead of applying them to the inline image.

[CatStrategist]

After further discusions we have some more options:

• Pass element to get attributes from to insertContent()
• Pass attributes to apply to insertContent()
• Use similar behaviour to retaining attributes from PastePlainText plugin

We need to decide what insertContent() should be responsible for.

[niegowski]

Usages of model.insertContent()

Inserting a widget (findOptimalInsertionRange() used only for block widgets)

• HorizontalLineCommand - DocumentSelection and optionally empty paragraph after it
• PageBreakCommand - DocumentSelection and optionally empty paragraph after it
• HtmlEmbedCommand - DocumentSelection (and select the widget)
  • Note: we don't use fOIR() yet here but we should
• InsertTableCommand - findOptimalInsertionRange() from DocumentSelection
• MediaEmbedCommand - findOptimalInsertionRange() from DocumentSelection
• ImageUtils.insertImage() - selectable or fallbacks to findOptimalInsertionRange() from DocumentSelection
  • Inserting inline image sometimes auto-paragraphs that image. How will it know from where to copy block attributes?

Widget type around

• InsertParagraphCommand - a position from options (created from a selected element and fake caret position)
• WidgetTypeAround - redirects insertContent to a selection created from a selected element and a fake caret position

Clipboard

• ClipboardPipeline - DocumentSelection

* This is probably out of scope.

Inline non-widget

• ShiftEnterCommand - selection.focus or selection.getFirstRange().end (if collapsed)

* This is probably out of scope.

Text

• InputCommand - range from options or DocumentSelection
• LinkCommand - DocumentSelection.getFirstPosition()
• MentionCommand - range from options or first selection range + ' ' after it
• ReplaceCommand - range from marker
• TextTransformation - range computed from match

* This is probably out of scope.

Idea

Maybe we should introduce a widget helper that would replace usage of findOptimalInsertionRange() + model.insertContent() (it would still use those internally) but would provide a single consistent API for widget inserting.

1. Would find the best insertion range (by using findOptimalInsertionRange() internally)
2. Would know what was the original target of the insertion and would be able to gather attributes that should be applied to the inserted content
3. Could optionally add an empty paragraph after the inserted object (in case there is no place to put selection)
4. Would call the model.insertContent() to insert the object with updated attributes that should persist and the optional empty paragraph.
5. Could optionally set the document selection after inserting an object (on it, in an empty paragraph, etc).

[Reinmar]

model.insertObject()?

Maybe, in the future, we could have more of these specialized methods: insertText(), insertBlock()? We'd need to be cautious to only cover cases that are distinctive in some way (e.g. what's the difference between insertContent() and insertBlock()?). But in this case (object vs content) the difference is quite visible.

[Reinmar]

The below will resolve all the issues that we have in the "Inserting a widget (findOptimalInsertionRange() used only for block widgets)" section.

Very rough idea how it could work:

model.insertObject( object, selectable, offset, {
    setSelection: 'on|in|after',
    findOptimalPosition: true,

    // Maybe:
    doNotInheritBlockAttributes: true
} );

// Simplified implementation of `insertObject()`:
function insertObject( object, selectable, offset, options ) {
    let targetPosition = selectableOrDocumentSelection();

    // Store attributes before we optimize the position.
    const attributesToCopy = findAttributesToCopy( targetPosition );

    if ( options.findOptimalPosition ) {
        targetPosition = findOptimalInsertionPosition( targetPosition );
    }

    model.deleteContent();

    if ( objectIsBlock ) {
        writer.setAttributes( attributesToCopy, object );
        thingToInsert = object;
    } else if ( cannotInsertObjectHere ) {
        block = autoParagraph( object )
        writer.setAttributes( attributesToCopy, block );
        thingToInsert = block
    } else {
        thingToInsert = block
    }

    model.insertContent( thingToInsert, targetPosition );

    // Finally, set selection based on options.setSelection.
}

[Reinmar]

As for the issues related to widget type around, the solution can be hardcoded by us in this feature. This is a very specific feature and most likely no one else does something similar.

[Reinmar]

    // Store attributes before we optimize the position.
    const attributesToCopy = findAttributesToCopy( targetPosition );

The remaining question is based on what we can figure out:

• where to take the attributes from?
  • the selection may be collapsed and non collapsed, inside a block, in a text, on an object in a block, between blocks, etc.
  • Ideas:
    • starting point:
      • startgetSelectedElement() || selection.anchor.start
      • search up the tree, get all attributes with the right props
    • maybe getSelectedBlocks()
      • question: does it work for a selected block image?
• where to set them?

[oleq]

Closed in #11425.