From 46084a27ebe25999af464e27f09e5f73fea50bfe Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 18 Aug 2017 12:01:43 +0200 Subject: [PATCH 1/6] Changed: Introduced `view.utils` and `view.utils.getTouchingTextNode`. --- src/view/utils.js | 41 ++++++++++++++++++++++++++++++++ tests/view/utils.js | 58 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) create mode 100644 src/view/utils.js create mode 100644 tests/view/utils.js diff --git a/src/view/utils.js b/src/view/utils.js new file mode 100644 index 000000000..0d3df7688 --- /dev/null +++ b/src/view/utils.js @@ -0,0 +1,41 @@ +/** + * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. + */ + +import ViewPosition from './position'; +import ViewTreeWalker from './treewalker'; + +/** + * Contains utility functions for working on view. + * + * @module engine/view/utils + */ + +/** + * 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. + * + * @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. + * @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 ) { + const treeWalker = new ViewTreeWalker( { + startPosition: getNext ? ViewPosition.createAfter( node ) : ViewPosition.createBefore( node ), + direction: getNext ? 'forward' : 'backward' + } ); + + 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. + return null; + } else if ( value.item.is( 'text' ) ) { + // Found a text node in the same container element. + return value.item; + } + } + + return null; +} diff --git a/tests/view/utils.js b/tests/view/utils.js new file mode 100644 index 000000000..b63ee562e --- /dev/null +++ b/tests/view/utils.js @@ -0,0 +1,58 @@ +/** + * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. + */ + +import ViewAttributeElement from '../../src/view/attributeelement'; +import ViewContainerElement from '../../src/view/containerelement'; +import ViewText from '../../src/view/text'; + +import { getTouchingTextNode } from '../../src/view/utils'; + +describe( 'getTouchingTextNode()', () => { + let a, b, x, y; + + beforeEach( () => { + a = new ViewText( 'a' ); + b = new ViewText( 'b' ); + x = new ViewText( 'x' ); + y = new ViewText( 'y' ); + + //

ab

xy

+ /* eslint-disable no-new */ + new ViewContainerElement( 'div', null, [ + new ViewContainerElement( 'p', null, [ a, b ] ), + + new ViewContainerElement( 'p', null, [ + new ViewAttributeElement( 'em', null, new ViewAttributeElement( 'strong', null, x ) ), + y + ] ) + ] ); + } ); + + it( 'should return next touching view text node', () => { + expect( getTouchingTextNode( a, true ) ).to.equal( b ); + } ); + + it( 'should return previous touching view text node', () => { + expect( getTouchingTextNode( b, false ) ).to.equal( a ); + } ); + + it( 'should go outside of attribute element looking for text nodes', () => { + expect( getTouchingTextNode( x, true ) ).to.equal( y ); + } ); + + it( 'should go inside attribute element looking for text nodes', () => { + expect( getTouchingTextNode( y, false ) ).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; + } ); + + 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; + } ); +} ); From 91dbb6ab4b21c10f9d91c72c96cff84d526cf893 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 18 Aug 2017 12:42:19 +0200 Subject: [PATCH 2/6] Fixed: Fixed handling spaces in view text nodes in view-to-dom conversion. --- src/view/domconverter.js | 114 ++++++------------------- tests/view/domconverter/view-to-dom.js | 79 ++++++++++++++--- 2 files changed, 93 insertions(+), 100 deletions(-) diff --git a/src/view/domconverter.js b/src/view/domconverter.js index 2ae076e05..b75d2b1d1 100644 --- a/src/view/domconverter.js +++ b/src/view/domconverter.js @@ -15,8 +15,8 @@ import ViewPosition from './position'; import ViewRange from './range'; import ViewSelection from './selection'; import ViewDocumentFragment from './documentfragment'; -import ViewTreeWalker from './treewalker'; import { BR_FILLER, INLINE_FILLER_LENGTH, isBlockFiller, isInlineFiller, startsWithFiller, getDataWithoutFiller } from './filler'; +import { getTouchingTextNode } from './utils'; import global from '@ckeditor/ckeditor5-utils/src/dom/global'; import indexOf from '@ckeditor/ckeditor5-utils/src/dom/indexof'; @@ -65,7 +65,7 @@ export default class DomConverter { * * @member {Array.} module:engine/view/domconverter~DomConverter#preElements */ - this.preElements = [ 'pre' ]; + this.preElements = [ 'pre', 'code' ]; /** * Tag names of DOM `Element`s which are considered block elements. @@ -890,8 +890,8 @@ export default class DomConverter { * Following changes are done: * * * multiple spaces are replaced to a chain of spaces and ` `, - * * space at the beginning of the text node is changed to ` ` if it is a first text node in it's container - * element or if previous text node ends by space character, + * * space at the beginning and at the end of the text node is changed to ` ` if it is a first text node in it's container + * element or if previous text node ends with space character, * * space at the end of the text node is changed to ` ` if it is a last text node in it's container. * * @private @@ -899,7 +899,7 @@ export default class DomConverter { * @returns {String} Processed text data. */ _processDataFromViewText( node ) { - const data = node.data; + let data = node.data; // If any of node ancestors has a name which is in `preElements` array, then currently processed // view text node is (will be) in preformatted element. We should not change whitespaces then. @@ -907,105 +907,43 @@ export default class DomConverter { return data; } - const prevNode = this._getTouchingViewTextNode( node, false ); - const nextNode = this._getTouchingViewTextNode( node, true ); - - // Second part of text data, from the space after the last non-space character to the end. - // We separate `textEnd` and `textStart` because `textEnd` needs some special handling. - let textEnd = data.match( / *$/ )[ 0 ]; - // First part of data, between first and last part of data. - let textStart = data.substr( 0, data.length - textEnd.length ); - - // If previous text node does not exist or it ends by space character, replace space character at the beginning of text. - // ` x` -> `_x` - // ` x` -> `_ x` - // ` x` -> `_ x` - if ( !prevNode || prevNode.data.charAt( prevNode.data.length - 1 ) == ' ' ) { - textStart = textStart.replace( /^ /, '\u00A0' ); + // 1. Replace first space with nbsp if previous node ends with space or there is no previous node (container element boundary). + if ( data.charAt( 0 ) == ' ' ) { + const prevNode = getTouchingTextNode( node, false ); + const prevEndsWithSpace = prevNode && this._nodeEndsWithSpace( prevNode ); + + if ( prevEndsWithSpace || !prevNode ) { + data = '\u00A0' + data.substr( 1 ); + } } - // Multiple consecutive spaces. Change them to `  ` pairs. - // `_x x` -> `_x _x` - // `_ x x` -> `_ x _x` - // `_ x x` -> `_ _x _x` - // `_ x x` -> `_ _x _ x` - // `_ x x` -> `_ _x _ _x` - // `_ x x` -> `_ _ x _ _x` - textStart = textStart.replace( / {2}/g, ' \u00A0' ); - - // Process `textEnd` only if there is anything to process. - if ( textEnd.length > 0 ) { - // (1) We need special treatment for the last part of text node, it has to end on ` `, not space: - // `x ` -> `x_` - // `x ` -> `x _` - // `x ` -> `x_ _` - // `x ` -> `x _ _` - // (2) Different case when there is a node after: - // `x b` -> `x b` - // `x b` -> `x _b` - // `x b` -> `x _ b` - // `x b` -> `x _ _b` - // (3) But different, when that node starts by   (or space that will be converted to  ): - // `x _b` -> `x _b` - // `x _b` -> `x_ _b` - // `x _b` -> `x _ _b` - // `x _b` -> `x_ _ _b` - // Let's assume that starting from space is normal behavior, because starting from   is a less frequent case. - let textEndStartsFromNbsp = false; + // 2. Replace last space with nbsp if it is the last text node (container element boundary). + if ( data.charAt( data.length - 1 ) == ' ' ) { + const nextNode = getTouchingTextNode( node, true ); if ( !nextNode ) { - // (1) - if ( textEnd.length % 2 ) { - textEndStartsFromNbsp = true; - } - } else if ( nextNode.data.charAt( 0 ) == ' ' || nextNode.data.charAt( 0 ) == '\u00A0' ) { - // (3) - if ( textEnd.length % 2 === 0 ) { - textEndStartsFromNbsp = true; - } + data = data.substr( 0, data.length - 1 ) + '\u00A0'; } - - if ( textEndStartsFromNbsp ) { - textEnd = '\u00A0' + textEnd.substr( 0, textEnd.length - 1 ); - } - - textEnd = textEnd.replace( / {2}/g, ' \u00A0' ); } - return textStart + textEnd; + return data.replace( / {2}/gi, ' \u00A0' ); } /** - * 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 block element. If there is no such sibling, `null` is returned. + * Checks whether given node ends with a space character after changing appropriate space characters to ` `s. * * @private - * @param {module:engine/view/text~Text} node - * @param {Boolean} getNext - * @returns {module:engine/view/text~Text} + * @param {module:engine/view/text~Text} node Node to check. + * @returns {Boolean} `true` if given `node` ends with space, `false` otherwise. */ - _getTouchingViewTextNode( node, getNext ) { - if ( !node.parent ) { - return null; + _nodeEndsWithSpace( node ) { + if ( node.getAncestors().some( parent => this.preElements.includes( parent.name ) ) ) { + return false; } - const treeWalker = new ViewTreeWalker( { - startPosition: getNext ? ViewPosition.createAfter( node ) : ViewPosition.createBefore( node ), - direction: getNext ? 'forward' : 'backward' - } ); - - 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. - return null; - } else if ( value.item.is( 'text' ) ) { - // Found a text node in the same container element. - return value.item; - } - } + const data = this._processDataFromViewText( node ); - return null; + return data.charAt( data.length - 1 ) == ' '; } /** diff --git a/tests/view/domconverter/view-to-dom.js b/tests/view/domconverter/view-to-dom.js index 3788014ed..f23f06da1 100644 --- a/tests/view/domconverter/view-to-dom.js +++ b/tests/view/domconverter/view-to-dom.js @@ -269,7 +269,7 @@ describe( 'DomConverter', () => { // At the end. test( 'x ', 'x_' ); test( 'x ', 'x _' ); - test( 'x ', 'x_ _' ); + test( 'x ', 'x __' ); test( 'x ', 'x _ _' ); // In the middle. @@ -282,11 +282,19 @@ describe( 'DomConverter', () => { test( ' x ', '_x_' ); test( ' x x ', '_ x _x _' ); test( ' x x ', '_ _x x _' ); - test( ' x x ', '_ _x x_ _' ); + test( ' x x ', '_ _x x __' ); test( ' x x ', '_ _x _ _x_' ); + + // Only spaces. test( ' ', '_' ); + test( ' ', '__' ); + test( ' ', '_ _' ); + test( ' ', '_ __' ); + test( ' ', '_ _ _' ); + test( ' ', '_ _ __' ); // With hard   + // It should be treated like a normal sign. test( '_x', '_x' ); test( ' _x', '__x' ); test( ' _x', '_ _x' ); @@ -323,33 +331,80 @@ describe( 'DomConverter', () => { test( [ 'x', ' y' ], 'x y' ); test( [ 'x ', ' y' ], 'x _y' ); - test( [ 'x ', ' y' ], 'x_ _y' ); + test( [ 'x ', ' y' ], 'x _ y' ); test( [ 'x ', ' y' ], 'x _ _y' ); - test( [ 'x ', ' y' ], 'x_ _ _y' ); + test( [ 'x ', ' y' ], 'x _ _ y' ); test( [ 'x', '_y' ], 'x_y' ); test( [ 'x ', '_y' ], 'x _y' ); - test( [ 'x ', '_y' ], 'x_ _y' ); + test( [ 'x ', '_y' ], 'x __y' ); test( [ 'x ', '_y' ], 'x _ _y' ); - test( [ 'x ', '_y' ], 'x_ _ _y' ); + test( [ 'x ', '_y' ], 'x _ __y' ); test( [ 'x', ' y' ], 'x _y' ); test( [ 'x ', ' y' ], 'x _ y' ); - test( [ 'x ', ' y' ], 'x_ _ y' ); + test( [ 'x ', ' y' ], 'x _ _y' ); test( [ 'x ', ' y' ], 'x _ _ y' ); - test( [ 'x ', ' y' ], 'x_ _ _ y' ); + test( [ 'x ', ' y' ], 'x _ _ _y' ); test( [ 'x', ' y' ], 'x _ y' ); test( [ 'x ', ' y' ], 'x _ _y' ); - test( [ 'x ', ' y' ], 'x_ _ _y' ); + test( [ 'x ', ' y' ], 'x _ _ y' ); test( [ 'x ', ' y' ], 'x _ _ _y' ); - test( [ 'x ', ' y' ], 'x_ _ _ _y' ); + test( [ 'x ', ' y' ], 'x _ _ _ y' ); + + // "Non-empty" + "empty" text nodes. + 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' ); + test( [ 'x', ' ', ' x' ], 'x _ _ x' ); + test( [ 'x ', ' ', ' x' ], 'x _ x' ); + test( [ 'x ', ' ', ' x' ], 'x _ _x' ); + test( [ 'x ', ' ', ' x' ], 'x _ _ _x' ); + test( [ 'x ', ' ', ' x' ], 'x _ _x' ); + test( [ 'x ', ' ', ' x' ], 'x _ _ x' ); + test( [ 'x ', ' ', ' x' ], 'x _ _ _ x' ); + test( [ 'x ', ' ', ' x' ], 'x _ _ x' ); + test( [ 'x ', ' ', ' x' ], 'x _ _ _x' ); + test( [ 'x ', ' ', ' x' ], 'x _ _ _ _x' ); + + // "Empty" + "empty" text nodes. + test( [ ' ', ' ' ], '__' ); + test( [ ' ', ' ' ], '_ _' ); + test( [ ' ', ' ' ], '_ __' ); + test( [ ' ', ' ' ], '_ _' ); + test( [ ' ', ' ' ], '_ __' ); + test( [ ' ', ' ' ], '_ __' ); + test( [ ' ', ' ' ], '_ _ _' ); + test( [ ' ', ' ' ], '_ _ _' ); + test( [ ' ', ' ' ], '_ _ __' ); it( 'not in preformatted blocks', () => { - const viewDiv = new ViewContainerElement( 'pre', null, new ViewText( ' foo ' ) ); + const viewDiv = new ViewContainerElement( 'pre', null, [ new ViewText( ' foo ' ), new ViewText( ' bar ' ) ] ); + const domDiv = converter.viewToDom( viewDiv, document ); + + expect( domDiv.innerHTML ).to.equal( ' foo bar ' ); + } ); + + it( 'text node before in a preformatted node', () => { + const viewCode = new ViewAttributeElement( 'code', null, new ViewText( 'foo ' ) ); + const viewDiv = new ViewContainerElement( 'div', null, [ viewCode, new ViewText( ' bar' ) ] ); const domDiv = converter.viewToDom( viewDiv, document ); - expect( domDiv.innerHTML ).to.equal( ' foo ' ); + expect( domDiv.innerHTML ).to.equal( 'foo bar' ); } ); } ); } ); From 99d5f24ab65caa7163388e4c80242d5ed41332f8 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 18 Aug 2017 12:58:38 +0200 Subject: [PATCH 3/6] Changed: Renderer should re-render also next text node if a text node has changed. --- src/view/renderer.js | 17 ++++++++++++++--- tests/view/renderer.js | 21 ++++++++++++++++----- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/src/view/renderer.js b/src/view/renderer.js index c2fa297bf..ebb8b0f31 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -10,6 +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 mix from '@ckeditor/ckeditor5-utils/src/mix'; import diff from '@ckeditor/ckeditor5-utils/src/diff'; @@ -198,9 +199,7 @@ export default class Renderer { } for ( const node of this.markedTexts ) { - if ( !this.markedChildren.has( node.parent ) && this.domConverter.mapViewToDom( node.parent ) ) { - this._updateText( node, { inlineFillerPosition } ); - } + this._updateText( node, { inlineFillerPosition } ); } for ( const element of this.markedAttributes ) { @@ -407,6 +406,12 @@ export default class Renderer { */ _updateText( viewText, options ) { const domText = this.domConverter.findCorrespondingDomText( viewText ); + + // If this is a new text node and it is not in DOM, it will be created and handled in `_updateChildren`. + if ( !domText ) { + return; + } + const newDomText = this.domConverter.viewToDom( viewText, domText.ownerDocument ); const actualText = domText.data; @@ -420,6 +425,12 @@ export default class Renderer { if ( actualText != expectedText ) { domText.data = expectedText; + + const nextTouchingTextNode = getTouchingTextNode( viewText, true ); + + if ( nextTouchingTextNode ) { + this.markedTexts.add( nextTouchingTextNode ); + } } } diff --git a/tests/view/renderer.js b/tests/view/renderer.js index 7fc1ef786..799044ae9 100644 --- a/tests/view/renderer.js +++ b/tests/view/renderer.js @@ -214,18 +214,29 @@ describe( 'Renderer', () => { expect( renderer.markedTexts.size ).to.equal( 0 ); } ); - it( 'should not update text parent child list changed', () => { - const viewImg = new ViewElement( 'img' ); + it( 'should update next touching text nodes of updated text node', () => { const viewText = new ViewText( 'foo' ); - viewRoot.appendChildren( [ viewImg, viewText ] ); + + viewRoot.appendChildren( [ + viewText, + new ViewAttributeElement( 'strong', null, new ViewText( ' bar' ) ) + ] ); renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.childNodes.length ).to.equal( 2 ); + expect( domRoot.childNodes[ 0 ].data ).to.equal( 'foo' ); + expect( domRoot.childNodes[ 1 ].childNodes[ 0 ].data ).to.equal( ' bar' ); + + viewText.data = 'foo '; + renderer.markToSync( 'text', viewText ); renderer.render(); expect( domRoot.childNodes.length ).to.equal( 2 ); - expect( domRoot.childNodes[ 0 ].tagName ).to.equal( 'IMG' ); - expect( domRoot.childNodes[ 1 ].data ).to.equal( 'foo' ); + expect( domRoot.childNodes[ 0 ].data ).to.equal( 'foo ' ); + expect( domRoot.childNodes[ 1 ].childNodes[ 0 ].data ).to.equal( '\u00A0bar' ); } ); it( 'should not change text if it is the same during text rendering', () => { From 4ca50951b3d24161e9a68964927eccd81fb1ef46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Mon, 21 Aug 2017 12:29:17 +0200 Subject: [PATCH 4/6] Docs and minor regexp correction. --- src/view/domconverter.js | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/view/domconverter.js b/src/view/domconverter.js index b75d2b1d1..e67ca2100 100644 --- a/src/view/domconverter.js +++ b/src/view/domconverter.js @@ -885,14 +885,17 @@ export default class DomConverter { } /** - * Takes text data from given {@link module:engine/view/text~Text#data} and processes it so it is correctly displayed in DOM. + * Takes text data from a given {@link module:engine/view/text~Text#data} and processes it so + * it is correctly displayed in the DOM. * * Following changes are done: * - * * multiple spaces are replaced to a chain of spaces and ` `, - * * space at the beginning and at the end of the text node is changed to ` ` if it is a first text node in it's container - * element or if previous text node ends with space character, - * * space at the end of the text node is changed to ` ` if it is a last text node in it's container. + * * a space at the beginning is changed to ` ` if this is the first text node in its container + * element or if a previous text node ends with a space character, + * * space at the end of the text node is changed to ` ` if this is the last text node in its container, + * * remaining spaces are replaced to a chain of spaces and ` ` (e.g. `'x x'` becomes `'x   x'`). + * + * Content of {@link #preElements} is not processed. * * @private * @param {module:engine/view/text~Text} node View text node to process. @@ -907,7 +910,8 @@ export default class DomConverter { return data; } - // 1. Replace first space with nbsp if previous node ends with space or there is no previous node (container element boundary). + // 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 prevEndsWithSpace = prevNode && this._nodeEndsWithSpace( prevNode ); @@ -917,7 +921,7 @@ export default class DomConverter { } } - // 2. Replace last space with nbsp if it is the last text node (container element boundary). + // 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 ); @@ -926,7 +930,7 @@ export default class DomConverter { } } - return data.replace( / {2}/gi, ' \u00A0' ); + return data.replace( / {2}/g, ' \u00A0' ); } /** From dbb7b6f78b573d7b2d9b8c86e4575169e7004fef Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 22 Aug 2017 08:53:33 +0200 Subject: [PATCH 5/6] Changed: minor fixes and several new tests for handling spaces in view->dom conversion. --- src/view/domconverter.js | 8 ++--- src/view/renderer.js | 13 +++++-- src/view/{utils.js => treewalker-utils.js} | 11 +++--- tests/view/domconverter/view-to-dom.js | 13 +++++++ tests/view/{utils.js => treewalker-utils.js} | 36 ++++++++++++++------ 5 files changed, 59 insertions(+), 22 deletions(-) rename src/view/{utils.js => treewalker-utils.js} (72%) rename tests/view/{utils.js => treewalker-utils.js} (55%) diff --git a/src/view/domconverter.js b/src/view/domconverter.js index e67ca2100..f6fb8a45a 100644 --- a/src/view/domconverter.js +++ b/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'; diff --git a/src/view/renderer.js b/src/view/renderer.js index ebb8b0f31..e53fc91d1 100644 --- a/src/view/renderer.js +++ b/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:

foo bar

+ // 2. Space is typed after foo:

foo bar

+ // 3. After refresh:

foo  bar

+ const nextTouchingTextNode = getTouchingTextNode( viewText ); if ( nextTouchingTextNode ) { this.markedTexts.add( nextTouchingTextNode ); diff --git a/src/view/utils.js b/src/view/treewalker-utils.js similarity index 72% rename from src/view/utils.js rename to src/view/treewalker-utils.js index 0d3df7688..79b89d2a6 100644 --- a/src/view/utils.js +++ b/src/view/treewalker-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. diff --git a/tests/view/domconverter/view-to-dom.js b/tests/view/domconverter/view-to-dom.js index f23f06da1..f709192f8 100644 --- a/tests/view/domconverter/view-to-dom.js +++ b/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' ); diff --git a/tests/view/utils.js b/tests/view/treewalker-utils.js similarity index 55% rename from tests/view/utils.js rename to tests/view/treewalker-utils.js index b63ee562e..8fc3c3281 100644 --- a/tests/view/utils.js +++ b/tests/view/treewalker-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' ); //

ab

xy

/* 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; } ); } ); From 4aaaf7b7e55411d21a9a1a9b2b4da5a84794c850 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 22 Aug 2017 12:55:31 +0200 Subject: [PATCH 6/6] Changed: Reverted refreshing touching text nodes in `view.Renderer`. Moved `getTouchingTextNode` back to `view.DomConverter`. --- src/view/domconverter.js | 34 ++++++++++++++-- src/view/renderer.js | 26 ++---------- src/view/treewalker-utils.js | 42 -------------------- tests/view/renderer.js | 21 +++------- tests/view/treewalker-utils.js | 72 ---------------------------------- 5 files changed, 39 insertions(+), 156 deletions(-) delete mode 100644 src/view/treewalker-utils.js delete mode 100644 tests/view/treewalker-utils.js diff --git a/src/view/domconverter.js b/src/view/domconverter.js index f6fb8a45a..fc22e89c2 100644 --- a/src/view/domconverter.js +++ b/src/view/domconverter.js @@ -15,8 +15,8 @@ import ViewPosition from './position'; import ViewRange from './range'; import ViewSelection from './selection'; import ViewDocumentFragment from './documentfragment'; +import ViewTreeWalker from './treewalker'; import { BR_FILLER, INLINE_FILLER_LENGTH, isBlockFiller, isInlineFiller, startsWithFiller, getDataWithoutFiller } from './filler'; -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'; @@ -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 = getTouchingViewTextNode( node, 'backward' ); + const prevNode = this._getTouchingViewTextNode( node, false ); 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 = getTouchingViewTextNode( node, 'forward' ); + const nextNode = this._getTouchingViewTextNode( node, true ); if ( !nextNode ) { data = data.substr( 0, data.length - 1 ) + '\u00A0'; @@ -1017,6 +1017,34 @@ export default class DomConverter { return data; } + /** + * 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. + * + * @param {module:engine/view/text~Text} node Reference node. + * @param {Boolean} getNext + * @returns {module:engine/view/text~Text|null} Touching text node or `null` if there is no next or previous touching text node. + */ + _getTouchingViewTextNode( node, getNext ) { + const treeWalker = new ViewTreeWalker( { + startPosition: getNext ? ViewPosition.createAfter( node ) : ViewPosition.createBefore( node ), + direction: getNext ? 'forward' : 'backward' + } ); + + 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 its container element. + return null; + } else if ( value.item.is( 'text' ) ) { + // Found a text node in the same container element. + return value.item; + } + } + + return null; + } + /** * 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. diff --git a/src/view/renderer.js b/src/view/renderer.js index e53fc91d1..c2fa297bf 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -10,7 +10,6 @@ import ViewText from './text'; import ViewPosition from './position'; import { INLINE_FILLER, INLINE_FILLER_LENGTH, startsWithFiller, isInlineFiller, isBlockFiller } from './filler'; -import { getTouchingTextNode } from './treewalker-utils'; import mix from '@ckeditor/ckeditor5-utils/src/mix'; import diff from '@ckeditor/ckeditor5-utils/src/diff'; @@ -199,7 +198,9 @@ export default class Renderer { } for ( const node of this.markedTexts ) { - this._updateText( node, { inlineFillerPosition } ); + if ( !this.markedChildren.has( node.parent ) && this.domConverter.mapViewToDom( node.parent ) ) { + this._updateText( node, { inlineFillerPosition } ); + } } for ( const element of this.markedAttributes ) { @@ -406,12 +407,6 @@ export default class Renderer { */ _updateText( viewText, options ) { const domText = this.domConverter.findCorrespondingDomText( viewText ); - - // If this is a new text node and it is not in DOM, it will be created and handled in `_updateChildren`. - if ( !domText ) { - return; - } - const newDomText = this.domConverter.viewToDom( viewText, domText.ownerDocument ); const actualText = domText.data; @@ -425,21 +420,6 @@ export default class Renderer { if ( actualText != expectedText ) { domText.data = expectedText; - - // 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:

foo bar

- // 2. Space is typed after foo:

foo bar

- // 3. After refresh:

foo  bar

- const nextTouchingTextNode = getTouchingTextNode( viewText ); - - if ( nextTouchingTextNode ) { - this.markedTexts.add( nextTouchingTextNode ); - } } } diff --git a/src/view/treewalker-utils.js b/src/view/treewalker-utils.js deleted file mode 100644 index 79b89d2a6..000000000 --- a/src/view/treewalker-utils.js +++ /dev/null @@ -1,42 +0,0 @@ -/** - * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved. - * For licensing, see LICENSE.md. - */ - -import ViewPosition from './position'; -import ViewTreeWalker from './treewalker'; - -/** - * Contains utility functions for working on view. - * - * @module engine/view/utils - */ - -/** - * 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. - * - * @param {module:engine/view/text~Text} node Reference node. - * @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, direction = 'forward' ) { - const treeWalker = new ViewTreeWalker( { - 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 its container element. - return null; - } else if ( value.item.is( 'text' ) ) { - // Found a text node in the same container element. - return value.item; - } - } - - return null; -} diff --git a/tests/view/renderer.js b/tests/view/renderer.js index 799044ae9..7fc1ef786 100644 --- a/tests/view/renderer.js +++ b/tests/view/renderer.js @@ -214,29 +214,18 @@ describe( 'Renderer', () => { expect( renderer.markedTexts.size ).to.equal( 0 ); } ); - it( 'should update next touching text nodes of updated text node', () => { + it( 'should not update text parent child list changed', () => { + const viewImg = new ViewElement( 'img' ); const viewText = new ViewText( 'foo' ); - - viewRoot.appendChildren( [ - viewText, - new ViewAttributeElement( 'strong', null, new ViewText( ' bar' ) ) - ] ); + viewRoot.appendChildren( [ viewImg, viewText ] ); renderer.markToSync( 'children', viewRoot ); - renderer.render(); - - expect( domRoot.childNodes.length ).to.equal( 2 ); - expect( domRoot.childNodes[ 0 ].data ).to.equal( 'foo' ); - expect( domRoot.childNodes[ 1 ].childNodes[ 0 ].data ).to.equal( ' bar' ); - - viewText.data = 'foo '; - renderer.markToSync( 'text', viewText ); renderer.render(); expect( domRoot.childNodes.length ).to.equal( 2 ); - expect( domRoot.childNodes[ 0 ].data ).to.equal( 'foo ' ); - expect( domRoot.childNodes[ 1 ].childNodes[ 0 ].data ).to.equal( '\u00A0bar' ); + expect( domRoot.childNodes[ 0 ].tagName ).to.equal( 'IMG' ); + expect( domRoot.childNodes[ 1 ].data ).to.equal( 'foo' ); } ); it( 'should not change text if it is the same during text rendering', () => { diff --git a/tests/view/treewalker-utils.js b/tests/view/treewalker-utils.js deleted file mode 100644 index 8fc3c3281..000000000 --- a/tests/view/treewalker-utils.js +++ /dev/null @@ -1,72 +0,0 @@ -/** - * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved. - * For licensing, see LICENSE.md. - */ - -import ViewAttributeElement from '../../src/view/attributeelement'; -import ViewContainerElement from '../../src/view/containerelement'; -import ViewText from '../../src/view/text'; - -import { getTouchingTextNode } from '../../src/view/treewalker-utils'; - -describe( 'getTouchingTextNode()', () => { - 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' ); - - //

ab

xy

- /* eslint-disable no-new */ - new ViewContainerElement( 'div', null, [ - new ViewContainerElement( 'p', null, [ a, b ] ), - - 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, 'forward' ) ).to.equal( b ); - } ); - - it( 'should return previous touching view text node', () => { - expect( getTouchingTextNode( b, 'backward' ) ).to.equal( a ); - } ); - - it( 'should go outside of attribute element looking for text nodes', () => { - expect( getTouchingTextNode( x, 'forward' ) ).to.equal( y ); - } ); - - it( 'should go inside attribute element looking for text nodes', () => { - 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, '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, '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; - } ); -} );