Bladeren bron

Changed: minor fixes and several new tests for handling spaces in view->dom conversion.

Szymon Cofalik 8 jaren geleden
bovenliggende
commit
31535beed1

+ 4 - 4
packages/ckeditor5-engine/src/view/domconverter.js

@@ -16,7 +16,7 @@ import ViewRange from './range';
 import ViewSelection from './selection';
 import ViewDocumentFragment from './documentfragment';
 import { BR_FILLER, INLINE_FILLER_LENGTH, isBlockFiller, isInlineFiller, startsWithFiller, getDataWithoutFiller } from './filler';
-import { getTouchingTextNode } from './utils';
+import { getTouchingTextNode as getTouchingViewTextNode } from './treewalker-utils';
 
 import global from '@ckeditor/ckeditor5-utils/src/dom/global';
 import indexOf from '@ckeditor/ckeditor5-utils/src/dom/indexof';
@@ -177,7 +177,7 @@ export default class DomConverter {
 	 * @param {Document} domDocument Document which will be used to create DOM nodes.
 	 * @param {Object} [options] Conversion options.
 	 * @param {Boolean} [options.bind=false] Determines whether new elements will be bound.
-	 * @param {Boolean} [options.withChildren=true] If true node's and document fragment's children  will be converted too.
+	 * @param {Boolean} [options.withChildren=true] If `true`, node's and document fragment's children will be converted too.
 	 * @returns {Node|DocumentFragment} Converted node or DocumentFragment.
 	 */
 	viewToDom( viewNode, domDocument, options = {} ) {
@@ -913,7 +913,7 @@ export default class DomConverter {
 		// 1. Replace the first space with a nbsp if the previous node ends with a space or there is no previous node
 		// (container element boundary).
 		if ( data.charAt( 0 ) == ' ' ) {
-			const prevNode = getTouchingTextNode( node, false );
+			const prevNode = getTouchingViewTextNode( node, 'backward' );
 			const prevEndsWithSpace = prevNode && this._nodeEndsWithSpace( prevNode );
 
 			if ( prevEndsWithSpace || !prevNode ) {
@@ -923,7 +923,7 @@ export default class DomConverter {
 
 		// 2. Replace the last space with a nbsp if this is the last text node (container element boundary).
 		if ( data.charAt( data.length - 1 ) == ' ' ) {
-			const nextNode = getTouchingTextNode( node, true );
+			const nextNode = getTouchingViewTextNode( node, 'forward' );
 
 			if ( !nextNode ) {
 				data = data.substr( 0, data.length - 1 ) + '\u00A0';

+ 11 - 2
packages/ckeditor5-engine/src/view/renderer.js

@@ -10,7 +10,7 @@
 import ViewText from './text';
 import ViewPosition from './position';
 import { INLINE_FILLER, INLINE_FILLER_LENGTH, startsWithFiller, isInlineFiller, isBlockFiller } from './filler';
-import { getTouchingTextNode } from './utils';
+import { getTouchingTextNode } from './treewalker-utils';
 
 import mix from '@ckeditor/ckeditor5-utils/src/mix';
 import diff from '@ckeditor/ckeditor5-utils/src/diff';
@@ -426,7 +426,16 @@ export default class Renderer {
 		if ( actualText != expectedText ) {
 			domText.data = expectedText;
 
-			const nextTouchingTextNode = getTouchingTextNode( viewText, true );
+			// If we changed text node's data, we might need to re-render one or more following text nodes.
+			// This is because of space handling. If the text node starts with a space, that space might be
+			// changed to ` `. Whether it will be changed, depends on the previous text node's data.
+			// So when we change a text node, we need to refresh all following text nodes.
+			//
+			// Example:
+			// 1. Initial state:             <p>foo<em> bar</em></p>
+			// 2. Space is typed after foo:  <p>foo <em> bar</em></p>
+			// 3. After refresh:             <p>foo <em>&nbsp;bar</em><p>
+			const nextTouchingTextNode = getTouchingTextNode( viewText );
 
 			if ( nextTouchingTextNode ) {
 				this.markedTexts.add( nextTouchingTextNode );

+ 6 - 5
packages/ckeditor5-engine/src/view/utils.js

@@ -17,19 +17,20 @@ import ViewTreeWalker from './treewalker';
  * in the same container element. If there is no such sibling, `null` is returned.
  *
  * @param {module:engine/view/text~Text} node Reference node.
- * @param {Boolean} getNext If `true` next touching sibling will be returned. If `false` previous touching sibling will be returned.
+ * @param {'forward'|'backward'} [direction='forward'] If set to `'forward'`, next touching sibling will be returned.
+ * If set to `'backward'` previous touching sibling will be returned.
  * @returns {module:engine/view/text~Text|null} Touching text node or `null` if there is no next or previous touching text node.
  */
-export function getTouchingTextNode( node, getNext ) {
+export function getTouchingTextNode( node, direction = 'forward' ) {
 	const treeWalker = new ViewTreeWalker( {
-		startPosition: getNext ? ViewPosition.createAfter( node ) : ViewPosition.createBefore( node ),
-		direction: getNext ? 'forward' : 'backward'
+		startPosition: direction == 'forward' ? ViewPosition.createAfter( node ) : ViewPosition.createBefore( node ),
+		direction
 	} );
 
 	for ( const value of treeWalker ) {
 		if ( value.item.is( 'containerElement' ) ) {
 			// ViewContainerElement is found on a way to next ViewText node, so given `node` was first/last
-			// text node in it's container element.
+			// text node in its container element.
 			return null;
 		} else if ( value.item.is( 'text' ) ) {
 			// Found a text node in the same container element.

+ 13 - 0
packages/ckeditor5-engine/tests/view/domconverter/view-to-dom.js

@@ -367,6 +367,19 @@ describe( 'DomConverter', () => {
 			test( [ 'x   ',	'  '	], 'x _ __' );
 			test( [ 'x   ',	'   '	], 'x _ _ _' );
 
+			test( [ ' ',	'x'		], '_x' );
+			test( [ '  ',	'x'		], '_ x' );
+			test( [ '   ',	'x'		], '_ _x' );
+			test( [ ' ',	' x'	], '_ x' );
+			test( [ '  ',	' x'	], '_ _x' );
+			test( [ '   ',	' x'	], '_ _ x' );
+			test( [ ' ',	'  x'	], '_ _x' );
+			test( [ '  ',	'  x'	], '_ _ x' );
+			test( [ '   ',	'  x'	], '_ _ _x' );
+			test( [ ' ',	'   x'	], '_ _ x' );
+			test( [ '  ',	'   x'	], '_ _ _x' );
+			test( [ '   ',	'   x'	], '_ _ _ x' );
+
 			test( [ 'x',	' ',		'x'		],	'x x' );
 			test( [ 'x',	' ',		' x'	],	'x _x' );
 			test( [ 'x',	'  ',		' x'	],	'x _ x' );

+ 25 - 11
packages/ckeditor5-engine/tests/view/utils.js

@@ -7,16 +7,17 @@ import ViewAttributeElement from '../../src/view/attributeelement';
 import ViewContainerElement from '../../src/view/containerelement';
 import ViewText from '../../src/view/text';
 
-import { getTouchingTextNode } from '../../src/view/utils';
+import { getTouchingTextNode } from '../../src/view/treewalker-utils';
 
 describe( 'getTouchingTextNode()', () => {
-	let a, b, x, y;
+	let a, b, x, y, z;
 
 	beforeEach( () => {
 		a = new ViewText( 'a' );
 		b = new ViewText( 'b' );
 		x = new ViewText( 'x' );
 		y = new ViewText( 'y' );
+		z = new ViewText( 'z' );
 
 		// <div><p>ab</p><p><em><strong>x</strong></em>y</p></div>
 		/* eslint-disable no-new */
@@ -26,33 +27,46 @@ describe( 'getTouchingTextNode()', () => {
 			new ViewContainerElement( 'p', null, [
 				new ViewAttributeElement( 'em', null, new ViewAttributeElement( 'strong', null, x ) ),
 				y
-			] )
+			] ),
+
+			z,
+
+			new ViewContainerElement( 'p', null, [ new ViewText( '_' ) ] )
 		] );
 	} );
 
+	it( 'should return next touching view text node when direction is not specified', () => {
+		expect( getTouchingTextNode( a ) ).to.equal( b );
+	} );
+
 	it( 'should return next touching view text node', () => {
-		expect( getTouchingTextNode( a, true ) ).to.equal( b );
+		expect( getTouchingTextNode( a, 'forward' ) ).to.equal( b );
 	} );
 
 	it( 'should return previous touching view text node', () => {
-		expect( getTouchingTextNode( b, false ) ).to.equal( a );
+		expect( getTouchingTextNode( b, 'backward' ) ).to.equal( a );
 	} );
 
 	it( 'should go outside of attribute element looking for text nodes', () => {
-		expect( getTouchingTextNode( x, true ) ).to.equal( y );
+		expect( getTouchingTextNode( x, 'forward' ) ).to.equal( y );
 	} );
 
 	it( 'should go inside attribute element looking for text nodes', () => {
-		expect( getTouchingTextNode( y, false ) ).to.equal( x );
+		expect( getTouchingTextNode( y, 'backward' ) ).to.equal( x );
 	} );
 
 	it( 'should return null if there is no next text node in given container element', () => {
-		expect( getTouchingTextNode( b, true ) ).to.be.null;
-		expect( getTouchingTextNode( y, true ) ).to.be.null;
+		expect( getTouchingTextNode( b, 'forward' ) ).to.be.null;
+		expect( getTouchingTextNode( y, 'forward' ) ).to.be.null;
 	} );
 
 	it( 'should return null if there is no previous text node in given container element', () => {
-		expect( getTouchingTextNode( a, false ) ).to.be.null;
-		expect( getTouchingTextNode( x, false ) ).to.be.null;
+		expect( getTouchingTextNode( a, 'backward' ) ).to.be.null;
+		expect( getTouchingTextNode( x, 'backward' ) ).to.be.null;
+	} );
+
+	it( 'should not enter container element when looking for touching text node', () => {
+		expect( getTouchingTextNode( z ) ).to.be.null;
+		expect( getTouchingTextNode( z, 'backward' ) ).to.be.null;
 	} );
 } );