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

Fixed view <-> DOM conversion of whitespaces around <br> elements #1430

Merged
merged 3 commits into from
Jun 11, 2018
Merged

Fixed view <-> DOM conversion of whitespaces around <br> elements #1430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f4de0eb
Minor docs fixes.
Reinmar Jun 7, 2018
8ff62da
Fixed nbsp encoding around <br>s.
Reinmar Jun 7, 2018
da38213
Fixed spaces around <br>s convertion to the view.
Reinmar Jun 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 88 additions & 21 deletions src/view/domconverter.js
View file
Open in desktop
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
Open in desktop
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