Explorar o código

Fixed spaces around <br>s convertion to the view.

Piotrek Koszuliński %!s(int64=7) %!d(string=hai) anos
pai
achega
aa7cce954a

+ 75 - 14
packages/ckeditor5-engine/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
@@ -971,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 );
 
-		// If previous dom text node does not exist or it ends by whitespace character, remove space character from the beginning
+		const shouldLeftTrim = this._checkShouldLeftTrimDomText( prevNode );
+		const shouldRightTrim = this._checkShouldRightTrimDomText( node, nextNode );
+
+		// 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( / $/, '' );
 		}
 
@@ -1002,15 +1006,15 @@ export default class DomConverter {
 		// Then, change &nbsp; character that is at the beginning of the text node to space character.
 		// As above, that &nbsp; 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 &nbsp; after `x` character.
 		// We have to fix it. Since we already change all ` &nbsp;`, we will have something like this at the end of text data:
 		// `x_ _ _` -> `x_    `. Find &nbsp; 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 &nbsp;.
-		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' );
 		}
 
@@ -1019,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.
@@ -1053,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;
 		}
@@ -1070,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;
 

+ 122 - 0
packages/ckeditor5-engine/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.
 			//

+ 139 - 1
packages/ckeditor5-engine/tests/view/domconverter/whitespace-handling-integration.js

@@ -5,14 +5,19 @@
 
 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 } from '../../../src/dev-utils/model';
 
+// NOTE:
+// dev utils' setData() loses white spaces so don't use it for tests here!!!
+// https://github.com/ckeditor/ckeditor5-engine/issues/1428
+
 describe( 'DomConverter – whitespace handling – integration', () => {
 	let editor;
 
 	// See https://github.com/ckeditor/ckeditor5-engine/issues/822.
-	describe( 'data loading', () => {
+	describe( 'normalizing whitespaces around block boundaries (#822)', () => {
 		beforeEach( () => {
 			return VirtualTestEditor
 				.create( { plugins: [ Paragraph ] } )
@@ -145,4 +150,137 @@ describe( 'DomConverter – whitespace handling – integration', () => {
 			expect( editor.getData() ).to.equal( '<p>foo</p><p>bar</p>' );
 		} );
 	} );
+
+	// https://github.com/ckeditor/ckeditor5/issues/1024
+	describe( 'whitespaces around <br>s', () => {
+		beforeEach( () => {
+			return VirtualTestEditor
+				.create( { plugins: [ Paragraph, ShiftEnter ] } )
+				.then( newEditor => {
+					editor = newEditor;
+				} );
+		} );
+
+		afterEach( () => {
+			return editor.destroy();
+		} );
+
+		it( 'single spaces around <br>', () => {
+			editor.setData( '<p>foo&nbsp;<br>&nbsp;bar</p>' );
+
+			expect( getData( editor.model, { withoutSelection: true } ) )
+				.to.equal( '<paragraph>foo <softBreak></softBreak> bar</paragraph>' );
+
+			expect( editor.getData() ).to.equal( '<p>foo&nbsp;<br>&nbsp;bar</p>' );
+		} );
+
+		it( 'single spaces around <br> (normalization)', () => {
+			editor.setData( '<p>foo&nbsp;<br> bar</p>' );
+
+			expect( getData( editor.model, { withoutSelection: true } ) )
+				.to.equal( '<paragraph>foo <softBreak></softBreak>bar</paragraph>' );
+
+			expect( editor.getData() ).to.equal( '<p>foo&nbsp;<br>bar</p>' );
+		} );
+
+		it( 'two spaces before a <br>', () => {
+			editor.setData( '<p>foo &nbsp;<br>bar</p>' );
+
+			expect( getData( editor.model, { withoutSelection: true } ) )
+				.to.equal( '<paragraph>foo  <softBreak></softBreak>bar</paragraph>' );
+
+			expect( editor.getData() ).to.equal( '<p>foo &nbsp;<br>bar</p>' );
+		} );
+
+		it( 'two spaces before a <br> (normalization)', () => {
+			editor.setData( '<p>foo&nbsp; <br>bar</p>' );
+
+			expect( getData( editor.model, { withoutSelection: true } ) )
+				.to.equal( '<paragraph>foo  <softBreak></softBreak>bar</paragraph>' );
+
+			expect( editor.getData() ).to.equal( '<p>foo &nbsp;<br>bar</p>' );
+		} );
+
+		it( 'two spaces before a <br> (normalization to a model nbsp)', () => {
+			editor.setData( '<p>foo&nbsp;&nbsp;<br>bar</p>' );
+
+			expect( getData( editor.model, { withoutSelection: true } ) )
+				.to.equal( '<paragraph>foo\u00a0 <softBreak></softBreak>bar</paragraph>' );
+
+			expect( editor.getData() ).to.equal( '<p>foo&nbsp;&nbsp;<br>bar</p>' );
+		} );
+
+		it( 'single space after a <br>', () => {
+			editor.setData( '<p>foo<br>&nbsp;bar</p>' );
+
+			expect( getData( editor.model, { withoutSelection: true } ) )
+				.to.equal( '<paragraph>foo<softBreak></softBreak> bar</paragraph>' );
+
+			expect( editor.getData() ).to.equal( '<p>foo<br>&nbsp;bar</p>' );
+		} );
+
+		it( 'single space after a <br> (normalization)', () => {
+			editor.setData( '<p>foo<br> bar</p>' );
+
+			expect( getData( editor.model, { withoutSelection: true } ) )
+				.to.equal( '<paragraph>foo<softBreak></softBreak>bar</paragraph>' );
+
+			expect( editor.getData() ).to.equal( '<p>foo<br>bar</p>' );
+		} );
+
+		it( 'two spaces after a <br>', () => {
+			editor.setData( '<p>foo<br>&nbsp; bar</p>' );
+
+			expect( getData( editor.model, { withoutSelection: true } ) )
+				.to.equal( '<paragraph>foo<softBreak></softBreak>  bar</paragraph>' );
+
+			expect( editor.getData() ).to.equal( '<p>foo<br>&nbsp; bar</p>' );
+		} );
+
+		it( 'two spaces after a <br> (normalization)', () => {
+			editor.setData( '<p>foo<br> &nbsp;bar</p>' );
+
+			expect( getData( editor.model, { withoutSelection: true } ) )
+				.to.equal( '<paragraph>foo<softBreak></softBreak> bar</paragraph>' );
+
+			expect( editor.getData() ).to.equal( '<p>foo<br>&nbsp;bar</p>' );
+		} );
+
+		it( 'two spaces after a <br> (normalization to a model nbsp)', () => {
+			editor.setData( '<p>foo<br>&nbsp;&nbsp;bar</p>' );
+
+			expect( getData( editor.model, { withoutSelection: true } ) )
+				.to.equal( '<paragraph>foo<softBreak></softBreak> \u00a0bar</paragraph>' );
+
+			expect( editor.getData() ).to.equal( '<p>foo<br>&nbsp;&nbsp;bar</p>' );
+		} );
+
+		// https://github.com/ckeditor/ckeditor5-engine/issues/1429
+		// it( 'space between <br>s', () => {
+		// 	editor.setData( '<p>foo<br>&nbsp;<br>bar</p>' );
+
+		// 	expect( getData( editor.model, { withoutSelection: true } ) )
+		// 		.to.equal( '<paragraph>foo<softBreak></softBreak> <softBreak></softBreak>bar</paragraph>' );
+
+		// 	expect( editor.getData() ).to.equal( '<p>foo<br>&nbsp;<br>bar</p>' );
+		// } );
+
+		it( 'space between <br>s (normalization)', () => {
+			editor.setData( '<p>foo<br> <br>bar</p>' );
+
+			expect( getData( editor.model, { withoutSelection: true } ) )
+				.to.equal( '<paragraph>foo<softBreak></softBreak><softBreak></softBreak>bar</paragraph>' );
+
+			expect( editor.getData() ).to.equal( '<p>foo<br><br>bar</p>' );
+		} );
+
+		it( 'two spaces between <br>s', () => {
+			editor.setData( '<p>foo<br>&nbsp;&nbsp;<br>bar</p>' );
+
+			expect( getData( editor.model, { withoutSelection: true } ) )
+				.to.equal( '<paragraph>foo<softBreak></softBreak>  <softBreak></softBreak>bar</paragraph>' );
+
+			expect( editor.getData() ).to.equal( '<p>foo<br>&nbsp;&nbsp;<br>bar</p>' );
+		} );
+	} );
 } );