Merge pull request #10237 from ckeditor/ck/8822-html-comment

Feature (html-support): Introduced the HTML comment plugin. Closes #8822.

Other (engine): Undeprecated the `elementToMarker` upcast helper. 

Feature (engine): Introduced new option `skipComments` in `DomConverter#domToView()` (`false` by default) to make it possible to decide whether HTML comments should be removed from the data.

Internal (paste-from-office): Removed all HTML comments from pasted data.

Fix (table): Fixed model mappings in table cell if a paragraph is bound to its parent.

docs/builds/guides/integration/features-html-output-overview.md

@@ -972,6 +972,21 @@ The data used to generate the following tables comes from the package metadata.
 				</p>
 			</td>
 		</tr>
+		<tr>
+			<td class="plugin">
+				<p>
+					<a href="../../../features/general-html-support.html#html-comments">HTML comment</a>
+				</p>
+				<p>
+					<a href="../../../api/module_html-support_htmlcomment-HtmlComment.html"><code>HtmlComment</code></a>
+				</p>
+			</td>
+			<td class="html-output">
+				<p>
+					The plugin can output the HTML comments that exist in the editor data.
+				</p>
+			</td>
+		</tr>
 	</tbody>
 </table>
 <h3 id="ckeditor5-image"><code>ckeditor5-image</code></h3>

packages/ckeditor5-engine/src/conversion/downcasthelpers.js

@@ -272,8 +272,11 @@ export default class DowncastHelpers extends ConversionHelpers {
 	/**
 	 * Model marker to view element conversion helper.
 	 *
-	 * **Note**: This method should be used only for editing downcast. For data downcast, use
-	 * {@link #markerToData `#markerToData()`} that produces valid HTML data.
+	 * **Note**: This method should be used mainly for editing downcast and it is recommended
+	 * to use {@link #markerToData `#markerToData()`} helper instead.
+	 *
+	 * This helper may produce invalid HTML code (e.g. a span between table cells).
+	 * It should be used only when you are sure that the produced HTML will be semantically correct.
 	 *
 	 * This conversion results in creating a view element on the boundaries of the converted marker. If the converted marker
 	 * is collapsed, only one element is created. For example, model marker set like this: `<paragraph>F[oo b]ar</paragraph>`

packages/ckeditor5-engine/src/conversion/upcasthelpers.js

@@ -7,7 +7,6 @@ import Matcher from '../view/matcher';
 import ConversionHelpers from './conversionhelpers';
 
 import { cloneDeep } from 'lodash-es';
-import { logWarning } from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 
 import priorities from '@ckeditor/ckeditor5-utils/src/priorities';
 import { isParagraphable, wrapInParagraph } from '../model/utils/autoparagraphing';
@@ -294,13 +293,17 @@ export default class UpcastHelpers extends ConversionHelpers {
 	/**
 	 * View element to model marker conversion helper.
 	 *
-	 * **Note**: This method was deprecated. Please use {@link #dataToMarker} instead.
-	 *
 	 * This conversion results in creating a model marker. For example, if the marker was stored in a view as an element:
 	 * `<p>Fo<span data-marker="comment" data-comment-id="7"></span>o</p><p>B<span data-marker="comment" data-comment-id="7"></span>ar</p>`,
 	 * after the conversion is done, the marker will be available in
 	 * {@link module:engine/model/model~Model#markers model document markers}.
 	 *
+	 * **Note**: When this helper is used in the data upcast in combination with
+	 * {@link module:engine/conversion/downcasthelpers~DowncastHelpers#markerToData `#markerToData()`} in the data downcast,
+	 * then invalid HTML code (e.g. a span between table cells) may be produced by the latter converter.
+	 *
+	 * In most of the cases, the {@link #dataToMarker} should be used instead.
+	 *
 	 *		editor.conversion.for( 'upcast' ).elementToMarker( {
 	 *			view: 'marker-search',
 	 *			model: 'search'
@@ -330,7 +333,6 @@ export default class UpcastHelpers extends ConversionHelpers {
 	 * See {@link module:engine/conversion/conversion~Conversion#for `conversion.for()`} to learn how to add a converter
 	 * to the conversion process.
 	 *
-	 * @deprecated
 	 * @method #elementToMarker
 	 * @param {Object} config Conversion configuration.
 	 * @param {module:engine/view/matcher~MatcherPattern} config.view Pattern matching all view elements which should be converted.
@@ -340,15 +342,6 @@ export default class UpcastHelpers extends ConversionHelpers {
 	 * @returns {module:engine/conversion/upcasthelpers~UpcastHelpers}
 	 */
 	elementToMarker( config ) {
-		/**
-		 * The {@link module:engine/conversion/upcasthelpers~UpcastHelpers#elementToMarker `UpcastHelpers#elementToMarker()`}
-		 * method was deprecated and will be removed in the near future.
-		 * Please use {@link module:engine/conversion/upcasthelpers~UpcastHelpers#dataToMarker `UpcastHelpers#dataToMarker()`} instead.
-		 *
-		 * @error upcast-helpers-element-to-marker-deprecated
-		 */
-		logWarning( 'upcast-helpers-element-to-marker-deprecated' );
-
 		return this.add( upcastElementToMarker( config ) );
 	}
 

packages/ckeditor5-engine/src/view/domconverter.js

@@ -11,6 +11,7 @@
 
 import ViewText from './text';
 import ViewElement from './element';
+import ViewUIElement from './uielement';
 import ViewPosition from './position';
 import ViewRange from './range';
 import ViewSelection from './selection';
@@ -252,8 +253,12 @@ export default class DomConverter {
 					this.bindDocumentFragments( domElement, viewNode );
 				}
 			} else if ( viewNode.is( 'uiElement' ) ) {
-				// UIElement has its own render() method (see #799).
-				domElement = viewNode.render( domDocument );
+				if ( viewNode.name === '$comment' ) {
+					domElement = domDocument.createComment( viewNode.getCustomProperty( '$rawContent' ) );
+				} else {
+					// UIElement has its own render() method (see #799).
+					domElement = viewNode.render( domDocument );
+				}
 
 				if ( options.bind ) {
 					this.bindElements( domElement, viewNode );
@@ -421,7 +426,9 @@ export default class DomConverter {
 	 * @param {Object} [options] Conversion options.
 	 * @param {Boolean} [options.bind=false] Determines whether new elements will be bound.
 	 * @param {Boolean} [options.withChildren=true] If `true`, node's and document fragment's children will be converted too.
-	 * @param {Boolean} [options.keepOriginalCase=false] If `false`, node's tag name will be converter to lower case.
+	 * @param {Boolean} [options.keepOriginalCase=false] If `false`, node's tag name will be converted to lower case.
+	 * @param {Boolean} [options.skipComments=false] If `false`, comment nodes will be converted to `$comment`
+	 * {@link module:engine/view/uielement~UIElement view UI elements}.
 	 * @returns {module:engine/view/node~Node|module:engine/view/documentfragment~DocumentFragment|null} Converted node or document fragment
 	 * or `null` if DOM node is a {@link module:engine/view/filler filler} or the given node is an empty text node.
 	 */
@@ -437,6 +444,10 @@ export default class DomConverter {
 			return hostElement;
 		}
 
+		if ( this.isComment( domNode ) && options.skipComments ) {
+			return null;
+		}
+
 		if ( isText( domNode ) ) {
 			if ( isInlineFiller( domNode ) ) {
 				return null;
@@ -445,8 +456,6 @@ export default class DomConverter {
 
 				return textData === '' ? null : new ViewText( this.document, textData );
 			}
-		} else if ( this.isComment( domNode ) ) {
-			return null;
 		} else {
 			if ( this.mapDomToView( domNode ) ) {
 				return this.mapDomToView( domNode );
@@ -463,8 +472,7 @@ export default class DomConverter {
 				}
 			} else {
 				// Create view element.
-				const viewName = options.keepOriginalCase ? domNode.tagName : domNode.tagName.toLowerCase();
-				viewElement = new ViewElement( this.document, viewName );
+				viewElement = this._createViewElement( domNode, options );
 
 				if ( options.bind ) {
 					this.bindElements( domNode, viewElement );
@@ -473,13 +481,18 @@ export default class DomConverter {
 				// Copy element's attributes.
 				const attrs = domNode.attributes;
 
-				for ( let i = attrs.length - 1; i >= 0; i-- ) {
-					viewElement._setAttribute( attrs[ i ].name, attrs[ i ].value );
+				if ( attrs ) {
+					for ( let i = attrs.length - 1; i >= 0; i-- ) {
+						viewElement._setAttribute( attrs[ i ].name, attrs[ i ].value );
+					}
 				}
 
 				// Treat this element's content as a raw data if it was registered as such.
-				if ( options.withChildren !== false && this._rawContentElementMatcher.match( viewElement ) ) {
-					viewElement._setCustomProperty( '$rawContent', domNode.innerHTML );
+				// Comment node is also treated as an element with raw data.
+				if ( this._isViewElementWithRawContent( viewElement, options ) || this.isComment( domNode ) ) {
+					const rawContent = this.isComment( domNode ) ? domNode.data : domNode.innerHTML;
+
+					viewElement._setCustomProperty( '$rawContent', rawContent );
 
 					// Store a DOM node to prevent left trimming of the following text node.
 					this._encounteredRawContentDomNodes.add( domNode );
@@ -1330,6 +1343,36 @@ export default class DomConverter {
 	_isInlineObjectElement( node ) {
 		return this.isElement( node ) && this.inlineObjectElements.includes( node.tagName.toLowerCase() );
 	}
+
+	/**
+	 * Creates view element basing on the node type.
+	 *
+	 * @private
+	 * @param {Node} node DOM node to check.
+	 * @param {Object} options Conversion options. See {@link module:engine/view/domconverter~DomConverter#domToView} options parameter.
+	 * @returns {Element}
+	 */
+	_createViewElement( node, options ) {
+		if ( this.isComment( node ) ) {
+			return new ViewUIElement( this.document, '$comment' );
+		}
+
+		const viewName = options.keepOriginalCase ? node.tagName : node.tagName.toLowerCase();
+
+		return new ViewElement( this.document, viewName );
+	}
+
+	/**
+	 * Checks if view element's content should be treated as a raw data.
+	 *
+	 * @private
+	 * @param {Element} viewElement View element to check.
+	 * @param {Object} options Conversion options. See {@link module:engine/view/domconverter~DomConverter#domToView} options parameter.
+	 * @returns {Boolean}
+	 */
+	_isViewElementWithRawContent( viewElement, options ) {
+		return options.withChildren !== false && this._rawContentElementMatcher.match( viewElement );
+	}
 }
 
 // Helper function.

packages/ckeditor5-engine/tests/conversion/upcasthelpers.js

@@ -33,8 +33,6 @@ import Writer from '../../src/model/writer';
 
 import toArray from '@ckeditor/ckeditor5-utils/src/toarray';
 
-/* globals console */
-
 describe( 'UpcastHelpers', () => {
 	let upcastDispatcher, model, schema, upcastHelpers, viewDocument;
 
@@ -751,16 +749,6 @@ describe( 'UpcastHelpers', () => {
 	} );
 
 	describe( 'elementToMarker()', () => {
-		beforeEach( () => {
-			// Silence warning about deprecated method.
-			// This whole suite will be removed when the deprecated method is removed.
-			sinon.stub( console, 'warn' );
-		} );
-
-		afterEach( () => {
-			console.warn.restore();
-		} );
-
 		it( 'should be chainable', () => {
 			expect( upcastHelpers.elementToMarker( { view: 'marker-search', model: 'search' } ) ).to.equal( upcastHelpers );
 		} );

packages/ckeditor5-engine/tests/view/domconverter/dom-to-view.js

@@ -6,6 +6,7 @@
 /* globals document */
 
 import ViewElement from '../../../src/view/element';
+import ViewUIElement from '../../../src/view/uielement';
 import ViewDocument from '../../../src/view/document';
 import ViewDocumentSelection from '../../../src/view/documentselection';
 import DomConverter from '../../../src/view/domconverter';
@@ -171,10 +172,38 @@ describe( 'DomConverter', () => {
 			expect( converter.domToView( textNode ) ).to.be.null;
 		} );
 
-		it( 'should return null for a comment', () => {
-			const comment = document.createComment( 'abc' );
+		it( 'should create UIElement for comment', () => {
+			const domComment = document.createComment( 'abc' );
 
-			expect( converter.domToView( comment ) ).to.be.null;
+			const viewComment = converter.domToView( domComment );
+
+			expect( viewComment ).to.be.an.instanceof( ViewUIElement );
+			expect( viewComment.name ).to.equal( '$comment' );
+
+			expect( viewComment.getCustomProperty( '$rawContent' ) ).to.equal( 'abc' );
+
+			expect( converter.mapViewToDom( viewComment ) ).to.not.equal( domComment );
+		} );
+
+		it( 'should create UIElement for comment and bind elements', () => {
+			const domComment = document.createComment( 'abc' );
+
+			const viewComment = converter.domToView( domComment, { bind: true } );
+
+			expect( viewComment ).to.be.an.instanceof( ViewUIElement );
+			expect( viewComment.name ).to.equal( '$comment' );
+
+			expect( viewComment.getCustomProperty( '$rawContent' ) ).to.equal( 'abc' );
+
+			expect( converter.mapViewToDom( viewComment ) ).to.equal( domComment );
+		} );
+
+		it( 'should return `null` for a comment when the `skipComments` option is set to `true`', () => {
+			const domComment = document.createComment( 'abc' );
+
+			const viewComment = converter.domToView( domComment, { skipComments: true } );
+
+			expect( viewComment ).to.be.null;
 		} );
 
 		describe( 'it should clear whitespaces', () => {

packages/ckeditor5-engine/tests/view/domconverter/rawcontent.js

@@ -52,9 +52,10 @@ describe( 'DOMConverter raw content matcher', () => {
 				expect( viewDiv.getChild( 0 ).name ).to.equal( 'img' );
 				expect( viewDiv.getChild( 1 ).getCustomProperty( '$rawContent' ) ).to.equal( '<!-- foo --><img>bar\n123' );
 				expect( viewDiv.getChild( 2 ).getCustomProperty( '$rawContent' ) ).to.be.undefined;
-				expect( viewDiv.getChild( 2 ).childCount ).to.equal( 2 );
-				expect( viewDiv.getChild( 2 ).getChild( 0 ).name ).to.equal( 'img' );
-				expect( viewDiv.getChild( 2 ).getChild( 1 ).data ).to.equal( 'bar 123' );
+				expect( viewDiv.getChild( 2 ).childCount ).to.equal( 3 );
+				expect( viewDiv.getChild( 2 ).getChild( 0 ).getCustomProperty( '$rawContent' ) ).to.equal( 'foo' );
+				expect( viewDiv.getChild( 2 ).getChild( 1 ).name ).to.equal( 'img' );
+				expect( viewDiv.getChild( 2 ).getChild( 2 ).data ).to.equal( 'bar 123' );
 				expect( viewDiv.getChild( 3 ).data ).to.equal( 'abc' );
 			} );
 
@@ -92,9 +93,10 @@ describe( 'DOMConverter raw content matcher', () => {
 				expect( viewDiv.getChild( 0 ).name ).to.equal( 'img' );
 				expect( viewDiv.getChild( 1 ).getCustomProperty( '$rawContent' ) ).to.equal( '<!-- foo --><img>bar\n123' );
 				expect( viewDiv.getChild( 2 ).getCustomProperty( '$rawContent' ) ).to.be.undefined;
-				expect( viewDiv.getChild( 2 ).childCount ).to.equal( 2 );
-				expect( viewDiv.getChild( 2 ).getChild( 0 ).name ).to.equal( 'img' );
-				expect( viewDiv.getChild( 2 ).getChild( 1 ).data ).to.equal( 'bar 123' );
+				expect( viewDiv.getChild( 2 ).childCount ).to.equal( 3 );
+				expect( viewDiv.getChild( 2 ).getChild( 0 ).getCustomProperty( '$rawContent' ) ).to.equal( 'foo' );
+				expect( viewDiv.getChild( 2 ).getChild( 1 ).name ).to.equal( 'img' );
+				expect( viewDiv.getChild( 2 ).getChild( 2 ).data ).to.equal( 'bar 123' );
 				expect( viewDiv.getChild( 3 ).data ).to.equal( 'abc' );
 			} );
 
@@ -148,9 +150,10 @@ describe( 'DOMConverter raw content matcher', () => {
 					'<!-- foo --><img><span data-foo="bar">nested span</span>bar\n123'
 				);
 				expect( viewDiv.getChild( 2 ).getCustomProperty( '$rawContent' ) ).to.be.undefined;
-				expect( viewDiv.getChild( 2 ).childCount ).to.equal( 2 );
-				expect( viewDiv.getChild( 2 ).getChild( 0 ).name ).to.equal( 'img' );
-				expect( viewDiv.getChild( 2 ).getChild( 1 ).data ).to.equal( 'bar 123' );
+				expect( viewDiv.getChild( 2 ).childCount ).to.equal( 3 );
+				expect( viewDiv.getChild( 2 ).getChild( 0 ).getCustomProperty( '$rawContent' ) ).to.equal( 'foo' );
+				expect( viewDiv.getChild( 2 ).getChild( 1 ).name ).to.equal( 'img' );
+				expect( viewDiv.getChild( 2 ).getChild( 2 ).data ).to.equal( 'bar 123' );
 				expect( viewDiv.getChild( 3 ).getCustomProperty( '$rawContent' ) ).to.equal( 'some span' );
 				expect( viewDiv.getChild( 4 ).name ).to.equal( 'span' );
 				expect( viewDiv.getChild( 4 ).getChild( 0 ).data ).to.equal( 'other span' );

packages/ckeditor5-engine/tests/view/domconverter/view-to-dom.js

@@ -3,10 +3,11 @@
  * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
  */
 
-/* globals Range, DocumentFragment, HTMLElement, document, Text */
+/* globals Range, DocumentFragment, HTMLElement, Comment, document, Text */
 
 import ViewText from '../../../src/view/text';
 import ViewElement from '../../../src/view/element';
+import ViewUIElement from '../../../src/view/uielement';
 import ViewPosition from '../../../src/view/position';
 import ViewContainerElement from '../../../src/view/containerelement';
 import ViewAttributeElement from '../../../src/view/attributeelement';
@@ -186,6 +187,34 @@ describe( 'DomConverter', () => {
 			expect( domSvg.createSVGRect ).to.be.a( 'function' );
 		} );
 
+		it( 'should create a DOM comment node from a view `$comment` UIElement', () => {
+			const viewComment = new ViewUIElement( viewDocument, '$comment' );
+
+			viewComment._setCustomProperty( '$rawContent', 'foo' );
+
+			const domComment = converter.viewToDom( viewComment, document );
+
+			expect( domComment ).to.be.an.instanceof( Comment );
+			expect( domComment.nodeName ).to.equal( '#comment' );
+			expect( domComment.data ).to.equal( 'foo' );
+
+			expect( converter.mapDomToView( domComment ) ).to.not.equal( viewComment );
+		} );
+
+		it( 'should create a DOM comment node from a view `$comment` UIElement and bind them', () => {
+			const viewComment = new ViewUIElement( viewDocument, '$comment' );
+
+			viewComment._setCustomProperty( '$rawContent', 'foo' );
+
+			const domComment = converter.viewToDom( viewComment, document, { bind: true } );
+
+			expect( domComment ).to.be.an.instanceof( Comment );
+			expect( domComment.nodeName ).to.equal( '#comment' );
+			expect( domComment.data ).to.equal( 'foo' );
+
+			expect( converter.mapDomToView( domComment ) ).to.equal( viewComment );
+		} );
+
 		describe( 'it should convert spaces to  ', () => {
 			it( 'at the beginning of each container element', () => {
 				const viewDiv = new ViewContainerElement( viewDocument, 'div', null, [

packages/ckeditor5-html-support/ckeditor5-metadata.json

@@ -36,6 +36,18 @@
 			"className": "DataSchema",
 			"description": "Holds representation of the extended HTML document type definitions. Part of General HTML Support feature.",
 			"path": "src/dataschema.js"
+		},
+		{
+			"name": "HTML comment",
+			"className": "HtmlComment",
+			"description": "Preserves the HTML comments in the editor data.",
+			"docs": "features/general-html-support.html#html-comments",
+			"path": "src/htmlcomment.js",
+			"htmlOutput": [
+				{
+					"_comment": "The plugin can output HTML comments that were added from the editor inital data or by the plugin API."
+				}
+			]
 		}
 	]
 }