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 #988 from ckeditor/t/822
Browse files Browse the repository at this point in the history 
Fix: Singular white spaces (new lines, tabs and carriage returns) will be ignored when loading data when used outside/between block elements. Closes #822.

Also, the range of characters which are being normalized during DOM to view conversion was reduced to `[ \n\t\r]` to avoid losing space characters (which matches `/\s/`) that could be significant.
  • Loading branch information
@szymonkups
szymonkups authored Jun 30, 2017
2 parents 93639d0 + 9763270 commit 4c9a0af
Show file tree
Hide file tree
Showing 3 changed files with 289 additions and 4 deletions.
11 changes: 7 additions & 4 deletions src/view/domconverter.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1037,10 +1037,11 @@ export default class DomConverter {
return data;
}

// Change all consecutive whitespace characters to a single space character. That's how multiple whitespaces
// are treated when rendered, so we normalize those whitespaces.
// Note that   (`\u00A0`) should not be treated as a whitespace because it is rendered.
data = data.replace( /[^\S\u00A0]{2,}/g, ' ' );
// Change all consecutive whitespace characters (from the [ \n\t\r] set –
// see https://github.com/ckeditor/ckeditor5-engine/issues/822#issuecomment-311670249) to a single space character.
// That's how multiple whitespaces are treated when rendered, so we normalize those whitespaces.
// 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 );
Expand All @@ -1062,12 +1063,14 @@ export default class DomConverter {
// ` \u00A0` to ensure proper rendering. Since here we convert back, we recognize those pairs and change them
// to ` ` which is what we expect to have in model/view.
data = data.replace( / \u00A0/g, ' ' );

// 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 ) ) ) {
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).
Expand Down
134 changes: 134 additions & 0 deletions tests/view/domconverter/dom-to-view.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,111 @@ describe( 'DomConverter', () => {
expect( viewDiv.getChild( 1 ).getChild( 0 ).data ).to.equal( 'foo' );
} );

it( 'after a block element', () => {
const domDiv = createElement( document, 'div', {}, [
createElement( document, 'p', {}, [
document.createTextNode( 'foo' )
] ),
document.createTextNode( ' ' )
] );

const viewDiv = converter.domToView( domDiv );

expect( viewDiv.childCount ).to.equal( 1 );
expect( viewDiv.getChild( 0 ).getChild( 0 ).data ).to.equal( 'foo' );
} );

it( 'after a block element (new line)', () => {
const domDiv = createElement( document, 'div', {}, [
createElement( document, 'p', {}, [
document.createTextNode( 'foo' )
] ),
document.createTextNode( '\n' )
] );

const viewDiv = converter.domToView( domDiv );

expect( viewDiv.childCount ).to.equal( 1 );
expect( viewDiv.getChild( 0 ).getChild( 0 ).data ).to.equal( 'foo' );
} );

it( 'after a block element (carriage return)', () => {
const domDiv = createElement( document, 'div', {}, [
createElement( document, 'p', {}, [
document.createTextNode( 'foo' )
] ),
document.createTextNode( '\r' )
] );

const viewDiv = converter.domToView( domDiv );

expect( viewDiv.childCount ).to.equal( 1 );
expect( viewDiv.getChild( 0 ).getChild( 0 ).data ).to.equal( 'foo' );
} );

it( 'after a block element (tab)', () => {
const domDiv = createElement( document, 'div', {}, [
createElement( document, 'p', {}, [
document.createTextNode( 'foo' )
] ),
document.createTextNode( '\t' )
] );

const viewDiv = converter.domToView( domDiv );

expect( viewDiv.childCount ).to.equal( 1 );
expect( viewDiv.getChild( 0 ).getChild( 0 ).data ).to.equal( 'foo' );
} );

// See https://github.com/ckeditor/ckeditor5-engine/issues/822#issuecomment-311670249
it( 'but preserve all except " \\n\\r\\t"', () => {
const domDiv = createElement( document, 'div', {}, [
createElement( document, 'p', {}, [
document.createTextNode( 'x\fx\vx\u00a0x\u1680x\u2000x\u200ax\u2028x\u2029x\u202fx\u205fx\u3000x\ufeffx' )
] ),
createElement( document, 'p', {}, [
// x<two spaces>x because it behaved differently than "x<space>x" when I've been fixing this
document.createTextNode( 'x\f\vx\u00a0\u1680x\u2000\u200ax\u2028\u2029x\u202f\u205fx\u3000\ufeffx' )
] )
] );

const viewDiv = converter.domToView( domDiv );

expect( viewDiv.childCount ).to.equal( 2 );
expect( viewDiv.getChild( 0 ).getChild( 0 ).data )
.to.equal( 'x\fx\vx\u00a0x\u1680x\u2000x\u200ax\u2028x\u2029x\u202fx\u205fx\u3000x\ufeffx' );
expect( viewDiv.getChild( 1 ).getChild( 0 ).data )
.to.equal( 'x\f\vx\u00a0\u1680x\u2000\u200ax\u2028\u2029x\u202f\u205fx\u3000\ufeffx' );
} );

it( 'before a block element', () => {
const domDiv = createElement( document, 'div', {}, [
document.createTextNode( ' ' ),
createElement( document, 'p', {}, [
document.createTextNode( ' foo' )
] )
] );

const viewDiv = converter.domToView( domDiv );

expect( viewDiv.childCount ).to.equal( 1 );
expect( viewDiv.getChild( 0 ).getChild( 0 ).data ).to.equal( 'foo' );
} );

it( 'before a block element (new line)', () => {
const domDiv = createElement( document, 'div', {}, [
document.createTextNode( '\n' ),
createElement( document, 'p', {}, [
document.createTextNode( 'foo' )
] )
] );

const viewDiv = converter.domToView( domDiv );

expect( viewDiv.childCount ).to.equal( 1 );
expect( viewDiv.getChild( 0 ).getChild( 0 ).data ).to.equal( 'foo' );
} );

it( 'multiple consecutive whitespaces changed to one', () => {
const domDiv = createElement( document, 'div', {}, [
createElement( document, 'p', {}, [
Expand All @@ -228,6 +333,21 @@ describe( 'DomConverter', () => {
expect( viewDiv.getChild( 1 ).getChild( 0 ).data ).to.equal( 'fo o' );
} );

it( 'multiple consecutive whitespaces changed to one (tab, new line, carriage return)', () => {
const domDiv = createElement( document, 'div', {}, [
document.createTextNode( '\n\n \t\r\n' ),
createElement( document, 'p', {}, [
document.createTextNode( 'f\n\t\r\n\to\n\n\no' )
] ),
document.createTextNode( '\n\n \t\r\n' )
] );

const viewDiv = converter.domToView( domDiv );

expect( viewDiv.childCount ).to.equal( 1 );
expect( viewDiv.getChild( 0 ).getChild( 0 ).data ).to.equal( 'f o o' );
} );

function test( inputTexts, output ) {
if ( typeof inputTexts == 'string' ) {
inputTexts = [ inputTexts ];
Expand Down Expand Up @@ -339,6 +459,10 @@ describe( 'DomConverter', () => {

expect( viewDiv.getChild( 0 ).getChild( 0 ).data ).to.equal( ' foo\n foo ' );
} );

//
// See also whitespace-handling-integration.js.
//
} );
} );

Expand Down Expand Up @@ -602,6 +726,8 @@ describe( 'DomConverter', () => {

expect( viewSelection.rangeCount ).to.equal( 1 );
expect( stringify( viewP, viewSelection.getFirstRange() ) ).to.equal( '<p>f{oo<b>ba}r</b></p>' );

domP.remove();
} );

it( 'should convert empty selection to empty selection', () => {
Expand Down Expand Up @@ -638,6 +764,8 @@ describe( 'DomConverter', () => {
expect( viewSelection.anchor.offset ).to.equal( 2 );
expect( viewSelection.focus.offset ).to.equal( 1 );
expect( viewSelection.isBackward ).to.be.true;

domP.remove();
} );

it( 'should not add null ranges', () => {
Expand All @@ -659,6 +787,8 @@ describe( 'DomConverter', () => {
const viewSelection = converter.domSelectionToView( domSelection );

expect( viewSelection.rangeCount ).to.equal( 0 );

domP.remove();
} );

it( 'should return fake selection', () => {
Expand All @@ -679,6 +809,8 @@ describe( 'DomConverter', () => {
const bindViewSelection = converter.domSelectionToView( domSelection );

expect( bindViewSelection.isEqual( viewSelection ) ).to.be.true;

domContainer.remove();
} );

it( 'should return fake selection if selection is placed inside text node', () => {
Expand All @@ -699,6 +831,8 @@ describe( 'DomConverter', () => {
const bindViewSelection = converter.domSelectionToView( domSelection );

expect( bindViewSelection.isEqual( viewSelection ) ).to.be.true;

domContainer.remove();
} );
} );
} );
148 changes: 148 additions & 0 deletions tests/view/domconverter/whitespace-handling-integration.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
/**
* @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';

import { getData } from '../../../src/dev-utils/model';

describe( 'DomConverter – whitespace handling – integration', () => {
let editor;

// See https://github.com/ckeditor/ckeditor5-engine/issues/822.
describe( 'data loading', () => {
beforeEach( () => {
return VirtualTestEditor
.create( { plugins: [ Paragraph ] } )
.then( newEditor => {
editor = newEditor;
} );
} );

afterEach( () => {
return editor.destroy();
} );

it( 'new line at the end of the content is ignored', () => {
editor.setData( '<p>foo</p>\n' );

expect( getData( editor.document, { withoutSelection: true } ) )
.to.equal( '<paragraph>foo</paragraph>' );

expect( editor.getData() ).to.equal( '<p>foo</p>' );
} );

it( 'whitespaces at the end of the content are ignored', () => {
editor.setData( '<p>foo</p>\n\r\n \t' );

expect( getData( editor.document, { withoutSelection: true } ) )
.to.equal( '<paragraph>foo</paragraph>' );

expect( editor.getData() ).to.equal( '<p>foo</p>' );
} );

// Controversial result. See https://github.com/ckeditor/ckeditor5-engine/issues/987.
it( 'nbsp at the end of the content is not ignored', () => {
editor.setData( '<p>foo</p>' );

expect( getData( editor.document, { withoutSelection: true } ) )
.to.equal( '<paragraph>foo</paragraph>' );

expect( editor.getData() ).to.equal( '<p>foo</p>' );
} );

it( 'new line at the beginning of the content is ignored', () => {
editor.setData( '\n<p>foo</p>' );

expect( getData( editor.document, { withoutSelection: true } ) )
.to.equal( '<paragraph>foo</paragraph>' );

expect( editor.getData() ).to.equal( '<p>foo</p>' );
} );

it( 'whitespaces at the beginning of the content are ignored', () => {
editor.setData( '\n\n \t<p>foo</p>' );

expect( getData( editor.document, { withoutSelection: true } ) )
.to.equal( '<paragraph>foo</paragraph>' );

expect( editor.getData() ).to.equal( '<p>foo</p>' );
} );

// Controversial result. See https://github.com/ckeditor/ckeditor5-engine/issues/987.
it( 'nbsp at the beginning of the content is not ignored', () => {
editor.setData( '<p>foo</p>' );

expect( getData( editor.document, { withoutSelection: true } ) )
.to.equal( '<paragraph>foo</paragraph>' );

expect( editor.getData() ).to.equal( '<p>foo</p>' );
} );

it( 'new line between blocks is ignored', () => {
editor.setData( '<p>foo</p>\n<p>bar</p>' );

expect( getData( editor.document, { withoutSelection: true } ) )
.to.equal( '<paragraph>foo</paragraph><paragraph>bar</paragraph>' );

expect( editor.getData() ).to.equal( '<p>foo</p><p>bar</p>' );
} );

it( 'whitespaces between blocks are ignored', () => {
editor.setData( '<p>foo</p>\n\n \t<p>bar</p>' );

expect( getData( editor.document, { withoutSelection: true } ) )
.to.equal( '<paragraph>foo</paragraph><paragraph>bar</paragraph>' );

expect( editor.getData() ).to.equal( '<p>foo</p><p>bar</p>' );
} );

// Controversial result. See https://github.com/ckeditor/ckeditor5-engine/issues/987.
it( 'nbsp between blocks is not ignored', () => {
editor.setData( '<p>foo</p>&nbsp;<p>bar</p>' );

expect( getData( editor.document, { withoutSelection: true } ) )
.to.equal( '<paragraph>foo</paragraph><paragraph>bar</paragraph>' );

expect( editor.getData() ).to.equal( '<p>foo</p><p>bar</p>' );
} );

it( 'new lines inside blocks are ignored', () => {
editor.setData( '<p>\nfoo\n</p>' );

expect( getData( editor.document, { withoutSelection: true } ) )
.to.equal( '<paragraph>foo</paragraph>' );

expect( editor.getData() ).to.equal( '<p>foo</p>' );
} );

it( 'whitespaces inside blocks are ignored', () => {
editor.setData( '<p>\n\n \tfoo\n\n \t</p>' );

expect( getData( editor.document, { withoutSelection: true } ) )
.to.equal( '<paragraph>foo</paragraph>' );

expect( editor.getData() ).to.equal( '<p>foo</p>' );
} );

it( 'nbsp inside blocks are not ignored', () => {
editor.setData( '<p>&nbsp;foo&nbsp;</p>' );

expect( getData( editor.document, { withoutSelection: true } ) )
.to.equal( '<paragraph> foo </paragraph>' );

expect( editor.getData() ).to.equal( '<p>&nbsp;foo&nbsp;</p>' );
} );

it( 'all whitespaces together are ignored', () => {
editor.setData( '\n<p>foo\n\r\n \t</p>\n<p> bar</p>' );

expect( getData( editor.document, { withoutSelection: true } ) )
.to.equal( '<paragraph>foo</paragraph><paragraph>bar</paragraph>' );

expect( editor.getData() ).to.equal( '<p>foo</p><p>bar</p>' );
} );
} );
} );

0 comments on commit 4c9a0af

Please sign in to comment.