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

T/ckeditor5 typing/101 Multiple spaces fix #1083

Merged
merged 9 commits into from
Aug 22, 2017
Merged

T/ckeditor5 typing/101 Multiple spaces fix #1083

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
46084a2
Changed: Introduced `view.utils` and `view.utils.getTouchingTextNode`.
scofalik Aug 18, 2017
91dbb6a
Fixed: Fixed handling spaces in view text nodes in view-to-dom conver…
scofalik Aug 18, 2017
99d5f24
Changed: Renderer should re-render also next text node if a text node…
scofalik Aug 18, 2017
e29624b
Merge branch 'master' into t/ckeditor5-typing/101
Reinmar Aug 21, 2017
4ca5095
Docs and minor regexp correction.
Reinmar Aug 21, 2017
dbb7b6f
Changed: minor fixes and several new tests for handling spaces in vie…
scofalik Aug 22, 2017
decc702
Merge branch 'master' into t/ckeditor5-typing/101
Reinmar Aug 22, 2017
4aaaf7b
Changed: Reverted refreshing touching text nodes in `view.Renderer`. …
scofalik Aug 22, 2017
9515717
Merge branch 'master' into t/ckeditor5-typing/101
Reinmar Aug 22, 2017
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
114 changes: 26 additions & 88 deletions src/view/domconverter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ import ViewPosition from './position';
import ViewRange from './range';
import ViewSelection from './selection';
import ViewDocumentFragment from './documentfragment';
import ViewTreeWalker from './treewalker';
import { BR_FILLER, INLINE_FILLER_LENGTH, isBlockFiller, isInlineFiller, startsWithFiller, getDataWithoutFiller } from './filler';
import { getTouchingTextNode } from './utils';

import global from '@ckeditor/ckeditor5-utils/src/dom/global';
import indexOf from '@ckeditor/ckeditor5-utils/src/dom/indexof';
Expand Down Expand Up @@ -65,7 +65,7 @@ export default class DomConverter {
*
* @member {Array.<String>} module:engine/view/domconverter~DomConverter#preElements
*/
this.preElements = [ 'pre' ];
this.preElements = [ 'pre', 'code' ];

/**
* Tag names of DOM `Element`s which are considered block elements.
Expand Down Expand Up @@ -890,122 +890,60 @@ export default class DomConverter {
* Following changes are done:
*
* * multiple spaces are replaced to a chain of spaces and `&nbsp;`,
* * space at the beginning of the text node is changed to `&nbsp;` if it is a first text node in it's container
* element or if previous text node ends by space character,
* * space at the beginning and at the end of the text node is changed to `&nbsp;` if it is a first text node in it's container
* element or if previous text node ends with space character,
* * space at the end of the text node is changed to `&nbsp;` if it is a last text node in it's container.
*
* @private
* @param {module:engine/view/text~Text} node View text node to process.
* @returns {String} Processed text data.
*/
_processDataFromViewText( node ) {
const data = node.data;
let data = node.data;

// If any of node ancestors has a name which is in `preElements` array, then currently processed
// view text node is (will be) in preformatted element. We should not change whitespaces then.
if ( node.getAncestors().some( parent => this.preElements.includes( parent.name ) ) ) {
return data;
}

const prevNode = this._getTouchingViewTextNode( node, false );
const nextNode = this._getTouchingViewTextNode( node, true );

// Second part of text data, from the space after the last non-space character to the end.
// We separate `textEnd` and `textStart` because `textEnd` needs some special handling.
let textEnd = data.match( / *$/ )[ 0 ];
// First part of data, between first and last part of data.
let textStart = data.substr( 0, data.length - textEnd.length );

// If previous text node does not exist or it ends by space character, replace space character at the beginning of text.
// ` x` -> `_x`
// ` x` -> `_ x`
// ` x` -> `_ x`
if ( !prevNode || prevNode.data.charAt( prevNode.data.length - 1 ) == ' ' ) {
textStart = textStart.replace( /^ /, '\u00A0' );
// 1. Replace first space with nbsp if previous node ends with space or there is no previous node (container element boundary).
if ( data.charAt( 0 ) == ' ' ) {
const prevNode = getTouchingTextNode( node, false );
const prevEndsWithSpace = prevNode && this._nodeEndsWithSpace( prevNode );

if ( prevEndsWithSpace || !prevNode ) {
data = '\u00A0' + data.substr( 1 );
}
}

// Multiple consecutive spaces. Change them to ` &nbsp;` pairs.
// `_x x` -> `_x _x`
// `_ x x` -> `_ x _x`
// `_ x x` -> `_ _x _x`
// `_ x x` -> `_ _x _ x`
// `_ x x` -> `_ _x _ _x`
// `_ x x` -> `_ _ x _ _x`
textStart = textStart.replace( / {2}/g, ' \u00A0' );

// Process `textEnd` only if there is anything to process.
if ( textEnd.length > 0 ) {
// (1) We need special treatment for the last part of text node, it has to end on `&nbsp;`, not space:
// `x ` -> `x_`
// `x ` -> `x _`
// `x ` -> `x_ _`
// `x ` -> `x _ _`
// (2) Different case when there is a node after:
// `x <b>b</b>` -> `x <b>b</b>`
// `x <b>b</b>` -> `x _<b>b</b>`
// `x <b>b</b>` -> `x _ <b>b</b>`
// `x <b>b</b>` -> `x _ _<b>b</b>`
// (3) But different, when that node starts by &nbsp; (or space that will be converted to &nbsp;):
// `x <b>_b</b>` -> `x <b>_b</b>`
// `x <b>_b</b>` -> `x_ <b>_b</b>`
// `x <b>_b</b>` -> `x _ <b>_b</b>`
// `x <b>_b</b>` -> `x_ _ <b>_b</b>`
// Let's assume that starting from space is normal behavior, because starting from &nbsp; is a less frequent case.
let textEndStartsFromNbsp = false;
// 2. Replace last space with nbsp if it is the last text node (container element boundary).
if ( data.charAt( data.length - 1 ) == ' ' ) {
const nextNode = getTouchingTextNode( node, true );

if ( !nextNode ) {
// (1)
if ( textEnd.length % 2 ) {
textEndStartsFromNbsp = true;
}
} else if ( nextNode.data.charAt( 0 ) == ' ' || nextNode.data.charAt( 0 ) == '\u00A0' ) {
// (3)
if ( textEnd.length % 2 === 0 ) {
textEndStartsFromNbsp = true;
}
data = data.substr( 0, data.length - 1 ) + '\u00A0';
}

if ( textEndStartsFromNbsp ) {
textEnd = '\u00A0' + textEnd.substr( 0, textEnd.length - 1 );
}

textEnd = textEnd.replace( / {2}/g, ' \u00A0' );
}

return textStart + textEnd;
return data.replace( / {2}/gi, ' \u00A0' );
}

/**
* 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 block element. If there is no such sibling, `null` is returned.
* Checks whether given node ends with a space character after changing appropriate space characters to `&nbsp;`s.
*
* @private
* @param {module:engine/view/text~Text} node
* @param {Boolean} getNext
* @returns {module:engine/view/text~Text}
* @param {module:engine/view/text~Text} node Node to check.
* @returns {Boolean} `true` if given `node` ends with space, `false` otherwise.
*/
_getTouchingViewTextNode( node, getNext ) {
if ( !node.parent ) {
return null;
_nodeEndsWithSpace( node ) {
if ( node.getAncestors().some( parent => this.preElements.includes( parent.name ) ) ) {
return false;
}

const treeWalker = new ViewTreeWalker( {
startPosition: getNext ? ViewPosition.createAfter( node ) : ViewPosition.createBefore( node ),
direction: getNext ? 'forward' : 'backward'
} );

for ( const value of treeWalker ) {
if ( value.item.is( 'containerElement' ) ) {
// ViewContainerElement is found on a way to next ViewText node, so given `node` was first/last
// text node in it's container element.
return null;
} else if ( value.item.is( 'text' ) ) {
// Found a text node in the same container element.
return value.item;
}
}
const data = this._processDataFromViewText( node );

return null;
return data.charAt( data.length - 1 ) == ' ';
}

/**
Expand Down
17 changes: 14 additions & 3 deletions src/view/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import ViewText from './text';
import ViewPosition from './position';
import { INLINE_FILLER, INLINE_FILLER_LENGTH, startsWithFiller, isInlineFiller, isBlockFiller } from './filler';
import { getTouchingTextNode } from './utils';

import mix from '@ckeditor/ckeditor5-utils/src/mix';
import diff from '@ckeditor/ckeditor5-utils/src/diff';
Expand Down Expand Up @@ -198,9 +199,7 @@ export default class Renderer {
}

for ( const node of this.markedTexts ) {
if ( !this.markedChildren.has( node.parent ) && this.domConverter.mapViewToDom( node.parent ) ) {
this._updateText( node, { inlineFillerPosition } );
}
this._updateText( node, { inlineFillerPosition } );
}

for ( const element of this.markedAttributes ) {
Expand Down Expand Up @@ -407,6 +406,12 @@ export default class Renderer {
*/
_updateText( viewText, options ) {
const domText = this.domConverter.findCorrespondingDomText( viewText );

// If this is a new text node and it is not in DOM, it will be created and handled in `_updateChildren`.
if ( !domText ) {
Copy link
Member

@Reinmar Reinmar Aug 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very suspicious. How did this function work without this condition so far? Why is it needed here now?

Copy link
Contributor Author

@scofalik scofalik Aug 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It worked because there was a condition in view.Renderer#render method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So why doesn't that condition save us from adding one here?

Copy link
Contributor Author

@scofalik scofalik Aug 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. Before there was a condition in render method in the loop where _updateText is called. I removed this condition because I wanted _updateText to be fired more often. But this caused a situation where _upadateText was fired on text nodes that weren't yet converted to DOM. That's why I needed to add the statement in _updateText.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aaand it turned out we can't make this change ;> We'll also need to make a step back and remove the fix for subsequent text nodes updating. It seems that we may not be able to finalise it now because of how the text nodes are mapped.

We can neither update all text nodes before children are updated because changes in children might've changed the indexes of text nodes (so updating text nodes would fail). Changing the order to 1) update children, 2) update text nodes doesn't help too because in the following case, the whole paragraph is updated first and then text nodes are not:

  1. <p>x<strong> b</strong></p>
  2. Type space after "x".
  3. Paragraph's children get updated (because for some reason the browser mutated text nodes too much when typing the space).
  4. The "x " text node is up to date when it comes to updating text nodes, so we don't mark " b" to be updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scofalik will you report a ticket for this case and the one you mentioned in #1083 (comment)? And of course, please revert changes which we can't do now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/ckeditor/ckeditor5-engine/issues/1093

return;
}

const newDomText = this.domConverter.viewToDom( viewText, domText.ownerDocument );

const actualText = domText.data;
Expand All @@ -420,6 +425,12 @@ export default class Renderer {

if ( actualText != expectedText ) {
domText.data = expectedText;

const nextTouchingTextNode = getTouchingTextNode( viewText, true );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing comment why we need to do that.


if ( nextTouchingTextNode ) {
this.markedTexts.add( nextTouchingTextNode );
}
}
}

Expand Down
41 changes: 41 additions & 0 deletions src/view/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

import ViewPosition from './position';
import ViewTreeWalker from './treewalker';

/**
* Contains utility functions for working on view.
*
* @module engine/view/utils
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this to walker-utils since the utils that come to our mind are Walker related. We also considered putting this to the walker's module but we don't want to make it too big (hence, a new module).

*/

/**
* 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.
*
* @param {module:engine/view/text~Text} node Reference node.
* @param {Boolean} getNext If `true` next touching sibling will be returned. If `false` previous touching sibling will be returned.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to find it a better name... but I can't :D. lookRight?

Also, this is an optional param. Should be marked with []. Maybe this will clarify its meaning (typically, boolean flags are optional).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for this flag being optional – it's always passed in the tests: https://github.com/ckeditor/ckeditor5-engine/pull/1083/files#diff-41de3c28cf4c9630dfaec4e35850f0faR38. This should be changed also there.

* @returns {module:engine/view/text~Text|null} Touching text node or `null` if there is no next or previous touching text node.
*/
export function getTouchingTextNode( node, getNext ) {
const treeWalker = new ViewTreeWalker( {
startPosition: getNext ? ViewPosition.createAfter( node ) : ViewPosition.createBefore( node ),
direction: getNext ? 'forward' : 'backward'
} );

for ( const value of treeWalker ) {
if ( value.item.is( 'containerElement' ) ) {
// ViewContainerElement is found on a way to next ViewText node, so given `node` was first/last
// text node in it's container element.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its

return null;
} else if ( value.item.is( 'text' ) ) {
// Found a text node in the same container element.
return value.item;
}
}

return null;
}
79 changes: 67 additions & 12 deletions tests/view/domconverter/view-to-dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,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 @@ -282,11 +282,19 @@ 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_' );

// Only spaces.
test( ' ', '_' );
test( ' ', '__' );
test( ' ', '_ _' );
test( ' ', '_ __' );
test( ' ', '_ _ _' );
test( ' ', '_ _ __' );

// With hard &nbsp;
// It should be treated like a normal sign.
test( '_x', '_x' );
test( ' _x', '__x' );
test( ' _x', '_ _x' );
Expand Down Expand Up @@ -323,33 +331,80 @@ describe( 'DomConverter', () => {

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' );
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about mirrored cases with "empty" + "non-empty"?

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' ], '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( [ ' ', ' ' ], '_ _ __' );

it( 'not in preformatted blocks', () => {
const viewDiv = new ViewContainerElement( 'pre', null, new ViewText( ' foo ' ) );
const viewDiv = new ViewContainerElement( 'pre', null, [ new ViewText( ' foo ' ), new ViewText( ' bar ' ) ] );
const domDiv = converter.viewToDom( viewDiv, document );

expect( domDiv.innerHTML ).to.equal( ' foo bar ' );
} );

it( 'text node before in a preformatted node', () => {
const viewCode = new ViewAttributeElement( 'code', null, new ViewText( 'foo ' ) );
const viewDiv = new ViewContainerElement( 'div', null, [ viewCode, new ViewText( ' bar' ) ] );
const domDiv = converter.viewToDom( viewDiv, document );

expect( domDiv.innerHTML ).to.equal( ' foo ' );
expect( domDiv.innerHTML ).to.equal( '<code>foo </code> bar' );
} );
} );
} );
Expand Down
Loading