Merge pull request #1430 from ckeditor/t/ckeditor5/1024

Fix: Fixed view <-> DOM conversion of whitespaces around `<br>` elements. Closes ckeditor/ckeditor5#1024.

Author: Reinmar

src/view/domconverter.js

@@ -23,6 +23,7 @@ import indexOf from '@ckeditor/ckeditor5-utils/src/dom/indexof';
 import getAncestors from '@ckeditor/ckeditor5-utils/src/dom/getancestors';
 import getCommonAncestor from '@ckeditor/ckeditor5-utils/src/dom/getcommonancestor';
 import isText from '@ckeditor/ckeditor5-utils/src/dom/istext';
+import isElement from '@ckeditor/ckeditor5-utils/src/lib/lodash/isElement';
 
 /**
  * DomConverter is a set of tools to do transformations between DOM nodes and view nodes. It also handles
@@ -948,10 +949,11 @@ export default class DomConverter {
 	 * Takes text data from native `Text` node and processes it to a correct {@link module:engine/view/text~Text view text node} data.
 	 *
 	 * Following changes are done:
+	 *
 	 * * multiple whitespaces are replaced to a single space,
-	 * * space at the beginning of the text node is removed, if it is a first text node in it's container
-	 * element or if previous text node ends by space character,
-	 * * space at the end of the text node is removed, if it is a last text node in it's container.
+	 * * space at the beginning of a text node is removed if it is the first text node in its container
+	 * element or if the previous text node ends with a space character,
+	 * * space at the end of the text node is removed, if it is the last text node in its container.
 	 *
 	 * @param {Node} node DOM text node to process.
 	 * @returns {String} Processed data.
@@ -970,17 +972,20 @@ export default class DomConverter {
 		// We're replacing 1+ (and not 2+) to also normalize singular \n\t\r characters (#822).
 		data = data.replace( /[ \n\t\r]{1,}/g, ' ' );
 
-		const prevNode = this._getTouchingDomTextNode( node, false );
-		const nextNode = this._getTouchingDomTextNode( node, true );
+		const prevNode = this._getTouchingInlineDomNode( node, false );
+		const nextNode = this._getTouchingInlineDomNode( node, true );
+
+		const shouldLeftTrim = this._checkShouldLeftTrimDomText( prevNode );
+		const shouldRightTrim = this._checkShouldRightTrimDomText( node, nextNode );
 
-		// If previous dom text node does not exist or it ends by whitespace character, remove space character from the beginning
+		// If the previous dom text node does not exist or it ends by whitespace character, remove space character from the beginning
 		// of this text node. Such space character is treated as a whitespace.
-		if ( !prevNode || /[^\S\u00A0]/.test( prevNode.data.charAt( prevNode.data.length - 1 ) ) ) {
+		if ( shouldLeftTrim ) {
 			data = data.replace( /^ /, '' );
 		}
 
-		// If next text node does not exist remove space character from the end of this text node.
-		if ( !nextNode && !startsWithFiller( node ) ) {
+		// If the next text node does not exist remove space character from the end of this text node.
+		if ( shouldRightTrim ) {
 			data = data.replace( / $/, '' );
 		}
 
@@ -1001,15 +1006,15 @@ export default class DomConverter {
 		// Then, change   character that is at the beginning of the text node to space character.
 		// As above, that   was created for rendering reasons but it's real meaning is just a space character.
 		// We do that replacement only if this is the first node or the previous node ends on whitespace character.
-		if ( !prevNode || /[^\S\u00A0]/.test( prevNode.data.charAt( prevNode.data.length - 1 ) ) ) {
+		if ( shouldLeftTrim ) {
 			data = data.replace( /^\u00A0/, ' ' );
 		}
 
 		// Since input text data could be: `x_ _`, we would not replace the first   after `x` character.
 		// We have to fix it. Since we already change all `  `, we will have something like this at the end of text data:
 		// `x_ _ _` -> `x_    `. Find   at the end of string (can be followed only by spaces).
-		// We do that replacement only if this is the last node or the next node starts by  .
-		if ( !nextNode || nextNode.data.charAt( 0 ) == '\u00A0' ) {
+		// We do that replacement only if this is the last node or the next node starts with &nbsp; or is a <br>.
+		if ( isText( nextNode ) ? nextNode.data.charAt( 0 ) == '\u00A0' : true ) {
 			data = data.replace( /\u00A0( *)$/, ' $1' );
 		}
 
@@ -1018,6 +1023,39 @@ export default class DomConverter {
 		return data;
 	}
 
+	/**
+	 * Helper function which checks if a DOM text node, preceded by the given `prevNode` should
+	 * be trimmed from the left side.
+	 *
+	 * @param {Node} prevNode
+	 */
+	_checkShouldLeftTrimDomText( prevNode ) {
+		if ( !prevNode ) {
+			return true;
+		}
+
+		if ( isElement( prevNode ) ) {
+			return true;
+		}
+
+		return /[^\S\u00A0]/.test( prevNode.data.charAt( prevNode.data.length - 1 ) );
+	}
+
+	/**
+	 * Helper function which checks if a DOM text node, succeeded by the given `nextNode` should
+	 * be trimmed from the right side.
+	 *
+	 * @param {Node} node
+	 * @param {Node} prevNode
+	 */
+	_checkShouldRightTrimDomText( node, nextNode ) {
+		if ( nextNode ) {
+			return false;
+		}
+
+		return !startsWithFiller( node );
+	}
+
 	/**
 	 * Helper function. For given {@link module:engine/view/text~Text view text node}, it finds previous or next sibling
 	 * that is contained in the same container element. If there is no such sibling, `null` is returned.
@@ -1033,12 +1071,17 @@ export default class DomConverter {
 		} );
 
 		for ( const value of treeWalker ) {
+			// ViewContainerElement is found on a way to next ViewText node, so given `node` was first/last
+			// text node in its container element.
 			if ( value.item.is( 'containerElement' ) ) {
-				// ViewContainerElement is found on a way to next ViewText node, so given `node` was first/last
-				// text node in its container element.
 				return null;
-			} else if ( value.item.is( 'textProxy' ) ) {
-				// Found a text node in the same container element.
+			}
+			// <br> found – it works like a block boundary, so do not scan further.
+			else if ( value.item.is( 'br' ) ) {
+				return null;
+			}
+			// Found a text node in the same container element.
+			else if ( value.item.is( 'textProxy' ) ) {
 				return value.item;
 			}
 		}
@@ -1047,15 +1090,27 @@ export default class DomConverter {
 	}
 
 	/**
-	 * Helper function. For given `Text` node, it finds previous or next sibling that is contained in the same block element.
-	 * If there is no such sibling, `null` is returned.
+	 * Helper function. For the given text node, it finds the closest touching node which is either
+	 * a text node or a `<br>`. The search is terminated at block element boundaries and if a matching node
+	 * wasn't found so far, `null` is returned.
+	 *
+	 * In the following DOM structure:
+	 *
+	 *		<p>foo<b>bar</b><br>bom</p>
+	 *
+	 * * `foo` doesn't have its previous touching inline node (`null` is returned),
+	 * * `foo`'s next touching inline node is `bar`
+	 * * `bar`'s next touching inline node is `<br>`
+	 *
+	 * This method returns text nodes and `<br>` elements because these types of nodes affect how
+	 * spaces in the given text node need to be converted.
 	 *
 	 * @private
 	 * @param {Text} node
 	 * @param {Boolean} getNext
-	 * @returns {Text|null}
+	 * @returns {Text|Element|null}
 	 */
-	_getTouchingDomTextNode( node, getNext ) {
+	_getTouchingInlineDomNode( node, getNext ) {
 		if ( !node.parentNode ) {
 			return null;
 		}
@@ -1064,7 +1119,19 @@ export default class DomConverter {
 		const document = node.ownerDocument;
 		const topmostParent = getAncestors( node )[ 0 ];
 
-		const treeWalker = document.createTreeWalker( topmostParent, NodeFilter.SHOW_TEXT );
+		const treeWalker = document.createTreeWalker( topmostParent, NodeFilter.SHOW_TEXT | NodeFilter.SHOW_ELEMENT, {
+			acceptNode( node ) {
+				if ( isText( node ) ) {
+					return NodeFilter.FILTER_ACCEPT;
+				}
+
+				if ( node.tagName == 'BR' ) {
+					return NodeFilter.FILTER_ACCEPT;
+				}
+
+				return NodeFilter.FILTER_SKIP;
+			}
+		} );
 
 		treeWalker.currentNode = node;
 

tests/view/domconverter/dom-to-view.js

@@ -316,6 +316,77 @@ describe( 'DomConverter', () => {
 				expect( viewDiv.getChild( 0 ).getChild( 0 ).data ).to.equal( 'foo' );
 			} );
 
+			it( 'after a <br>', () => {
+				const domP = createElement( document, 'p', {}, [
+					document.createTextNode( 'foo' ),
+					createElement( document, 'br' ),
+					document.createTextNode( ' bar' )
+				] );
+
+				const viewP = converter.domToView( domP );
+
+				expect( viewP.childCount ).to.equal( 3 );
+				expect( viewP.getChild( 2 ).data ).to.equal( 'bar' );
+			} );
+
+			it( 'after a <br> – two spaces', () => {
+				const domP = createElement( document, 'p', {}, [
+					document.createTextNode( 'foo' ),
+					createElement( document, 'br' ),
+					document.createTextNode( ' \u00a0bar' )
+				] );
+
+				const viewP = converter.domToView( domP );
+
+				expect( viewP.childCount ).to.equal( 3 );
+				expect( viewP.getChild( 2 ).data ).to.equal( ' bar' );
+			} );
+
+			// This TC ensures that the algorithm stops on <br>.
+			// If not, situations like https://github.com/ckeditor/ckeditor5/issues/1024#issuecomment-393109558 might occur.
+			it( 'after a <br> – when <br> is preceeded with a nbsp', () => {
+				const domP = createElement( document, 'p', {}, [
+					document.createTextNode( 'foo\u00a0' ),
+					createElement( document, 'br' ),
+					document.createTextNode( ' bar' )
+				] );
+
+				const viewP = converter.domToView( domP );
+
+				expect( viewP.childCount ).to.equal( 3 );
+				expect( viewP.getChild( 2 ).data ).to.equal( 'bar' );
+			} );
+
+			it( 'after a <br> – when text after that <br> is nested', () => {
+				const domP = createElement( document, 'p', {}, [
+					document.createTextNode( 'foo' ),
+					createElement( document, 'br' ),
+					createElement( document, 'b', {}, [
+						document.createTextNode( ' bar' )
+					] )
+				] );
+
+				const viewP = converter.domToView( domP );
+
+				expect( viewP.childCount ).to.equal( 3 );
+				expect( viewP.getChild( 2 ).getChild( 0 ).data ).to.equal( 'bar' );
+			} );
+
+			it( 'between <br>s - trim only the left boundary', () => {
+				const domP = createElement( document, 'p', {}, [
+					document.createTextNode( 'x' ),
+					createElement( document, 'br' ),
+					document.createTextNode( ' foo ' ),
+					createElement( document, 'br' ),
+					document.createTextNode( 'x' )
+				] );
+
+				const viewP = converter.domToView( domP );
+
+				expect( viewP.childCount ).to.equal( 5 );
+				expect( viewP.getChild( 2 ).data ).to.equal( 'foo ' );
+			} );
+
 			it( 'multiple consecutive whitespaces changed to one', () => {
 				const domDiv = createElement( document, 'div', {}, [
 					createElement( document, 'p', {}, [
@@ -521,6 +592,57 @@ describe( 'DomConverter', () => {
 				expect( viewDiv.getChild( 0 ).getChild( 1 ).getChild( 0 ).data ).to.equal( '\u00a0' );
 			} );
 
+			// While we render `X&nbsp;<br>X`, `X <br>X` is ok too – the space needs to be preserved.
+			it( 'not before a <br>', () => {
+				const domP = createElement( document, 'p', {}, [
+					document.createTextNode( 'foo ' ),
+					createElement( document, 'br' )
+				] );
+
+				const viewP = converter.domToView( domP );
+
+				expect( viewP.childCount ).to.equal( 2 );
+				expect( viewP.getChild( 0 ).data ).to.equal( 'foo ' );
+			} );
+
+			it( 'not before a <br> (nbsp+space)', () => {
+				const domP = createElement( document, 'p', {}, [
+					document.createTextNode( 'foo\u00a0 ' ),
+					createElement( document, 'br' )
+				] );
+
+				const viewP = converter.domToView( domP );
+
+				expect( viewP.childCount ).to.equal( 2 );
+				expect( viewP.getChild( 0 ).data ).to.equal( 'foo  ' );
+			} );
+
+			it( 'before a <br> (space+space=>space)', () => {
+				const domP = createElement( document, 'p', {}, [
+					document.createTextNode( 'foo  ' ),
+					createElement( document, 'br' )
+				] );
+
+				const viewP = converter.domToView( domP );
+
+				expect( viewP.childCount ).to.equal( 2 );
+				expect( viewP.getChild( 0 ).data ).to.equal( 'foo ' );
+			} );
+
+			it( 'not before a <br> – when text before that <br> is nested', () => {
+				const domP = createElement( document, 'p', {}, [
+					createElement( document, 'b', {}, [
+						document.createTextNode( 'foo ' )
+					] ),
+					createElement( document, 'br' )
+				] );
+
+				const viewP = converter.domToView( domP );
+
+				expect( viewP.childCount ).to.equal( 2 );
+				expect( viewP.getChild( 0 ).getChild( 0 ).data ).to.equal( 'foo ' );
+			} );
+
 			//
 			// See also whitespace-handling-integration.js.
 			//