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 #1748 from ckeditor/t/1747
Browse files Browse the repository at this point in the history 
Other: Changed how  s are generated on view->DOM rendering. Closes #1747.
  • Loading branch information
Piotr Jasiun authored Jun 17, 2019
2 parents e143f2f + 78511eb commit da5670a
Show file tree
Hide file tree
Showing 5 changed files with 95 additions and 99 deletions.
47 changes: 28 additions & 19 deletions src/view/domconverter.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -889,7 +889,8 @@ export default class DomConverter {
*
* * 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,
* * space at the end of the text node is changed to ` ` if there are two spaces at the end of a node or if next node
* starts with a space or if it 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.
Expand Down Expand Up @@ -918,15 +919,24 @@ export default class DomConverter {
}
}

// 2. Replace the last space with a nbsp if this is the last text node (container element boundary).
// 2. Replace the last space with nbsp if there are two spaces at the end or if the next node starts with space or there is no
// next node (container element boundary).
//
// Keep in mind that Firefox prefers $nbsp; before tag, not inside it:
//
// Foo <span>&nbsp;bar</span> <-- bad.
// Foo&nbsp;<span> bar</span> <-- good.
//
// More here: https://github.com/ckeditor/ckeditor5-engine/issues/1747.
if ( data.charAt( data.length - 1 ) == ' ' ) {
const nextNode = this._getTouchingViewTextNode( node, true );

if ( !nextNode ) {
if ( data.charAt( data.length - 2 ) == ' ' || !nextNode || nextNode.data.charAt( 0 ) == ' ' ) {
data = data.substr( 0, data.length - 1 ) + '\u00A0';
}
}

// 3. Create space+nbsp pairs.
return data.replace( / {2}/g, ' \u00A0' );
}

Expand Down Expand Up @@ -955,7 +965,9 @@ export default class DomConverter {
* * multiple whitespaces are replaced to a single space,
* * 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.
* * space at the end of the text node is removed if there are two spaces at the end of a node or if next node
* starts with a space or if it is the last text node in its container
* * nbsps are converted to spaces.
*
* @param {Node} node DOM text node to process.
* @returns {String} Processed data.
Expand Down Expand Up @@ -998,28 +1010,25 @@ export default class DomConverter {
data = getDataWithoutFiller( new Text( data ) );

// At this point we should have removed all whitespaces from DOM text data.

// Now we have to change &nbsp; chars, that were in DOM text data because of rendering reasons, to spaces.
// First, change all ` \u00A0` pairs (space + &nbsp;) to two spaces. DOM converter changes two spaces from model/view as
// ` \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.
//
// Now, We will reverse the process that happens in `_processDataFromViewText`.
//
// We have to change &nbsp; chars, that were in DOM text data because of rendering reasons, to spaces.
// First, change all ` \u00A0` pairs (space + &nbsp;) to two spaces. DOM converter changes two spaces from model/view to
// ` \u00A0` to ensure proper rendering. Since here we convert back, we recognize those pairs and change them back to ` `.
data = data.replace( / \u00A0/g, ' ' );

// Then, let's change the last nbsp to a space.
if ( /( |\u00A0)\u00A0$/.test( data ) || !nextNode || ( nextNode.data && nextNode.data.charAt( 0 ) == ' ' ) ) {
data = data.replace( /\u00A0$/, ' ' );
}

// 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 ( 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 with &nbsp; or is a <br>.
if ( isText( nextNode ) ? nextNode.data.charAt( 0 ) == '\u00A0' : true ) {
data = data.replace( /\u00A0( *)$/, ' $1' );
}

// At this point, all whitespaces should be removed and all &nbsp; created for rendering reasons should be
// changed to normal space. All left &nbsp; are &nbsp; inserted intentionally.
return data;
Expand Down Expand Up @@ -1048,7 +1057,7 @@ export default class DomConverter {
* be trimmed from the right side.
*
* @param {Node} node
* @param {Node} prevNode
* @param {Node} nextNode
*/
_checkShouldRightTrimDomText( node, nextNode ) {
if ( nextNode ) {
Expand Down
33 changes: 14 additions & 19 deletions tests/view/domconverter/dom-to-view.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,7 @@ describe( 'DomConverter', () => {
// At the end.
test( 'x_', 'x ' );
test( 'x _', 'x ' );
test( 'x_ _', 'x ' );
test( 'x __', 'x ' );
test( 'x _ _', 'x ' );

// In the middle.
Expand All @@ -464,7 +464,7 @@ 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 ' );
test( '_', ' ' );

Expand All @@ -477,10 +477,6 @@ describe( 'DomConverter', () => {
test( 'x_', 'x ' );
test( 'x__', 'x_ ' );
test( 'x___', 'x__ ' );
// This is an edge case, but it's impossible to write elegant and compact algorithm that is also
// 100% correct. We might assume that expected result is `x _` but it will be converted to `x `
// by the algorithm. This is acceptable, though.
test( 'x __', 'x ' );

test( 'x_x', 'x_x' );
test( 'x___x', 'x___x' );
Expand All @@ -496,26 +492,25 @@ describe( 'DomConverter', () => {
test( [ 'x', 'y' ], 'xy' );
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' );

// Some tests with hard &nbsp;
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' );

it( 'not in preformatted blocks', () => {
Expand Down Expand Up @@ -605,9 +600,9 @@ describe( 'DomConverter', () => {
expect( viewP.getChild( 0 ).data ).to.equal( 'foo ' );
} );

it( 'not before a <br> (nbsp+space)', () => {
it( 'not before a <br> (space+nbsp)', () => {
const domP = createElement( document, 'p', {}, [
document.createTextNode( 'foo\u00a0 ' ),
document.createTextNode( 'foo \u00a0' ),
createElement( document, 'br' )
] );

Expand Down
69 changes: 35 additions & 34 deletions tests/view/domconverter/view-to-dom.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ describe( 'DomConverter', () => {

const domDiv = converter.viewToDom( viewDiv, document );

expect( domDiv.innerHTML ).to.equal( 'x &nbsp;x &nbsp; x x <b>&nbsp;x </b><i><b><u>&nbsp;x</u></b></i>' );
expect( domDiv.innerHTML ).to.equal( 'x &nbsp;x &nbsp; x x&nbsp;<b> x&nbsp;</b><i><b><u> x</u></b></i>' );
} );

it( 'all together', () => {
Expand All @@ -232,7 +232,7 @@ describe( 'DomConverter', () => {
const domDiv = converter.viewToDom( viewDiv, document );

expect( domDiv.innerHTML ).to.equal(
'<p>&nbsp;x &nbsp;x &nbsp; x x <b>&nbsp;x </b><i><b><u>&nbsp;x&nbsp;</u></b></i></p><p>&nbsp; x &nbsp;</p>'
'<p>&nbsp;x &nbsp;x &nbsp; x x&nbsp;<b> x&nbsp;</b><i><b><u> x&nbsp;</u></b></i></p><p>&nbsp; x &nbsp;</p>'
);
} );

Expand Down Expand Up @@ -321,86 +321,87 @@ describe( 'DomConverter', () => {
test( 'x _ x', 'x _ x' );
test( 'x _ x', 'x __ _x' );

// Two text nodes.
test( [ 'x', 'y' ], 'xy' );
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' );

// Two text nodes.
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 _ _' );
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' );
test( [ ' ', ' x' ], '_ _ _x' );
test( [ ' ', ' x' ], '_ _ x' );
test( [ ' ', ' x' ], '_ _ _x' );
test( [ ' ', ' x' ], '__ _ x' );
test( [ ' ', ' x' ], '_ _ _ x' );

// "Non-empty" + "empty" text nodes.
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' );
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( [ ' ', ' ' ], '__ _' );
test( [ ' ', ' ' ], '__ __' );
test( [ ' ', ' ' ], '_ _ _' );
test( [ ' ', ' ' ], '_ _ __' );

Expand Down
19 changes: 5 additions & 14 deletions tests/view/domconverter/whitespace-handling-integration.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,20 +165,20 @@ describe( 'DomConverter – whitespace handling – integration', () => {
return editor.destroy();
} );

it( 'single spaces around <br>', () => {
it( 'single spaces around <br> #1', () => {
editor.setData( '<p>foo&nbsp;<br>&nbsp;bar</p>' );

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

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

it( 'single spaces around <br> (normalization)', () => {
it( 'single spaces around <br> #2', () => {
editor.setData( '<p>foo&nbsp;<br> bar</p>' );

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

expect( editor.getData() ).to.equal( '<p>foo&nbsp;<br>bar</p>' );
} );
Expand All @@ -192,16 +192,7 @@ describe( 'DomConverter – whitespace handling – integration', () => {
expect( editor.getData() ).to.equal( '<p>foo &nbsp;<br>bar</p>' );
} );

it( 'two spaces before a <br> (normalization)', () => {
editor.setData( '<p>foo&nbsp; <br>bar</p>' );

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

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

it( 'two spaces before a <br> (normalization to a model nbsp)', () => {
it( 'two nbsps before a <br>', () => {
editor.setData( '<p>foo&nbsp;&nbsp;<br>bar</p>' );

expect( getData( editor.model, { withoutSelection: true } ) )
Expand Down
Loading

0 comments on commit da5670a

Please sign in to comment.