Added handling for loading <p><br></p>

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 );
 	}
 
 	/**

tests/tickets/5564.js

@@ -0,0 +1,49 @@
+/**
+ * @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>' );
+
+		const expectedData = '<p>x</p><p><br>&nbsp;</p><p>x</p>';
+		const actualData = editor.getData();
+
+		expect( actualData ).to.equal( expectedData );
+
+		// 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.
+		editor.setData( actualData );
+
+		expect( editor.getData() ).to.equal( expectedData );
+	} );
+} );

tests/view/domconverter/domconverter.js

@@ -297,37 +297,82 @@ describe( 'DomConverter', () => {
 				converter = new DomConverter( { blockFillerMode: 'nbsp' } );
 			} );
 
-			it( 'should return true if the node is an instance of the NBSP block filler', () => {
-				converter = new DomConverter( { blockFillerMode: 'nbsp' } );
+			it( 'should return true if the node is an nbsp filler and is a single child of a block level element', () => {
 				const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap
-				// NBSP must be check inside a context.
+
 				const context = document.createElement( 'div' );
 				context.appendChild( nbspFillerInstance );
 
 				expect( converter.isBlockFiller( nbspFillerInstance ) ).to.be.true;
 			} );
 
-			it( 'should return false if the node is an instance of the BR block filler', () => {
-				const brFillerInstance = BR_FILLER( document ); // eslint-disable-line new-cap
+			it( 'should return false if the node is an nbsp filler and is not a single child of a block level element', () => {
+				const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap
 
-				expect( converter.isBlockFiller( brFillerInstance ) ).to.be.false;
+				const context = document.createElement( 'div' );
+				context.appendChild( nbspFillerInstance );
+				context.appendChild( document.createTextNode( 'a' ) );
+
+				expect( converter.isBlockFiller( nbspFillerInstance ) ).to.be.false;
 			} );
 
-			it( 'should return false if the nbsp filler is inside context', () => {
-				converter = new DomConverter( { blockFillerMode: 'nbsp' } );
+			it( 'should return false if there are two nbsp fillers in a block element', () => {
 				const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap
-				// NBSP must be check inside a context.
+
 				const context = document.createElement( 'div' );
 				context.appendChild( nbspFillerInstance );
-				// eslint-disable-next-line new-cap
-				context.appendChild( NBSP_FILLER( document ) );
+				context.appendChild( NBSP_FILLER( document ) ); // eslint-disable-line new-cap
+
+				expect( converter.isBlockFiller( nbspFillerInstance ) ).to.be.false;
+			} );
+
+			it( 'should return false filler is placed in a non-block element', () => {
+				const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap
+
+				const context = document.createElement( 'span' );
+				context.appendChild( nbspFillerInstance );
 
 				expect( converter.isBlockFiller( nbspFillerInstance ) ).to.be.false;
 			} );
 
+			it( 'should return false if the node is an instance of the BR block filler', () => {
+				const brFillerInstance = BR_FILLER( document ); // eslint-disable-line new-cap
+
+				expect( converter.isBlockFiller( brFillerInstance ) ).to.be.false;
+			} );
+
 			it( 'should return false for inline filler', () => {
 				expect( converter.isBlockFiller( document.createTextNode( INLINE_FILLER ) ) ).to.be.false;
 			} );
+
+			it( 'should return false for a normal <br> element', () => {
+				const context = document.createElement( 'div' );
+				context.innerHTML = 'x<br>x';
+
+				expect( converter.isBlockFiller( context.childNodes[ 1 ] ) ).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 = '<br>&nbsp;';
+
+				expect( converter.isBlockFiller( context.firstChild ) ).to.be.false;
+			} );
 		} );
 
 		describe( 'mode "br"', () => {