Block filler recognition is incorrect when converting from DOM to view

[scofalik]

📝 Provide detailed reproduction steps (if any)

Filler element   is not correctly removed when it was placed after <br /> and the data is loaded.

1. editor.setData( '<p>A<br></p>' ) or prepare such content in the editor.
2. editor.getData() -> '<p>A<br>&nbsp;</p>' -> this is fine, filler is needed.
3. editor.setData( '<p>A<br>&nbsp;</p>' )

✔️  Expected result

There is no space in the editor (in the model).

❌  Actual result

The filler   has not been removed and has been added as a space to the model.

What is wrong

Block filler element is not recognised properly. We have a mechanism that should remove this   when it was converted from DOM to view but the mechanism didn't work.

We should start from looking at the method that adds the filler. For a container element it is:

export function getFillerOffset() {
	const children = [ ...this.getChildren() ];
	const lastChild = children[ this.childCount - 1 ];

	// Block filler is required after a `<br>` if it's the last element in its container. See #1422.
	if ( lastChild && lastChild.is( 'element', 'br' ) ) {
		return this.childCount;
	}

	for ( const child of children ) {
		// If there's any non-UI element – don't render the bogus.
		if ( !child.is( 'uiElement' ) ) {
			return null;
		}
	}

	// If there are only UI elements – render the bogus at the end of the element.
	return this.childCount;
}

As you can see, there is some logic here, but tl;dr - the filler is added at the end of the element if there are no or only UI elements in the container or if there is <br> at the end of the container.

Actually, different types of elements have different rules for that. And AFAIK, it is possible to set a custom behavior.

The logic presented above is not reflected in the function that tries to recognise and remove fillers. It looks like this:

function isNbspBlockFiller( domNode, blockElements ) {
	const isNBSP = isText( domNode ) && domNode.data == '\u00A0';

	return isNBSP && hasBlockParent( domNode, blockElements ) && domNode.parentNode.childNodes.length === 1;
}

As you can see, the logic here is simpler, it just checks if this is an   in an empty element. It is not compatible with getFillerOffset() for container elements and it would get outdated again if getFillerOffset() changes.

Solutions

Quick solution - simply fix isNbspBlockFiller() to be more compatible with current implementation of getFillerOffset(). Since UI elements are most of the time (always?) not rendered in the data pipeline, we could just expand the check in isNbspBlockFiller() to also include a scenario when   is after <br>. This should cover most of the cases and is very small effort.

Unfortunately, marker elements are UI elements, so  s are inserted into elements that have only a marker (suggestions).

Good solution - change the flow of block fillers removing. Instead of skipping them when traversing converted DOM tree, convert them to view. Then use getFillerOffset() of the view elements and remove fillers from the view layer. This requires more effort and may result in some bugs.

And in this case we cannot do that because during upcast we don't have "semantic" view elements (attribute element, container element, etc.) so we cannot check getFillerOffset().

[scofalik]

It seems that the only good solution will be to mark block filler spans by wrapping them in additional spans.

[scofalik]

The final decision is to add additional wrapping spans only if a certain flag is turned on.