Merge pull request #1779 from ckeditor/t/404

Fix: Remove only real block fillers on DOM to view conversion. Closes #404.

BREAKING CHANGE: The behavior of block filler detection on DOM to view conversion was changed. Now, it specifically checks the parent of a text node to check whether it is a block. Which means that a list of block element names has to be used. If you use custom elements or use one of the HTML elements which CKEditor 5 does not recognize as a block element, see #404 and `DomConverter.blockElements` documentation.

src/dataprocessor/htmldataprocessor.js

@@ -11,7 +11,6 @@
 
 import BasicHtmlWriter from './basichtmlwriter';
 import DomConverter from '../view/domconverter';
-import { NBSP_FILLER } from '../view/filler';
 
 /**
  * The HTML data processor class.
@@ -38,7 +37,7 @@ export default class HtmlDataProcessor {
 		 * @private
 		 * @member {module:engine/view/domconverter~DomConverter}
 		 */
-		this._domConverter = new DomConverter( { blockFiller: NBSP_FILLER } );
+		this._domConverter = new DomConverter( { blockFillerMode: 'nbsp' } );
 
 		/**
 		 * A basic HTML writer instance used to convert DOM elements to an HTML string.

src/dataprocessor/xmldataprocessor.js

@@ -11,7 +11,6 @@
 
 import BasicHtmlWriter from './basichtmlwriter';
 import DomConverter from '../view/domconverter';
-import { NBSP_FILLER } from '../view/filler';
 
 /**
  * The XML data processor class.
@@ -54,7 +53,7 @@ export default class XmlDataProcessor {
 		 * @private
 		 * @member {module:engine/view/domconverter~DomConverter}
 		 */
-		this._domConverter = new DomConverter( { blockFiller: NBSP_FILLER } );
+		this._domConverter = new DomConverter( { blockFillerMode: 'nbsp' } );
 
 		/**
 		 * A basic HTML writer instance used to convert DOM elements to an XML string.

src/view/domconverter.js

@@ -16,7 +16,7 @@ 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 { BR_FILLER, getDataWithoutFiller, INLINE_FILLER_LENGTH, isInlineFiller, NBSP_FILLER, startsWithFiller } from './filler';
 
 import global from '@ckeditor/ckeditor5-utils/src/dom/global';
 import indexOf from '@ckeditor/ckeditor5-utils/src/dom/indexof';
@@ -25,6 +25,9 @@ import getCommonAncestor from '@ckeditor/ckeditor5-utils/src/dom/getcommonancest
 import isText from '@ckeditor/ckeditor5-utils/src/dom/istext';
 import { isElement } from 'lodash-es';
 
+// eslint-disable-next-line new-cap
+const BR_FILLER_REF = BR_FILLER( document );
+
 /**
  * DomConverter is a set of tools to do transformations between DOM nodes and view nodes. It also handles
  * {@link module:engine/view/domconverter~DomConverter#bindElements binding} these nodes.
@@ -42,43 +45,47 @@ export default class DomConverter {
 	 * Creates DOM converter.
 	 *
 	 * @param {Object} options Object with configuration options.
-	 * @param {Function} [options.blockFiller=module:engine/view/filler~BR_FILLER] Block filler creator.
+	 * @param {module:engine/view/filler~BlockFillerMode} [options.blockFillerMode='br'] The type of the block filler to use.
 	 */
 	constructor( options = {} ) {
-		// Using WeakMap prevent memory leaks: when the converter will be destroyed all referenced between View and DOM
-		// will be removed. Also because it is a *Weak*Map when both view and DOM elements will be removed referenced
-		// will be also removed, isn't it brilliant?
-		//
-		// Yes, PJ. It is.
-		//
-		// You guys so smart.
-		//
-		// I've been here. Seen stuff. Afraid of code now.
-
 		/**
-		 * Block {@link module:engine/view/filler filler} creator, which is used to create all block fillers during the
-		 * view to DOM conversion and to recognize block fillers during the DOM to view conversion.
+		 * The mode of a block filler used by DOM converter.
 		 *
 		 * @readonly
-		 * @member {Function} module:engine/view/domconverter~DomConverter#blockFiller
+		 * @member {'br'|'nbsp'} module:engine/view/domconverter~DomConverter#blockFillerMode
 		 */
-		this.blockFiller = options.blockFiller || BR_FILLER;
+		this.blockFillerMode = options.blockFillerMode || 'br';
 
 		/**
-		 * Tag names of DOM `Element`s which are considered pre-formatted elements.
+		 * Elements which are considered pre-formatted elements.
 		 *
 		 * @readonly
 		 * @member {Array.<String>} module:engine/view/domconverter~DomConverter#preElements
 		 */
 		this.preElements = [ 'pre' ];
 
 		/**
-		 * Tag names of DOM `Element`s which are considered block elements.
+		 * Elements which are considered block elements (and hence should be filled with a
+		 * {@link ~isBlockFiller block filler}).
+		 *
+		 * Whether an element is considered a block element also affects handling of trailing whitespaces.
+		 *
+		 * You can extend this array if you introduce support for block elements which are not yet recognized here.
 		 *
 		 * @readonly
 		 * @member {Array.<String>} module:engine/view/domconverter~DomConverter#blockElements
 		 */
-		this.blockElements = [ 'p', 'div', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6' ];
+		this.blockElements = [ 'p', 'div', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'li', 'dd', 'dt', 'figcaption' ];
+
+		/**
+		 * Block {@link module:engine/view/filler filler} creator, which is used to create all block fillers during the
+		 * view to DOM conversion and to recognize block fillers during the DOM to view conversion.
+		 *
+		 * @readonly
+		 * @private
+		 * @member {Function} module:engine/view/domconverter~DomConverter#_blockFiller
+		 */
+		this._blockFiller = this.blockFillerMode == 'br' ? BR_FILLER : NBSP_FILLER;
 
 		/**
 		 * DOM to View mapping.
@@ -255,7 +262,7 @@ export default class DomConverter {
 
 		for ( const childView of viewElement.getChildren() ) {
 			if ( fillerPositionOffset === offset ) {
-				yield this.blockFiller( domDocument );
+				yield this._blockFiller( domDocument );
 			}
 
 			yield this.viewToDom( childView, domDocument, options );
@@ -264,7 +271,7 @@ export default class DomConverter {
 		}
 
 		if ( fillerPositionOffset === offset ) {
-			yield this.blockFiller( domDocument );
+			yield this._blockFiller( domDocument );
 		}
 	}
 
@@ -371,7 +378,7 @@ export default class DomConverter {
 	 * or `null` if DOM node is a {@link module:engine/view/filler filler} or the given node is an empty text node.
 	 */
 	domToView( domNode, options = {} ) {
-		if ( isBlockFiller( domNode, this.blockFiller ) ) {
+		if ( this.isBlockFiller( domNode, this.blockFillerMode ) ) {
 			return null;
 		}
 
@@ -529,7 +536,7 @@ export default class DomConverter {
 	 * @returns {module:engine/view/position~Position} viewPosition View position.
 	 */
 	domPositionToView( domParent, domOffset ) {
-		if ( isBlockFiller( domParent, this.blockFiller ) ) {
+		if ( this.isBlockFiller( domParent, this.blockFillerMode ) ) {
 			return this.domPositionToView( domParent.parentNode, indexOf( domParent ) );
 		}
 
@@ -786,6 +793,23 @@ export default class DomConverter {
 		return node && node.nodeType == Node.COMMENT_NODE;
 	}
 
+	/**
+	 * Checks if the node is an instance of the block filler for this DOM converter.
+	 *
+	 *		const converter = new DomConverter( { blockFillerMode: 'br' } );
+	 *
+	 *		converter.isBlockFiller( BR_FILLER( document ) ); // true
+	 *		converter.isBlockFiller( NBSP_FILLER( document ) ); // false
+	 *
+	 * **Note:**: For the `'nbsp'` mode the method also checks context of a node so it cannot be a detached node.
+	 *
+	 * @param {Node} domNode DOM node to check.
+	 * @returns {Boolean} True if a node is considered a block filler for given mode.
+	 */
+	isBlockFiller( domNode ) {
+		return this.blockFillerMode == 'br' ? domNode.isEqualNode( BR_FILLER_REF ) : isNbspBlockFiller( domNode, this.blockElements );
+	}
+
 	/**
 	 * Returns `true` if given selection is a backward selection, that is, if it's `focus` is before `anchor`.
 	 *
@@ -1197,3 +1221,36 @@ function forEachDomNodeAncestor( node, callback ) {
 		node = node.parentNode;
 	}
 }
+
+// Checks if given node is a nbsp block filler.
+//
+// A &nbsp; is a block filler only if it is a single child of a block element.
+//
+// @param {Node} domNode DOM node.
+// @returns {Boolean}
+function isNbspBlockFiller( domNode, blockElements ) {
+	const isNBSP = isText( domNode ) && domNode.data == '\u00A0';
+
+	return isNBSP && hasBlockParent( domNode, blockElements ) && domNode.parentNode.childNodes.length === 1;
+}
+
+// Checks if domNode has block parent.
+//
+// @param {Node} domNode DOM node.
+// @returns {Boolean}
+function hasBlockParent( domNode, blockElements ) {
+	const parent = domNode.parentNode;
+
+	return parent && parent.tagName && blockElements.includes( parent.tagName.toLowerCase() );
+}
+
+/**
+ * Enum representing type of the block filler.
+ *
+ * Possible values:
+ *
+ * * `br` - for `<br>` block filler used in editing view,
+ * * `nbsp` - for `&nbsp;` block fillers used in the data.
+ *
+ * @typedef {String} module:engine/view/filler~BlockFillerMode
+ */

src/view/filler.js

@@ -3,8 +3,6 @@
  * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
  */
 
-/* globals window */
-
 import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard';
 import isText from '@ckeditor/ckeditor5-utils/src/dom/istext';
 
@@ -36,6 +34,15 @@ import isText from '@ckeditor/ckeditor5-utils/src/dom/istext';
  * @module engine/view/filler
  */
 
+/**
+ * Non-breaking space filler creator. This is a function which creates ` ` text node.
+ * It defines how the filler is created.
+ *
+ * @see module:engine/view/filler~BR_FILLER
+ * @function
+ */
+export const NBSP_FILLER = domDocument => domDocument.createTextNode( '\u00A0' );
+
 /**
  * `<br>` filler creator. This is a function which creates `<br data-cke-filler="true">` element.
  * It defines how the filler is created.
@@ -50,28 +57,23 @@ export const BR_FILLER = domDocument => {
 	return fillerBr;
 };
 
-/**
- * Non-breaking space filler creator. This is a function which creates `&nbsp;` text node.
- * It defines how the filler is created.
- *
- * @see module:engine/view/filler~BR_FILLER
- * @function
- */
-export const NBSP_FILLER = domDocument => domDocument.createTextNode( '\u00A0' );
-
 /**
  * Length of the {@link module:engine/view/filler~INLINE_FILLER INLINE_FILLER}.
  */
 export const INLINE_FILLER_LENGTH = 7;
 
 /**
- * Inline filler which is sequence of the zero width spaces.
+ * Inline filler which is a sequence of the zero width spaces.
  */
-export let INLINE_FILLER = '';
+export const INLINE_FILLER = ( () => {
+	let inlineFiller = '';
 
-for ( let i = 0; i < INLINE_FILLER_LENGTH; i++ ) {
-	INLINE_FILLER += '\u200b';
-}
+	for ( let i = 0; i < INLINE_FILLER_LENGTH; i++ ) {
+		inlineFiller += '\u200b';
+	}
+
+	return inlineFiller;
+} )(); // Usu IIF so the INLINE_FILLER appears as a constant in the docs.
 
 /**
  * Checks if the node is a text node which starts with the {@link module:engine/view/filler~INLINE_FILLER inline filler}.
@@ -119,30 +121,6 @@ export function getDataWithoutFiller( domText ) {
 	}
 }
 
-// Cache block fillers templates to improve performance.
-const templateBlockFillers = new WeakMap();
-
-/**
- * Checks if the node is an instance of the block filler of the given type.
- *
- *		const brFillerInstance = BR_FILLER( document );
- *		isBlockFiller( brFillerInstance, BR_FILLER ); // true
- *
- * @param {Node} domNode DOM node to check.
- * @param {Function} blockFiller Block filler creator.
- * @returns {Boolean} True if text node contains only {@link module:engine/view/filler~INLINE_FILLER inline filler}.
- */
-export function isBlockFiller( domNode, blockFiller ) {
-	let templateBlockFiller = templateBlockFillers.get( blockFiller );
-
-	if ( !templateBlockFiller ) {
-		templateBlockFiller = blockFiller( window.document );
-		templateBlockFillers.set( blockFiller, templateBlockFiller );
-	}
-
-	return domNode.isEqualNode( templateBlockFiller );
-}
-
 /**
  * Assign key observer which move cursor from the end of the inline filler to the beginning of it when
  * the left arrow is pressed, so the filler does not break navigation.

src/view/renderer.js

@@ -11,7 +11,7 @@
 
 import ViewText from './text';
 import ViewPosition from './position';
-import { INLINE_FILLER, INLINE_FILLER_LENGTH, startsWithFiller, isInlineFiller, isBlockFiller } from './filler';
+import { INLINE_FILLER, INLINE_FILLER_LENGTH, startsWithFiller, isInlineFiller } from './filler';
 
 import mix from '@ckeditor/ckeditor5-utils/src/mix';
 import diff from '@ckeditor/ckeditor5-utils/src/diff';
@@ -590,7 +590,7 @@ export default class Renderer {
 	_diffNodeLists( actualDomChildren, expectedDomChildren ) {
 		actualDomChildren = filterOutFakeSelectionContainer( actualDomChildren, this._fakeSelectionContainer );
 
-		return diff( actualDomChildren, expectedDomChildren, sameNodes.bind( null, this.domConverter.blockFiller ) );
+		return diff( actualDomChildren, expectedDomChildren, sameNodes.bind( null, this.domConverter ) );
 	}
 
 	/**
@@ -938,11 +938,11 @@ function areSimilar( node1, node2 ) {
 //		* Two block filler elements.
 //
 // @private
-// @param {Function} blockFiller Block filler creator function, see {@link module:engine/view/domconverter~DomConverter#blockFiller}.
+// @param {String} blockFillerMode Block filler mode, see {@link module:engine/view/domconverter~DomConverter#blockFillerMode}.
 // @param {Node} node1
 // @param {Node} node2
 // @returns {Boolean}
-function sameNodes( blockFiller, actualDomChild, expectedDomChild ) {
+function sameNodes( domConverter, actualDomChild, expectedDomChild ) {
 	// Elements.
 	if ( actualDomChild === expectedDomChild ) {
 		return true;
@@ -952,8 +952,8 @@ function sameNodes( blockFiller, actualDomChild, expectedDomChild ) {
 		return actualDomChild.data === expectedDomChild.data;
 	}
 	// Block fillers.
-	else if ( isBlockFiller( actualDomChild, blockFiller ) &&
-		isBlockFiller( expectedDomChild, blockFiller ) ) {
+	else if ( domConverter.isBlockFiller( actualDomChild ) &&
+		domConverter.isBlockFiller( expectedDomChild ) ) {
 		return true;
 	}