Merge pull request #10765 from ckeditor/ci/1106

Other (engine): Allowed unsafe view element attributes so they get rendered in the editing pipeline. Attribute names can be specified when creating elements using `DowncastWriter` (`DowncastWriter#createAttributeElement()`, `DowncastWriter#createContainerElement()`, etc.).

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

@@ -385,11 +385,9 @@ export default class DomConverter {
 				for ( const key of viewNode.getAttributeKeys() ) {
 					const value = viewNode.getAttribute( key );
 
-					if ( !this.shouldRenderAttribute( key, value ) ) {
-						continue;
+					if ( this.shouldRenderAttribute( key, value ) || viewNode.shouldRenderUnsafeAttribute( key ) ) {
+						domElement.setAttribute( key, value );
 					}
-
-					domElement.setAttribute( key, value );
 				}
 			}
 

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

@@ -186,6 +186,8 @@ export default class DowncastWriter {
 	 * @param {Object} [options] Element's options.
 	 * @param {Number} [options.priority] Element's {@link module:engine/view/attributeelement~AttributeElement#priority priority}.
 	 * @param {Number|String} [options.id] Element's {@link module:engine/view/attributeelement~AttributeElement#id id}.
+	 * @param {Array.<String>} [options.renderUnsafeAttributes] A list of attribute names that should be rendered in the editing
+	 * pipeline even though they would normally be filtered out by unsafe attribute detection mechanisms.
 	 * @returns {module:engine/view/attributeelement~AttributeElement} Created element.
 	 */
 	createAttributeElement( name, attributes, options = {} ) {
@@ -199,6 +201,10 @@ export default class DowncastWriter {
 			attributeElement._id = options.id;
 		}
 
+		if ( options.renderUnsafeAttributes ) {
+			attributeElement._unsafeAttributesToRender.push( ...options.renderUnsafeAttributes );
+		}
+
 		return attributeElement;
 	}
 
@@ -222,6 +228,8 @@ export default class DowncastWriter {
 	 * @param {Boolean} [options.isAllowedInsideAttributeElement=false] Whether an element is
 	 * {@link module:engine/view/element~Element#isAllowedInsideAttributeElement allowed inside an AttributeElement} and can be wrapped
 	 * with {@link module:engine/view/attributeelement~AttributeElement} by {@link module:engine/view/downcastwriter~DowncastWriter}.
+	 * @param {Array.<String>} [options.renderUnsafeAttributes] A list of attribute names that should be rendered in the editing
+	 * pipeline even though they would normally be filtered out by unsafe attribute detection mechanisms.
 	 * @returns {module:engine/view/containerelement~ContainerElement} Created element.
 	 */
 	createContainerElement( name, attributes, options = {} ) {
@@ -231,6 +239,10 @@ export default class DowncastWriter {
 			containerElement._isAllowedInsideAttributeElement = options.isAllowedInsideAttributeElement;
 		}
 
+		if ( options.renderUnsafeAttributes ) {
+			containerElement._unsafeAttributesToRender.push( ...options.renderUnsafeAttributes );
+		}
+
 		return containerElement;
 	}
 
@@ -245,12 +257,19 @@ export default class DowncastWriter {
 	 *
 	 * @param {String} name Name of the element.
 	 * @param {Object} [attributes] Elements attributes.
+	 * @param {Object} [options] Element's options.
+	 * @param {Array.<String>} [options.renderUnsafeAttributes] A list of attribute names that should be rendered in the editing
+	 * pipeline even though they would normally be filtered out by unsafe attribute detection mechanisms.
 	 * @returns {module:engine/view/editableelement~EditableElement} Created element.
 	 */
-	createEditableElement( name, attributes ) {
+	createEditableElement( name, attributes, options = {} ) {
 		const editableElement = new EditableElement( this.document, name, attributes );
 		editableElement._document = this.document;
 
+		if ( options.renderUnsafeAttributes ) {
+			editableElement._unsafeAttributesToRender.push( ...options.renderUnsafeAttributes );
+		}
+
 		return editableElement;
 	}
 
@@ -266,6 +285,8 @@ export default class DowncastWriter {
 	 * @param {Boolean} [options.isAllowedInsideAttributeElement=true] Whether an element is
 	 * {@link module:engine/view/element~Element#isAllowedInsideAttributeElement allowed inside an AttributeElement} and can be wrapped
 	 * with {@link module:engine/view/attributeelement~AttributeElement} by {@link module:engine/view/downcastwriter~DowncastWriter}.
+	 * @param {Array.<String>} [options.renderUnsafeAttributes] A list of attribute names that should be rendered in the editing
+	 * pipeline even though they would normally be filtered out by unsafe attribute detection mechanisms.
 	 * @returns {module:engine/view/emptyelement~EmptyElement} Created element.
 	 */
 	createEmptyElement( name, attributes, options = {} ) {
@@ -275,6 +296,10 @@ export default class DowncastWriter {
 			emptyElement._isAllowedInsideAttributeElement = options.isAllowedInsideAttributeElement;
 		}
 
+		if ( options.renderUnsafeAttributes ) {
+			emptyElement._unsafeAttributesToRender.push( ...options.renderUnsafeAttributes );
+		}
+
 		return emptyElement;
 	}
 
@@ -347,6 +372,8 @@ export default class DowncastWriter {
 	 * @param {Boolean} [options.isAllowedInsideAttributeElement=true] Whether an element is
 	 * {@link module:engine/view/element~Element#isAllowedInsideAttributeElement allowed inside an AttributeElement} and can be wrapped
 	 * with {@link module:engine/view/attributeelement~AttributeElement} by {@link module:engine/view/downcastwriter~DowncastWriter}.
+	 * @param {Array.<String>} [options.renderUnsafeAttributes] A list of attribute names that should be rendered in the editing
+	 * pipeline even though they would normally be filtered out by unsafe attribute detection mechanisms.
 	 * @returns {module:engine/view/rawelement~RawElement} The created element.
 	 */
 	createRawElement( name, attributes, renderFunction, options = {} ) {
@@ -358,6 +385,10 @@ export default class DowncastWriter {
 			rawElement._isAllowedInsideAttributeElement = options.isAllowedInsideAttributeElement;
 		}
 
+		if ( options.renderUnsafeAttributes ) {
+			rawElement._unsafeAttributesToRender.push( ...options.renderUnsafeAttributes );
+		}
+
 		return rawElement;
 	}
 

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

@@ -138,6 +138,21 @@ export default class Element extends Node {
 		 * @member {Boolean}
 		 */
 		this._isAllowedInsideAttributeElement = false;
+
+		/**
+		 * A list of attribute names that should be rendered in the editing pipeline even though filtering mechanisms
+		 * implemented in the {@link module:engine/view/domconverter~DomConverter} (for instance,
+		 * {@link module:engine/view/domconverter~DomConverter#shouldRenderAttribute}) would filter them out.
+		 *
+		 * These attributes can be specified as an option when the element is created by
+		 * the {@link module:engine/view/downcastwriter~DowncastWriter}. To check whether an unsafe an attribute should
+		 * be permitted, use the {@link #shouldRenderUnsafeAttribute} method.
+		 *
+		 * @private
+		 * @readonly
+		 * @member {Array.<String>}
+		 */
+		this._unsafeAttributesToRender = [];
 	}
 
 	/**
@@ -572,6 +587,19 @@ export default class Element extends Node {
 			( attributes == '' ? '' : ` ${ attributes }` );
 	}
 
+	/**
+	 * Decides whether an unsafe attribute is whitelisted and should be rendered in the editing pipeline even though filtering mechanisms
+	 * like {@link module:engine/view/domconverter~DomConverter#shouldRenderAttribute} say it should not.
+	 *
+	 * Unsafe attribute names can be specified when creating an element via {@link module:engine/view/downcastwriter~DowncastWriter}.
+	 *
+	 * @param {String} attributeName The name of the attribute to be checked.
+	 * @returns {Boolean}
+	 */
+	shouldRenderUnsafeAttribute( attributeName ) {
+		return this._unsafeAttributesToRender.includes( attributeName );
+	}
+
 	/**
 	 * Clones provided element.
 	 *

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

@@ -557,10 +557,10 @@ export default class Renderer {
 		for ( const key of viewAttrKeys ) {
 			const value = viewElement.getAttribute( key );
 
-			if ( !this.domConverter.shouldRenderAttribute( key, value ) ) {
-				domElement.removeAttribute( key );
-			} else {
+			if ( this.domConverter.shouldRenderAttribute( key, value ) || viewElement.shouldRenderUnsafeAttribute( key ) ) {
 				domElement.setAttribute( key, value );
+			} else {
+				domElement.removeAttribute( key );
 			}
 		}
 

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

@@ -457,7 +457,7 @@ describe( 'DomConverter', () => {
 			expect( converter.shouldRenderAttribute( 'anything', '<script>something</script>' ) ).to.be.true;
 		} );
 
-		it( 'should reject certain attributes', () => {
+		it( 'should reject certain attributes in the editing pipeline', () => {
 			expect( converter.shouldRenderAttribute( 'some-attribute', 'anything' ) ).to.be.true;
 			expect( converter.shouldRenderAttribute( 'data-custom-attribute', 'anything' ) ).to.be.true;
 			expect( converter.shouldRenderAttribute( 'class', 'anything' ) ).to.be.true;

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

@@ -15,6 +15,7 @@ import ViewEmptyElement from '../../../src/view/emptyelement';
 import DomConverter from '../../../src/view/domconverter';
 import ViewDocumentFragment from '../../../src/view/documentfragment';
 import ViewDocument from '../../../src/view/document';
+import DowncastWriter from '../../../src/view/downcastwriter';
 import { INLINE_FILLER, INLINE_FILLER_LENGTH, BR_FILLER, NBSP_FILLER, MARKED_NBSP_FILLER } from '../../../src/view/filler';
 
 import { parse } from '../../../src/dev-utils/view';
@@ -272,6 +273,72 @@ describe( 'DomConverter', () => {
 				expect( domP.childNodes[ 0 ].getAttribute( 'data-ck-hidden' ) ).to.equal( 'script' );
 				expect( domP.childNodes[ 1 ].data ).to.equal( 'foo' );
 			} );
+
+			describe( 'unsafe attribute names that were declaratively permitted', () => {
+				let writer;
+
+				beforeEach( () => {
+					writer = new DowncastWriter( viewDocument );
+					converter = new DomConverter( viewDocument, {
+						renderingMode: 'editing'
+					} );
+
+					converter.experimentalRenderingMode = true;
+				} );
+
+				it( 'should not be rejected when set on attribute elements', () => {
+					const viewElement = writer.createAttributeElement( 'span', {
+						onclick: 'foo',
+						onkeydown: 'bar'
+					}, { renderUnsafeAttributes: [ 'onclick' ] } );
+
+					expect( converter.viewToDom( viewElement, document ).outerHTML ).to.equal( '<span onclick="foo"></span>' );
+				} );
+
+				it( 'should not be rejected when set on container elements', () => {
+					const viewElement = writer.createContainerElement( 'p', {
+						onclick: 'foo',
+						onkeydown: 'bar'
+					}, { renderUnsafeAttributes: [ 'onclick' ] } );
+
+					viewElement.getFillerOffset = () => null;
+
+					expect( converter.viewToDom( viewElement, document ).outerHTML ).to.equal( '<p onclick="foo"></p>' );
+				} );
+
+				it( 'should not be rejected when set on editable elements', () => {
+					const viewElement = writer.createEditableElement( 'div', {
+						onclick: 'foo',
+						onkeydown: 'bar'
+					}, { renderUnsafeAttributes: [ 'onclick' ] } );
+
+					viewElement.getFillerOffset = () => null;
+
+					expect( converter.viewToDom( viewElement, document ).outerHTML ).to.equal( '<div onclick="foo"></div>' );
+				} );
+
+				it( 'should not be rejected when set on empty elements', () => {
+					const viewElement = writer.createEmptyElement( 'img', {
+						onclick: 'foo',
+						onkeydown: 'bar'
+					}, { renderUnsafeAttributes: [ 'onclick' ] } );
+
+					expect( converter.viewToDom( viewElement, document ).outerHTML ).to.equal( '<img onclick="foo">' );
+				} );
+
+				it( 'should not be rejected when set on raw elements', () => {
+					const viewElement = writer.createRawElement( 'p', {
+						onclick: 'foo',
+						onkeydown: 'bar'
+					}, function( domElement ) {
+						domElement.innerHTML = 'foo';
+					}, {
+						renderUnsafeAttributes: [ 'onclick' ]
+					} );
+
+					expect( converter.viewToDom( viewElement, document ).outerHTML ).to.equal( '<p onclick="foo">foo</p>' );
+				} );
+			} );
 		} );
 
 		describe( 'it should convert spaces to &nbsp;', () => {

packages/ckeditor5-engine/tests/view/downcastwriter/writer.js

@@ -113,13 +113,18 @@ describe( 'DowncastWriter', () => {
 		} );
 
 		it( 'should allow to pass additional options', () => {
-			const element = writer.createAttributeElement( 'foo', attributes, { priority: 99, id: 'bar' } );
+			const element = writer.createAttributeElement( 'foo', attributes, {
+				priority: 99,
+				id: 'bar',
+				renderUnsafeAttributes: [ 'baz' ]
+			} );
 
 			expect( element.is( 'attributeElement' ) ).to.be.true;
 			expect( element.name ).to.equal( 'foo' );
 			expect( element.isAllowedInsideAttributeElement ).to.be.false;
 			expect( element.priority ).to.equal( 99 );
 			expect( element.id ).to.equal( 'bar' );
+			expect( element.shouldRenderUnsafeAttribute( 'baz' ) ).to.be.true;
 			assertElementAttributes( element, attributes );
 		} );
 
@@ -141,11 +146,15 @@ describe( 'DowncastWriter', () => {
 		} );
 
 		it( 'should allow to pass additional options', () => {
-			const element = writer.createContainerElement( 'foo', attributes, { isAllowedInsideAttributeElement: true } );
+			const element = writer.createContainerElement( 'foo', attributes, {
+				isAllowedInsideAttributeElement: true,
+				renderUnsafeAttributes: [ 'baz' ]
+			} );
 
 			expect( element.is( 'containerElement' ) ).to.be.true;
 			expect( element.name ).to.equal( 'foo' );
 			expect( element.isAllowedInsideAttributeElement ).to.be.true;
+			expect( element.shouldRenderUnsafeAttribute( 'baz' ) ).to.be.true;
 			assertElementAttributes( element, attributes );
 		} );
 	} );
@@ -159,6 +168,14 @@ describe( 'DowncastWriter', () => {
 			expect( element.isAllowedInsideAttributeElement ).to.be.false;
 			assertElementAttributes( element, attributes );
 		} );
+
+		it( 'should allow to pass additional options', () => {
+			const element = writer.createEditableElement( 'foo', attributes, {
+				renderUnsafeAttributes: [ 'baz' ]
+			} );
+
+			expect( element.shouldRenderUnsafeAttribute( 'baz' ) ).to.be.true;
+		} );
 	} );
 
 	describe( 'createEmptyElement()', () => {
@@ -172,11 +189,15 @@ describe( 'DowncastWriter', () => {
 		} );
 
 		it( 'should allow to pass additional options', () => {
-			const element = writer.createEmptyElement( 'foo', attributes, { isAllowedInsideAttributeElement: false } );
+			const element = writer.createEmptyElement( 'foo', attributes, {
+				isAllowedInsideAttributeElement: false,
+				renderUnsafeAttributes: [ 'baz' ]
+			} );
 
 			expect( element.is( 'emptyElement' ) ).to.be.true;
 			expect( element.name ).to.equal( 'foo' );
 			expect( element.isAllowedInsideAttributeElement ).to.be.false;
+			expect( element.shouldRenderUnsafeAttribute( 'baz' ) ).to.be.true;
 			assertElementAttributes( element, attributes );
 		} );
 	} );
@@ -247,11 +268,15 @@ describe( 'DowncastWriter', () => {
 
 		it( 'should allow to pass additional options', () => {
 			const renderFn = function() {};
-			const element = writer.createRawElement( 'foo', attributes, renderFn, { isAllowedInsideAttributeElement: false } );
+			const element = writer.createRawElement( 'foo', attributes, renderFn, {
+				isAllowedInsideAttributeElement: false,
+				renderUnsafeAttributes: [ 'baz' ]
+			} );
 
 			expect( element.is( 'rawElement' ) ).to.be.true;
 			expect( element.name ).to.equal( 'foo' );
 			expect( element.isAllowedInsideAttributeElement ).to.be.false;
+			expect( element.shouldRenderUnsafeAttribute( 'baz' ) ).to.be.true;
 			assertElementAttributes( element, attributes );
 		} );
 	} );

packages/ckeditor5-engine/tests/view/element.js

@@ -1136,4 +1136,26 @@ describe( 'Element', () => {
 			);
 		} );
 	} );
+
+	describe( 'shouldRenderUnsafeAttribute()', () => {
+		let element;
+
+		beforeEach( () => {
+			element = new Element( document, 'p' );
+		} );
+
+		it( 'should return true if the atribute name is among unsafe attributes', () => {
+			element._unsafeAttributesToRender = [ 'foo', 'bar', 'baz' ];
+
+			expect( element.shouldRenderUnsafeAttribute( 'foo' ) ).to.be.true;
+			expect( element.shouldRenderUnsafeAttribute( 'bar' ) ).to.be.true;
+			expect( element.shouldRenderUnsafeAttribute( 'baz' ) ).to.be.true;
+		} );
+
+		it( 'should return false if the atribute name is not among unsafe attributes', () => {
+			element._unsafeAttributesToRender = [ 'foo', 'bar', 'baz' ];
+
+			expect( element.shouldRenderUnsafeAttribute( 'abc' ) ).to.be.false;
+		} );
+	} );
 } );

packages/ckeditor5-engine/tests/view/renderer.js

@@ -4031,6 +4031,25 @@ describe( 'Renderer', () => {
 				expect( normalizeHtml( domRoot.innerHTML ) ).to.equal( '<p>foo</p>' );
 			} );
 
+			it( 'should remove attributes that can affect editing pipeline unless permitted when the element was created', () => {
+				view.change( writer => {
+					const containerElement = writer.createContainerElement( 'p', {
+						onclick: 'foo',
+						onkeydown: 'bar'
+					}, {
+						renderUnsafeAttributes: [ 'onclick' ]
+					} );
+
+					writer.insert( writer.createPositionAt( containerElement, 'start' ), writer.createText( 'baz' ) );
+					writer.insert( writer.createPositionAt( view.document.getRoot(), 'start' ), containerElement );
+				} );
+
+				view.forceRender();
+
+				expect( getViewData( view ) ).to.equal( '<p onclick="foo" onkeydown="bar">baz</p>' );
+				expect( normalizeHtml( domRoot.innerHTML ) ).to.equal( '<p onclick="foo">baz</p>' );
+			} );
+
 			it( 'should remove attributes from the View that can are not present in the DOM', () => {
 				setViewData( view,
 					'<container:p>' +