Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add EmptyBlocks plugin that prevents adding   in exported data. #17756

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Mati365
Copy link
Member

@Mati365 Mati365 commented Jan 15, 2025

Suggested merge commit message (convention)

Feature (html-support): Add EmptyBlocks plugin that prevents adding   in exported data.


Additional information

It's part of https://github.com/cksource/ckeditor5-commercial/pull/6942
Original issue https://github.com/cksource/ckeditor5-commercial/issues/6941

More details: Notion

@Mati365 Mati365 marked this pull request as ready for review January 15, 2025 12:44
@Mati365 Mati365 changed the title Add EmptyBlocks plugin that allows for preserving empty block elements in the editor. Add EmptyBlocks plugin that prevents adding   in exported data. Jan 15, 2025
@Mati365 Mati365 requested review from niegowski and arkflpc January 15, 2025 13:05
Copy link
Contributor

@niegowski niegowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the comments below, the clipboard integration is missing.

Comment on lines +19 to +21
* Empty elements are detected during upcast and marked with a special attribute.
* During downcast, elements with this attribute have their `getFillerOffset` set to `null`
* which prevents adding block fillers.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This describes the internals of the plugin. Docs should focus on plugin usage.

Comment on lines +25 to +27
* * Preserve empty block elements exactly as they were in the source HTML
* * Allow for styling empty blocks with CSS (block fillers can interfere with height/margin)
* * Maintain compatibility with external systems that expect empty blocks to remain empty
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* * Preserve empty block elements exactly as they were in the source HTML
* * Allow for styling empty blocks with CSS (block fillers can interfere with height/margin)
* * Maintain compatibility with external systems that expect empty blocks to remain empty
* * Preserve empty block elements exactly as they were in the source HTML.
* * Allow for styling empty blocks with CSS (block fillers can interfere with height/margin).
* * Maintain compatibility with external systems that expect empty blocks to remain empty.

const editor = this.editor;
const schema = editor.model.schema;

// Register the attribute for block elements
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Register the attribute for block elements
// Register the attribute for block elements.

allowAttributes: [ EMPTY_BLOCK_MODEL_ATTRIBUTE ]
} );

// Upcast conversion - detect empty elements
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Upcast conversion - detect empty elements
// Upcast conversion - detect empty elements.

return;
}

const modelElement = modelRange?.start.nodeAfter as Element;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not yet support ?. operator.

Comment on lines +67 to +69
schema.extend( '$block', {
allowAttributes: [ EMPTY_BLOCK_MODEL_ATTRIBUTE ]
} );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered extending the $container? What about table cells? Those are not $block.

}, { priority: 'lowest' } );
} );

// Data downcast conversion - prevent filler in empty elements
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Data downcast conversion - prevent filler in empty elements
// Data downcast conversion - prevent filler in empty elements.

} );

// Data downcast conversion - prevent filler in empty elements
editor.conversion.for( 'dataDowncast' ).add( dispatcher => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the editing downcast?

if ( viewElement && data.attributeNewValue ) {
viewElement.getFillerOffset = () => null;
}
}, { priority: 'highest' } );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this priority or maybe it could be the default?

document.body.appendChild( element );

editor = await ClassicTestEditor.create( element, {
plugins: [ Paragraph, EmptyBlocks ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need more tests with more HTML element types, especially table cells. Missing tests for the editing pipeline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants