Add remove formatting support for block elements styled using the GHS or style plugin

[Mati365]

Suggested merge commit message (convention)

Feature (remove-format): It should now be possible to remove formatting from block and container elements styled using the GHS or Style plugin. Closes #13983
Fix (remove-format): It should now be possible to remove styles applied to links. Closes #15318

---

Additional information

Based on comment, #13983 (comment)

[Witoso]

Page Break elements used together with GHS seems to be affected

What do you mean by affected? That with the remove format it's possible to remove their style/classes but they are inline elements?

[Mati365]

@Witoso

It looks like it's block element and:

become:

Because page-break class and page-break-after:always; style that are stored in GHS attribute are removed

[Witoso]

I wonder if #15318 as well with this solution?

[Mati365]

@Witoso I'm not sure, it looks like different issue. @niegowski what do you think?

[Witoso]

Yep, sorry, I didn't notice schema.isBlock( item ). It won't work on links, but I'm thinking if we should cover it. That way, the remove format would be fully complete.

CKEditor 4 is a good reference to check. It leaves the page break intact and doesn't remove its styles.

[Witoso]

This PR/prototype that was created shows that ideally we have some logic that allows us to mark some elements as formatting.

Problems that we may want to solve with this mini-project

• allow users to remove styling attributes on blocks
• allow users to remove styling attributes on links (inline blocks)?
• give API for defining which attributes are styling.

Reference to CKEditor 4: https://github.com/ckeditor/ckeditor4/blob/3fd8eafd8d805024ca3280fd95899833bb16b163/plugins/removeformat/plugin.js#L143-L182

[Mati365]

It turns out that there are many leftovers after consumption of model elements, and it needs to be fixed. It was causing the issue with page-break.

For example:

[Mati365]

@niegowski Can you take a look? At this moment we remove every class and style assigned via GHS. Should we focus on making it configurable? /cc @Witoso

[Witoso]

At this moment we remove every class and style assigned via GHS. Should we focus on making it configurable? /cc @Witoso

I think this would be an incremental value. We could create a follow-up ticket for this API.

[niegowski]

I'm not sure if a direct modification of this model element attribute is a safe approach. What if we modify the internal format of how we store that data? Maybe we could use an approach similar to the style feature:

ckeditor5/packages/ckeditor5-style/src/stylecommand.ts

Lines 201 to 209 in 08270aa

	if ( shouldAddStyle ) {
	htmlSupport.addModelHtmlClass( definition.element, definition.classes, selectable );
	} else {
	htmlSupport.removeModelHtmlClass(
	definition.element,
	getDefinitionExclusiveClasses( activeDefinitions, definition ),
	selectable
	);
	}

ckeditor5/packages/ckeditor5-html-support/src/generalhtmlsupport.ts

Line 131 in 08270aa

public removeModelHtmlClass( viewElementName: string, className: ArrayOrItem<string>, selectable: Selectable ): void {

[Mati365]

@niegowski I checked these methods and I see few problems:

1. These methods assume that the element contains only one GHS attribute, but there are cases when there are multiple formatting GHS on a single element. The example is list item [1]. At this moment, we remove all of them.
2. getGhsAttributeNameForElement is used in these methods, so if we map model element to view element, it turns out that for list items it's not working. For example, in the [1] case these paragraph elements are resolved later to htmlSpanAttribute GHS attribute name, so we can't remove list item styling despite it's visible.
3. I'm not sure if we are able to remove all classes or elements according to logic of modifyGhsAttribute function (that is used in removeModelHtmlClass) and this particular if: https://github.com/ckeditor/ckeditor5/blob/ck/13983/packages/ckeditor5-html-support/src/utils.ts#L180-L182

[1]

[niegowski]

Same as above I'd suggest using some GHS API for such checks. Example:

ckeditor5/packages/ckeditor5-style/src/styleutils.ts

Lines 159 to 164 in 08270aa

	public isStyleActiveForBlock( definition: BlockStyleDefinition, block: Element ): boolean {
	const attributeName = this._htmlSupport.getGhsAttributeNameForElement( definition.element );
	const ghsAttributeValue = block.getAttribute( attributeName );
	return this.hasAllClasses( ghsAttributeValue, definition.classes );
	}

[Mati365]

It'll not work for elements containing multiple GHS attributes, as in the comment above: #16867 (comment)

[niegowski]

I checked how it works on such payload:

<p style="border-left: 3px solid dodgerblue; padding-left: 5px;">
	foo
</p>
<div style="border: 2px solid orange; padding: 0 10px;">
	<p>bar</p>
</div>

And unfortunately, there are multiple issues:

1. It replaces the htmlPAttributes with an empty {} instead of removing that attribute from the model.
2. Only some styles are properly removed from the view by downcast converters.
3. The <div> element attributes are completely ignored (as in such case is a container, not a block).

[Mati365]

@niegowski I fixed these issues, can you take a look?