Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Commit

Permalink
Merge pull request #1430 from ckeditor/t/ckeditor5/1024
Browse files Browse the repository at this point in the history 
Fix: Fixed view <-> DOM conversion of whitespaces around `<br>` elements. Closes ckeditor/ckeditor5#1024.
  • Loading branch information
@Reinmar
Reinmar committed Jun 11, 2018
2 parents ba3d641 + da38213 commit 3e74554
Show file tree
Hide file tree
Showing 4 changed files with 555 additions and 29 deletions.
109 changes: 88 additions & 21 deletions src/view/domconverter.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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( / $/, '' );
}

Expand All @@ -1001,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' );
}

Expand All @@ -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.
Expand All @@ -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;
}
}
Expand All @@ -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;
}
Expand All @@ -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;

Expand Down
122 changes: 122 additions & 0 deletions tests/view/domconverter/dom-to-view.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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', {}, [
Expand Down Expand Up @@ -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.
//
Expand Down
Loading

0 comments on commit 3e74554

Please sign in to comment.