Browse Source

Added handling for a special case when <br> in a block element should be treated as a block filler even in the nbsp mode.

Piotrek Koszuliński 6 years ago
parent
commit
f3fb795ed5

+ 13 - 1
packages/ckeditor5-engine/src/view/domconverter.js

@@ -807,11 +807,23 @@ export default class DomConverter {
 	 *
 	 * **Note:**: For the `'nbsp'` mode the method also checks context of a node so it cannot be a detached node.
 	 *
+	 * **Note:** A special case in the `'nbsp'` mode exists where the `<br>` in `<p><br></p>` is treated as a block filler.
+	 *
 	 * @param {Node} domNode DOM node to check.
 	 * @returns {Boolean} True if a node is considered a block filler for given mode.
 	 */
 	isBlockFiller( domNode ) {
-		return this.blockFillerMode == 'br' ? domNode.isEqualNode( BR_FILLER_REF ) : isNbspBlockFiller( domNode, this.blockElements );
+		if ( this.blockFillerMode == 'br' ) {
+			return domNode.isEqualNode( BR_FILLER_REF );
+		}
+
+		// Special case for <p><br></p> in which case the <br> should be treated as filler even
+		// when we're in the 'nbsp' mode. See ckeditor5#5564.
+		if ( domNode.tagName === 'BR' && hasBlockParent( domNode, this.blockElements ) && domNode.parentNode.childNodes.length === 1 ) {
+			return true;
+		}
+
+		return isNbspBlockFiller( domNode, this.blockElements );
 	}
 
 	/**

+ 43 - 0
packages/ckeditor5-engine/tests/tickets/5564.js

@@ -0,0 +1,43 @@
+/**
+ * @license Copyright (c) 2003-2019, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
+ */
+
+import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
+import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
+import ShiftEnter from '@ckeditor/ckeditor5-enter/src/shiftenter';
+
+import { getData as getModelData, setData as setModelData } from '../../src/dev-utils/model';
+
+describe( 'Bug ckeditor5#5564', () => {
+	let editor;
+
+	beforeEach( () => {
+		return VirtualTestEditor
+			.create( { plugins: [ Paragraph, ShiftEnter ] } )
+			.then( newEditor => {
+				editor = newEditor;
+			} );
+	} );
+
+	afterEach( () => {
+		return editor.destroy();
+	} );
+
+	it( 'does not create an excessive new line when loading <p>x</p><p><br></p><p>x</p>', () => {
+		editor.setData( '<p>x</p><p><br></p><p>x</p>' );
+
+		expect( getModelData( editor.model ) ).to.equal(
+			'<paragraph>[]x</paragraph><paragraph></paragraph><paragraph>x</paragraph>'
+		);
+	} );
+
+	it( 'preserves a soft break in an empty paragraph', () => {
+		setModelData( editor.model, '<paragraph>x</paragraph><paragraph><softBreak /></paragraph><paragraph>x</paragraph>' );
+
+		expect( editor.getData() ).to.equal( '<p>x</p><p><br>&nbsp;</p><p>x</p>' );
+
+		// Loading this data into the editor will actually create an excessive space as &nbsp; here isn't recognized as a filler.
+		// It's a known issue.
+	} );
+} );

+ 22 - 0
packages/ckeditor5-engine/tests/view/domconverter/domconverter.js

@@ -351,6 +351,28 @@ describe( 'DomConverter', () => {
 
 				expect( converter.isBlockFiller( context.firstChild ) ).to.be.false;
 			} );
+
+			// SPECIAL CASE (see ckeditor5#5564).
+			it( 'should return true for a <br> element which is the only child of its block parent', () => {
+				const context = document.createElement( 'div' );
+				context.innerHTML = '<br>';
+
+				expect( converter.isBlockFiller( context.firstChild ) ).to.be.true;
+			} );
+
+			it( 'should return false for a <br> element which is the only child of its non-block parent', () => {
+				const context = document.createElement( 'span' );
+				context.innerHTML = '<br>';
+
+				expect( converter.isBlockFiller( context.firstChild ) ).to.be.false;
+			} );
+
+			it( 'should return false for a <br> element which is followed by an nbsp', () => {
+				const context = document.createElement( 'span' );
+				context.innerHTML = '&nbsp;';
+
+				expect( converter.isBlockFiller( context.firstChild ) ).to.be.false;
+			} );
 		} );
 
 		describe( 'mode "br"', () => {