From 775b10e6ba1dcfb029e9c9b58a0a71b2ec5f1fa6 Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Thu, 12 Sep 2024 13:55:32 +0200 Subject: [PATCH 1/2] Pasting an inline widget as plain text no longer inserts additional empty lines before and after widget text. --- .../src/clipboardpipeline.ts | 2 +- .../src/utils/viewtoplaintext.ts | 16 +++++++++-- .../tests/utils/viewtoplaintext.js | 20 +++++++++++-- .../ckeditor5-engine/src/view/domconverter.ts | 18 ++++++------ .../tests/view/domconverter/domconverter.js | 28 +++++++++++++++++++ 5 files changed, 69 insertions(+), 15 deletions(-) diff --git a/packages/ckeditor5-clipboard/src/clipboardpipeline.ts b/packages/ckeditor5-clipboard/src/clipboardpipeline.ts index 99097453b2b..1bf8b863c1e 100644 --- a/packages/ckeditor5-clipboard/src/clipboardpipeline.ts +++ b/packages/ckeditor5-clipboard/src/clipboardpipeline.ts @@ -323,7 +323,7 @@ export default class ClipboardPipeline extends Plugin { this.listenTo( viewDocument, 'clipboardOutput', ( evt, data ) => { if ( !data.content.isEmpty ) { data.dataTransfer.setData( 'text/html', this.editor.data.htmlProcessor.toData( data.content ) ); - data.dataTransfer.setData( 'text/plain', viewToPlainText( data.content ) ); + data.dataTransfer.setData( 'text/plain', viewToPlainText( editor.editing.view.domConverter, data.content ) ); } if ( data.method == 'cut' ) { diff --git a/packages/ckeditor5-clipboard/src/utils/viewtoplaintext.ts b/packages/ckeditor5-clipboard/src/utils/viewtoplaintext.ts index bcd70a9b253..0d2a78e5f8f 100644 --- a/packages/ckeditor5-clipboard/src/utils/viewtoplaintext.ts +++ b/packages/ckeditor5-clipboard/src/utils/viewtoplaintext.ts @@ -7,7 +7,7 @@ * @module clipboard/utils/viewtoplaintext */ -import type { ViewDocumentFragment, ViewElement, ViewItem } from '@ckeditor/ckeditor5-engine'; +import type { DomConverter, ViewDocumentFragment, ViewElement, ViewItem } from '@ckeditor/ckeditor5-engine'; // Elements which should not have empty-line padding. // Most `view.ContainerElement` want to be separate by new-line, but some are creating one structure @@ -19,10 +19,14 @@ const listElements = [ 'ol', 'ul' ]; /** * Converts {@link module:engine/view/item~Item view item} and all of its children to plain text. * + * @param converter The converter instance. * @param viewItem View item to convert. * @returns Plain text representation of `viewItem`. */ -export default function viewToPlainText( viewItem: ViewItem | ViewDocumentFragment ): string { +export default function viewToPlainText( + converter: DomConverter, + viewItem: ViewItem | ViewDocumentFragment +): string { if ( viewItem.is( '$text' ) || viewItem.is( '$textProxy' ) ) { return viewItem.data; } @@ -44,7 +48,7 @@ export default function viewToPlainText( viewItem: ViewItem | ViewDocumentFragme let prev: ViewElement | null = null; for ( const child of ( viewItem as ViewElement | ViewDocumentFragment ).getChildren() ) { - text += newLinePadding( child as ViewElement, prev ) + viewToPlainText( child ); + text += newLinePadding( converter, child as ViewElement, prev ) + viewToPlainText( converter, child ); prev = child as ViewElement; } @@ -55,6 +59,7 @@ export default function viewToPlainText( viewItem: ViewItem | ViewDocumentFragme * Returns new line padding to prefix the given elements with. */ function newLinePadding( + converter: DomConverter, element: ViewElement, previous: ViewElement | null ): string { @@ -94,6 +99,11 @@ function newLinePadding( return ''; } + if ( !converter.isBlockViewElement( previous ) && !converter.isBlockViewElement( element ) ) { + // Don't add padding between non-block elements. + return ''; + } + // Add empty lines between container elements. return '\n\n'; } diff --git a/packages/ckeditor5-clipboard/tests/utils/viewtoplaintext.js b/packages/ckeditor5-clipboard/tests/utils/viewtoplaintext.js index fb364c28590..46555c14f87 100644 --- a/packages/ckeditor5-clipboard/tests/utils/viewtoplaintext.js +++ b/packages/ckeditor5-clipboard/tests/utils/viewtoplaintext.js @@ -3,14 +3,23 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ +import { DomConverter, StylesProcessor, ViewDocument } from '@ckeditor/ckeditor5-engine'; import viewToPlainText from '../../src/utils/viewtoplaintext.js'; import { parse as parseView } from '@ckeditor/ckeditor5-engine/src/dev-utils/view.js'; describe( 'viewToPlainText()', () => { + let converter; + + beforeEach( () => { + const viewDocument = new ViewDocument( new StylesProcessor() ); + + converter = new DomConverter( viewDocument ); + } ); + function testViewToPlainText( viewString, expectedText ) { const view = parseView( viewString ); - const text = viewToPlainText( view ); + const text = viewToPlainText( converter, view ); expect( text ).to.equal( expectedText ); } @@ -34,6 +43,13 @@ describe( 'viewToPlainText()', () => { ); } ); + it( 'should not put empty line between inline container elements', () => { + testViewToPlainText( + 'FooBar', + 'FooBar' + ); + } ); + it( 'should not put empty line before or after the element with `dataPipeline:transparentRendering` property', () => { const viewString = 'Abc Header xyz'; const expectedText = 'Abc Header xyz'; @@ -41,7 +57,7 @@ describe( 'viewToPlainText()', () => { const view = parseView( viewString ); view.getChild( 1 )._setCustomProperty( 'dataPipeline:transparentRendering', true ); - const text = viewToPlainText( view ); + const text = viewToPlainText( converter, view ); expect( text ).to.equal( expectedText ); } ); diff --git a/packages/ckeditor5-engine/src/view/domconverter.ts b/packages/ckeditor5-engine/src/view/domconverter.ts index 383f6a209e6..51ef4d9ca5a 100644 --- a/packages/ckeditor5-engine/src/view/domconverter.ts +++ b/packages/ckeditor5-engine/src/view/domconverter.ts @@ -766,7 +766,7 @@ export default class DomConverter { if ( viewChild !== null ) { // Whitespace cleaning before entering a block element (between block elements). - if ( this._isBlockViewElement( viewChild ) ) { + if ( this.isBlockViewElement( viewChild ) ) { this._processDomInlineNodes( domElement, inlineNodes, options ); } @@ -1162,6 +1162,13 @@ export default class DomConverter { return node && node.nodeType == Node.DOCUMENT_FRAGMENT_NODE; } + /** + * Returns `true` if a view node belongs to {@link #blockElements}. `false` otherwise. + */ + public isBlockViewElement( node: ViewNode ): boolean { + return node.is( 'element' ) && this.blockElements.includes( node.name ); + } + /** * Checks if the node is an instance of the block filler for this DOM converter. * @@ -1443,7 +1450,7 @@ export default class DomConverter { if ( this._isViewElementWithRawContent( viewElement, options ) ) { viewElement._setCustomProperty( '$rawContent', ( domNode as DomElement ).innerHTML ); - if ( !this._isBlockViewElement( viewElement ) ) { + if ( !this.isBlockViewElement( viewElement ) ) { inlineNodes.push( viewElement ); } @@ -1743,13 +1750,6 @@ export default class DomConverter { return this.isElement( node ) && this.blockElements.includes( node.tagName.toLowerCase() ); } - /** - * Returns `true` if a view node belongs to {@link #blockElements}. `false` otherwise. - */ - private _isBlockViewElement( node: ViewNode ): boolean { - return node.is( 'element' ) && this.blockElements.includes( node.name ); - } - /** * Returns `true` if a DOM node belongs to {@link #inlineObjectElements}. `false` otherwise. */ diff --git a/packages/ckeditor5-engine/tests/view/domconverter/domconverter.js b/packages/ckeditor5-engine/tests/view/domconverter/domconverter.js index 74865712241..1ddbe20cc68 100644 --- a/packages/ckeditor5-engine/tests/view/domconverter/domconverter.js +++ b/packages/ckeditor5-engine/tests/view/domconverter/domconverter.js @@ -18,6 +18,7 @@ import { StylesProcessor } from '../../../src/view/stylesmap.js'; import ViewPosition from '../../../src/view/position.js'; import ViewRange from '../../../src/view/range.js'; import { ViewText } from '@ckeditor/ckeditor5-engine'; +import { parse as parseView } from '@ckeditor/ckeditor5-engine/src/dev-utils/view.js'; describe( 'DomConverter', () => { let converter, viewDocument; @@ -425,6 +426,33 @@ describe( 'DomConverter', () => { } ); } ); + describe( 'isBlockViewElement()', () => { + const blockElements = [ + 'address', 'article', 'aside', 'blockquote', 'caption', 'center', 'dd', 'details', 'dir', 'div', + 'dl', 'dt', 'fieldset', 'figcaption', 'figure', 'footer', 'form', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'header', + 'hgroup', 'legend', 'li', 'main', 'menu', 'nav', 'ol', 'p', 'pre', 'section', 'summary', 'table', 'tbody', + 'td', 'tfoot', 'th', 'thead', 'tr', 'ul' + ]; + + const inlineElements = [ 'span', 'i', 'b', 'strong', 'a' ]; + + for ( const elementName of blockElements ) { + it( `should return true for <${ elementName }>`, () => { + const view = parseView( `<${ elementName }>` ); + + expect( converter.isBlockViewElement( view ) ).to.be.true; + } ); + } + + for ( const elementName of inlineElements ) { + it( `should return false for <${ elementName }>`, () => { + const view = parseView( `<${ elementName }>` ); + + expect( converter.isBlockViewElement( view ) ).to.be.false; + } ); + } + } ); + describe( 'shouldRenderAttribute()', () => { it( 'should allow all in data pipeline', () => { expect( converter.shouldRenderAttribute( 'onclick', 'anything' ) ).to.be.false; From 4ee6dd16d81ee487a9d5ca44f736afe0493df71f Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Fri, 20 Sep 2024 13:59:33 +0200 Subject: [PATCH 2/2] Apply CR remarks --- packages/ckeditor5-clipboard/src/clipboardpipeline.ts | 2 +- .../ckeditor5-clipboard/tests/utils/viewtoplaintext.js | 9 ++++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/ckeditor5-clipboard/src/clipboardpipeline.ts b/packages/ckeditor5-clipboard/src/clipboardpipeline.ts index 1bf8b863c1e..c420d030242 100644 --- a/packages/ckeditor5-clipboard/src/clipboardpipeline.ts +++ b/packages/ckeditor5-clipboard/src/clipboardpipeline.ts @@ -323,7 +323,7 @@ export default class ClipboardPipeline extends Plugin { this.listenTo( viewDocument, 'clipboardOutput', ( evt, data ) => { if ( !data.content.isEmpty ) { data.dataTransfer.setData( 'text/html', this.editor.data.htmlProcessor.toData( data.content ) ); - data.dataTransfer.setData( 'text/plain', viewToPlainText( editor.editing.view.domConverter, data.content ) ); + data.dataTransfer.setData( 'text/plain', viewToPlainText( editor.data.htmlProcessor.domConverter, data.content ) ); } if ( data.method == 'cut' ) { diff --git a/packages/ckeditor5-clipboard/tests/utils/viewtoplaintext.js b/packages/ckeditor5-clipboard/tests/utils/viewtoplaintext.js index 46555c14f87..8ded6a9a0b9 100644 --- a/packages/ckeditor5-clipboard/tests/utils/viewtoplaintext.js +++ b/packages/ckeditor5-clipboard/tests/utils/viewtoplaintext.js @@ -9,14 +9,17 @@ import viewToPlainText from '../../src/utils/viewtoplaintext.js'; import { parse as parseView } from '@ckeditor/ckeditor5-engine/src/dev-utils/view.js'; describe( 'viewToPlainText()', () => { - let converter; + let converter, viewDocument; beforeEach( () => { - const viewDocument = new ViewDocument( new StylesProcessor() ); - + viewDocument = new ViewDocument( new StylesProcessor() ); converter = new DomConverter( viewDocument ); } ); + afterEach( () => { + viewDocument.destroy(); + } ); + function testViewToPlainText( viewString, expectedText ) { const view = parseView( viewString ); const text = viewToPlainText( converter, view );