Fix for: editor.insertHtml and editor.insertElement pollutes editable with empty spans

CHANGES.md

@@ -22,6 +22,7 @@ Fixed Issues:
 * [#2235](https://github.com/ckeditor/ckeditor-dev/issues/2235): Fixed: [Image](https://ckeditor.com/cke4/addon/image) in table cell has an empty URL field when edited from context menu opened by right-click when [Table Selection](https://ckeditor.com/cke4/addon/tableselection) plugin is in use.
 * [#3120](https://github.com/ckeditor/ckeditor-dev/issues/3120): [IE8] Fixed: [`CKEDITOR.tools.extend`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_tool.html#method-extend) function doesn't work with [`DontEnum`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Properties) object property attribute.
 * [#3098](https://github.com/ckeditor/ckeditor-dev/issues/3098): Fixed: Unit pickers for table cell width and height in [Table Tools](https://ckeditor.com/cke4/addon/tabletools) plugin have different width.
+* [#2813](https://github.com/ckeditor/ckeditor-dev/issues/2813): Fixed: Editor HTML insertion methods ([`editor.insertHtml`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_editor.html#method-insertHtml), [`editor.insertHtmlIntoRange`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_editor.html#method-insertHtmlIntoRange), [`editor.insertElement`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_editor.html#method-insertElement) and [`editor.insertElementIntoRange`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_editor.html#method-insertElementIntoRange)) pollutes editable with empty spans.
 
 API Changes:
 

core/editable.js

@@ -460,12 +460,22 @@
 							( dtd = CKEDITOR.dtd[ current.getName() ] ) &&
 							!( dtd && dtd[ elementName ] ) ) {
 						// Split up inline elements.
-						if ( current.getName() in CKEDITOR.dtd.span )
-							range.splitElement( current );
+						if ( current.getName() in CKEDITOR.dtd.span ) {
+							var endNode = range.splitElement( current ),
+								bookmark = range.createBookmark();
+
+							// Remove empty element created after splitting (#2813).
+							// The range.splitElement() method splits the given element in two and places the selection
+							// in-between in such way that <div>F^oo</div> becomes <div>F</div>^<div>oo</div>.
+							// Then removeEmptyInlineElement() method removes any of these elements if they are empty.
+							removeEmptyInlineElement( current );
+							removeEmptyInlineElement( endNode );
+
+							range.moveToBookmark( bookmark );
 
 						// If we're in an empty block which indicate a new paragraph,
-						// simply replace it with the inserting block.(https://dev.ckeditor.com/ticket/3664)
-						else if ( range.checkStartOfBlock() && range.checkEndOfBlock() ) {
+						// simply replace it with the inserting block (https://dev.ckeditor.com/ticket/3664).
+						} else if ( range.checkStartOfBlock() && range.checkEndOfBlock() ) {
 							range.setStartBefore( current );
 							range.collapse( true );
 							current.remove();
@@ -1664,7 +1674,8 @@
 		function prepareRangeToDataInsertion( that ) {
 			var range = that.range,
 				mergeCandidates = that.mergeCandidates,
-				node, marker, path, startPath, endPath, previous, bm;
+				isHtml = that.type === 'html',
+				node, marker, path, startPath, endPath, previous, bm, endNode;
 
 			// If range starts in inline element then insert a marker, so empty
 			// inline elements won't be removed while range.deleteContents
@@ -1717,14 +1728,20 @@
 			// Split inline elements so HTML will be inserted with its own styles.
 			path = range.startPath();
 			if ( ( node = path.contains( isInline, false, 1 ) ) ) {
-				range.splitElement( node );
+				endNode = range.splitElement( node );
 				that.inlineStylesRoot = node;
 				that.inlineStylesPeak = path.lastElement;
 			}
 
 			// Record inline merging candidates for later cleanup in place.
 			bm = range.createBookmark();
 
+			// When called by insertHtml remove empty element created after splitting (#2813).
+			if ( isHtml ) {
+				removeEmptyInlineElement( node );
+				removeEmptyInlineElement( endNode );
+			}
+
 			// 1. Inline siblings.
 			node = bm.startNode.getPrevious( isNotEmpty );
 			node && checkIfElement( node ) && isInline( node ) && mergeCandidates.push( node );
@@ -1733,8 +1750,9 @@
 
 			// 2. Inline parents.
 			node = bm.startNode;
-			while ( ( node = node.getParent() ) && isInline( node ) )
+			while ( ( node = node.getParent() ) && isInline( node ) ) {
 				mergeCandidates.push( node );
+			}
 
 			range.moveToBookmark( bm );
 		}
@@ -2286,6 +2304,12 @@
 		return insert;
 	} )();
 
+	function removeEmptyInlineElement( element ) {
+		if ( element && element.isEmptyInlineRemoveable() ) {
+			element.remove();
+		}
+	}
+
 	function afterInsert( editable ) {
 		var editor = editable.editor;
 

tests/core/editable/insertion.js

@@ -13,18 +13,18 @@
 		}
 	};
 
-	bender.test( {
+	var tests = {
 		testInsertElement: function() {
 			var editor = this.editor;
 
 			// When editor has focus.
 			var ins = CKEDITOR.dom.element.createFromHtml( '<strong>baz</strong>', editor.document );
-			bender.tools.setHtmlWithSelection( editor, 'foo^bar' );
+			tools.setHtmlWithSelection( editor, 'foo^bar' );
 			editor.insertElement( ins );
 			assert.areSame( 'foo<strong>baz</strong>bar', tools.compatHtml( editor.getData() ), 'insert element with existing selection, editor focused' );
 
 			// When editor loose focus.
-			bender.tools.setHtmlWithSelection( editor, 'foo^bar' );
+			tools.setHtmlWithSelection( editor, 'foo^bar' );
 			ins = CKEDITOR.dom.element.createFromHtml( '<strong>baz</strong>', editor.document );
 			doc.getById( 'text_input' ).focus();
 			editor.insertElement( ins );
@@ -35,12 +35,12 @@
 			var editor = this.editor;
 
 			// When editor has focus.
-			bender.tools.setHtmlWithSelection( editor, 'foo^bar' );
+			tools.setHtmlWithSelection( editor, 'foo^bar' );
 			editor.insertHtml( 'baz' );
 			assert.areSame( 'foobazbar', tools.compatHtml( editor.getData() ), 'insert html with existing selection, editor focused' );
 
 			// When editor loose focus.
-			bender.tools.setHtmlWithSelection( editor, 'foo^bar' );
+			tools.setHtmlWithSelection( editor, 'foo^bar' );
 			doc.getById( 'text_input' ).focus();
 			editor.insertHtml( 'baz' );
 			assert.areSame( 'foobazbar', tools.compatHtml( editor.getData() ), 'insert html with existing selection, editor blurred' );
@@ -50,12 +50,12 @@
 			var editor = this.editor;
 
 			// When editor has focus.
-			bender.tools.setHtmlWithSelection( editor, 'foo^bar' );
+			tools.setHtmlWithSelection( editor, 'foo^bar' );
 			editor.insertText( 'baz' );
 			assert.areSame( 'foobazbar', tools.compatHtml( editor.getData() ), 'insert text with existing selection, editor focused' );
 
 			// When editor loose focus.
-			bender.tools.setHtmlWithSelection( editor, 'foo^bar' );
+			tools.setHtmlWithSelection( editor, 'foo^bar' );
 			doc.getById( 'text_input' ).focus();
 			editor.insertText( 'baz' );
 			assert.areSame( 'foobazbar', tools.compatHtml( editor.getData() ), 'insert text with existing selection, editor blurred' );
@@ -159,7 +159,118 @@
 			editor.insertHtml( '<p>bam</p>', 'html', range );
 
 			assert.isInnerHtmlMatching( '<p id="p1">foo</p><p>bam^@</p><p>bar@</p>',
-				bender.tools.selection.getWithHtml( editor ), { compareSelection: true, normalizeSelection: true } );
+				tools.selection.getWithHtml( editor ), { compareSelection: true, normalizeSelection: true } );
 		}
-	} );
+	};
+
+	// (#2813)
+	addInsertionTests( [
+		{
+			name: 'when selection starts at the beginning of span',
+			initial: '<span>{foo}bar</span>',
+			data: '<div><div>div</div></div>',
+			expected: '<div><div>div</div></div><span>bar</span>'
+		}, {
+			name: 'when selection ends at the end of span',
+			initial: '<span>foo{bar}</span>',
+			data: '<div><div>div</div></div>',
+			expected: '<span>foo</span><div><div>div</div></div>'
+		}, {
+			name: 'when selection covers span',
+			initial: '<span>{foobar}</span>',
+			data: '<div><div>div</div></div>',
+			expected: '<div><div>div</div></div>'
+		}, {
+			name: 'when selection is followed by space',
+			initial: '<span>foo{bar}&nbsp;</span>',
+			data: '<div><div>div</div></div>',
+			expected: '<span>foo</span><div><div>div</div></div><span>&nbsp;</span>',
+			ignore: CKEDITOR.env.ie && !CKEDITOR.env.edge // (#3061)
+		}, {
+			name: 'when selection is preceded by space',
+			initial: '<span>&nbsp;{foo}bar</span>',
+			data: '<div><div>div</div></div>',
+			expected: '<span>&nbsp;</span><div><div>div</div></div><span>bar</span>',
+			ignore: CKEDITOR.env.ie && !CKEDITOR.env.edge // (#3061)
+		}, {
+			name: 'when empty element is inserted at the end of span',
+			initial: '<span>foo{bar}</span>',
+			data: '<div></div>',
+			expected: '<span>foo</span><div></div>',
+			ignore: CKEDITOR.env.ie && CKEDITOR.env.version < 9 // IE8 fills empty element with `&nbsp;`, so we can skip this test.
+		}, {
+			name: 'when selection covers nested span',
+			initial: '<p><span>{foobar}</span></p>',
+			data: '<div><div>div</div></div>',
+			expected: '<div><div>div</div></div>'
+		}, {
+			name: 'when selection covers span with empty spans on boundaries',
+			initial: '<span></span><span>{foobar}</span><span></span>',
+			data: '<div><div>div</div></div>',
+			expected: '<span></span><div><div>div</div></div><span></span>'
+		}
+	] );
+
+	bender.test( tests );
+
+	function addInsertionTests( insertionTests ) {
+		CKEDITOR.tools.array.forEach( insertionTests, addInsertionTest );
+	}
+
+	function addInsertionTest( options ) {
+		var methods = [ 'insertHtml', 'insertHtmlIntoRange', 'insertElement', 'insertElementIntoRange', 'insertText' ];
+
+		CKEDITOR.tools.array.forEach( methods, function( methodName ) {
+			tests[ 'test ' + methodName + ' ' + options.name ] = function() {
+				if ( options.ignore ) {
+					assert.ignore();
+				}
+
+				var editor = this.editorBot.editor;
+
+				tools.selection.setWithHtml( editor, options.initial );
+
+				if ( methodName === 'insertText' ) {
+					assertInsertText( editor, options.initial.replace( /{.*}/, 'text' ), 'text' );
+				} else {
+					assertInsertionMethod( editor, options.expected, methodName, options.data );
+				}
+			};
+		} );
+	}
+
+	function assertInsertionMethod( editor, expected, methodName, html ) {
+		var useRange, useElement, data;
+
+		switch ( methodName ) {
+			case 'insertHtmlIntoRange':
+				useRange = true;
+				break;
+			case 'insertElement':
+				useElement = true;
+				break;
+			case 'insertElementIntoRange':
+				useRange = true;
+				useElement = true;
+		}
+
+		data = useElement ? CKEDITOR.dom.element.createFromHtml( html ) : html;
+
+		if ( useRange ) {
+			editor.editable()[ methodName ]( data, editor.getSelection().getRanges()[ 0 ] );
+		} else {
+			editor[ methodName ]( data );
+		}
+
+		assert.areSame( expected, normalizeHtml( editor.editable().getHtml() ) );
+	}
+
+	function assertInsertText( editor, expected, string ) {
+		editor.insertText( string );
+		assert.areSame( expected, normalizeHtml( editor.editable().getHtml() ) );
+	}
+
+	function normalizeHtml( html ) {
+		return html.toLowerCase().replace( /(<p>(<br>|&nbsp;)<\/p>|<p><\/p>$|<br>|\s|\u200B)/g, '' );
+	}
 } )();

tests/core/editable/manual/insert.html

@@ -0,0 +1,76 @@
+<textarea id="editor">
+	<p>
+		<span>Foo bar</span>
+	</p>
+	<ul>
+		<li>
+			<span>Foo bar</span>
+			<ul>
+				<li>
+					<span>Foo bar</span>
+				</li>
+			</ul>
+		</li>
+	</ul>
+	<div>
+		<table border="1" cellspacing="1" cellpadding="1" style="width: 500px;">
+			<tbody>
+			<tr>
+				<td>
+					<span>Foo bar</span>
+				</td>
+			</tr>
+			<tr>
+				<td>
+					<span>Foo bar</span>
+				</td>
+			</tr>
+			</tbody>
+		</table>
+	</div>
+</textarea>
+
+<p id="controls">
+	<button data-button="insert-html">Insert html</button>
+	<button data-button="insert-html-range">Insert html into range</button>
+	<button data-button="insert-element">Insert element</button>
+	<button data-button="insert-element-range">Insert element into range</button>
+	<button data-button="insert-text">Insert text</button>
+	<button data-button="reset">Reset editor</button>
+</p>
+
+<script>
+	var initialData = CKEDITOR.document.findOne( '#editor' ).getValue(),
+		editor = CKEDITOR.replace( 'editor', {
+			allowedContent: true,
+			height: 500
+		} ),
+		elementHtml = '<div><div>div</div></div>';
+
+	CKEDITOR.addCss(
+		'span{padding:10px;border:solid 1px red;}' +
+		'li,div{margin:20px 0;}' +
+		'td{padding:20px 0;}'
+	);
+
+	CKEDITOR.document.findOne( '#controls' ).on( 'click', function( evt ) {
+		var editable = editor.editable(),
+			range = editor.getSelection().getRanges()[ 0 ],
+			element = CKEDITOR.dom.element.createFromHtml( elementHtml );
+
+		switch ( evt.data.getTarget().getAttribute( 'data-button' ) ) {
+			case 'insert-html':
+				return editor.insertHtml( elementHtml );
+			case 'insert-html-range':
+				return editable.insertHtmlIntoRange( elementHtml, range );
+			case 'insert-element':
+				return editor.insertElement( element );
+			case 'insert-element-range':
+				return editable.insertElementIntoRange( element, range );
+			case 'insert-text':
+				return editor.insertText( 'text' );
+			case 'reset':
+				editor.setData( initialData );
+		}
+	} );
+</script>

tests/core/editable/manual/insert.md

@@ -0,0 +1,23 @@
+@bender-tags: bug, 4.12.0, 2813
+@bender-ui: collapsed
+@bender-ckeditor-plugins: wysiwygarea, toolbar, undo, sourcearea, list, table
+
+## For each insert button and for each element
+
+1. After each case use `reset editor` button.
+
+1. Select the word `bar`.
+
+1. Press button.
+
+## Expected
+
+- `Insert text` button: `text` replaces selected word inside the rectangle.
+
+- Any other button: `div` replaces selected word outside the rectangle.
+
+## Unexpected
+
+Clicked button causes the creation of an additional rectangle.
+
+**NOTE:** Because of [#3042](https://github.com/ckeditor/ckeditor-dev/issues/3042) two nested divs are used for insertion.