From da382139b652f6f6693ae028736007ba4598d23d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Thu, 7 Jun 2018 17:53:16 +0200 Subject: [PATCH] Fixed spaces around
s convertion to the view. --- src/view/domconverter.js | 89 +++++++++-- tests/view/domconverter/dom-to-view.js | 122 +++++++++++++++ .../whitespace-handling-integration.js | 140 +++++++++++++++++- 3 files changed, 336 insertions(+), 15 deletions(-) diff --git a/src/view/domconverter.js b/src/view/domconverter.js index a8befd6a1..104b32a85 100644 --- a/src/view/domconverter.js +++ b/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   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   or is a
. + 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 `
`. 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: + * + *

foobar
bom

+ * + * * `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 `
` + * + * This method returns text nodes and `
` 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; diff --git a/tests/view/domconverter/dom-to-view.js b/tests/view/domconverter/dom-to-view.js index d01ec1df9..c3bf0cd76 100644 --- a/tests/view/domconverter/dom-to-view.js +++ b/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
', () => { + 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
– 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
. + // If not, situations like https://github.com/ckeditor/ckeditor5/issues/1024#issuecomment-393109558 might occur. + it( 'after a
– when
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
– when text after that
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
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 
X`, `X
X` is ok too – the space needs to be preserved. + it( 'not before a
', () => { + 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
(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
(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
– when text before that
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. // diff --git a/tests/view/domconverter/whitespace-handling-integration.js b/tests/view/domconverter/whitespace-handling-integration.js index e29ce0400..47fe30e35 100644 --- a/tests/view/domconverter/whitespace-handling-integration.js +++ b/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( '

foo

bar

' ); } ); } ); + + // https://github.com/ckeditor/ckeditor5/issues/1024 + describe( 'whitespaces around
s', () => { + beforeEach( () => { + return VirtualTestEditor + .create( { plugins: [ Paragraph, ShiftEnter ] } ) + .then( newEditor => { + editor = newEditor; + } ); + } ); + + afterEach( () => { + return editor.destroy(); + } ); + + it( 'single spaces around
', () => { + editor.setData( '

foo 
 bar

' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( 'foo bar' ); + + expect( editor.getData() ).to.equal( '

foo 
 bar

' ); + } ); + + it( 'single spaces around
(normalization)', () => { + editor.setData( '

foo 
bar

' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( 'foo bar' ); + + expect( editor.getData() ).to.equal( '

foo 
bar

' ); + } ); + + it( 'two spaces before a
', () => { + editor.setData( '

foo  
bar

' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( 'foo bar' ); + + expect( editor.getData() ).to.equal( '

foo  
bar

' ); + } ); + + it( 'two spaces before a
(normalization)', () => { + editor.setData( '

foo 
bar

' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( 'foo bar' ); + + expect( editor.getData() ).to.equal( '

foo  
bar

' ); + } ); + + it( 'two spaces before a
(normalization to a model nbsp)', () => { + editor.setData( '

foo  
bar

' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( 'foo\u00a0 bar' ); + + expect( editor.getData() ).to.equal( '

foo  
bar

' ); + } ); + + it( 'single space after a
', () => { + editor.setData( '

foo
 bar

' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( 'foo bar' ); + + expect( editor.getData() ).to.equal( '

foo
 bar

' ); + } ); + + it( 'single space after a
(normalization)', () => { + editor.setData( '

foo
bar

' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( 'foobar' ); + + expect( editor.getData() ).to.equal( '

foo
bar

' ); + } ); + + it( 'two spaces after a
', () => { + editor.setData( '

foo
  bar

' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( 'foo bar' ); + + expect( editor.getData() ).to.equal( '

foo
  bar

' ); + } ); + + it( 'two spaces after a
(normalization)', () => { + editor.setData( '

foo
 bar

' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( 'foo bar' ); + + expect( editor.getData() ).to.equal( '

foo
 bar

' ); + } ); + + it( 'two spaces after a
(normalization to a model nbsp)', () => { + editor.setData( '

foo
  bar

' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( 'foo \u00a0bar' ); + + expect( editor.getData() ).to.equal( '

foo
  bar

' ); + } ); + + // https://github.com/ckeditor/ckeditor5-engine/issues/1429 + // it( 'space between
s', () => { + // editor.setData( '

foo
 
bar

' ); + + // expect( getData( editor.model, { withoutSelection: true } ) ) + // .to.equal( 'foo bar' ); + + // expect( editor.getData() ).to.equal( '

foo
 
bar

' ); + // } ); + + it( 'space between
s (normalization)', () => { + editor.setData( '

foo

bar

' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( 'foobar' ); + + expect( editor.getData() ).to.equal( '

foo

bar

' ); + } ); + + it( 'two spaces between
s', () => { + editor.setData( '

foo
  
bar

' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( 'foo bar' ); + + expect( editor.getData() ).to.equal( '

foo
  
bar

' ); + } ); + } ); } );