소스 검색

Merge pull request #1104 from ckeditor/t/1088

Fix: `DataController#insertContent()` and `DataController#deleteContent()` should strip disallowed attributes from text nodes. Closes #1088.
Szymon Cofalik 8 년 전
부모
커밋
75cf645da0

+ 43 - 4
packages/ckeditor5-engine/src/controller/deletecontent.js

@@ -68,6 +68,13 @@ export default function deleteContent( selection, batch, options = {} ) {
 	// want to override that behavior anyway.
 	if ( !options.leaveUnmerged ) {
 		mergeBranches( batch, startPos, endPos );
+
+		// We need to check and strip disallowed attributes in all nested nodes because after merge
+		// some attributes could end up in a path where are disallowed.
+		//
+		// e.g. bold is disallowed for <H1>
+		// <h1>Fo{o</h1><p>b}a<b>r</b><p> -> <h1>Fo{}a<b>r</b><h1> -> <h1>Fo{}ar<h1>.
+		removeDisallowedAttributes( startPos.parent.getChildren(), startPos, batch );
 	}
 
 	selection.setCollapsedAt( startPos );
@@ -208,9 +215,41 @@ function shouldEntireContentBeReplacedWithParagraph( schema, selection ) {
 		return false;
 	}
 
-	if ( !schema.check( { name: 'paragraph', inside: limitElement.name } ) ) {
-		return false;
-	}
+	return schema.check( { name: 'paragraph', inside: limitElement.name } );
+}
 
-	return true;
+// Gets a name under which we should check this node in the schema.
+//
+// @param {module:engine/model/node~Node} node The node.
+// @returns {String} node name.
+function getNodeSchemaName( node ) {
+	return node.is( 'text' ) ? '$text' : node.name;
+}
+
+// Creates AttributeDeltas that removes attributes that are disallowed by schema on given node and its children.
+//
+// @param {Array<module:engine/model/node~Node>} nodes Nodes that will be filtered.
+// @param {module:engine/model/schema~SchemaPath} inside Path inside which schema will be checked.
+// @param {module:engine/model/batch~Batch} batch Batch to which the deltas will be added.
+function removeDisallowedAttributes( nodes, inside, batch ) {
+	const schema = batch.document.schema;
+
+	for ( const node of nodes ) {
+		const name = getNodeSchemaName( node );
+
+		// When node with attributes is not allowed in current position.
+		if ( !schema.check( { name, inside, attributes: Array.from( node.getAttributeKeys() ) } ) ) {
+			// Let's remove attributes one by one.
+			// This should be improved to check all combination of attributes.
+			for ( const attribute of node.getAttributeKeys() ) {
+				if ( !schema.check( { name, inside, attributes: attribute } ) ) {
+					batch.removeAttribute( node, attribute );
+				}
+			}
+		}
+
+		if ( node.is( 'element' ) ) {
+			removeDisallowedAttributes( node.getChildren(), Position.createAt( node ), batch );
+		}
+	}
 }

+ 75 - 19
packages/ckeditor5-engine/src/controller/insertcontent.js

@@ -222,11 +222,17 @@ class Insertion {
 	 * @param {Object} context
 	 */
 	_handleDisallowedNode( node, context ) {
-		// Try inserting its children (strip the parent).
+		// If the node is an element, try inserting its children (strip the parent).
 		if ( node.is( 'element' ) ) {
 			this.handleNodes( node.getChildren(), context );
 		}
-		// Try autoparagraphing.
+		// If the node is a text and bare text is allowed in current position it means that the node
+		// contains disallowed attributes and we have to remove them.
+		else if ( this.schema.check( { name: '$text', inside: this.position } ) ) {
+			removeDisallowedAttributes( [ node ], this.position, this.schema );
+			this._handleNode( node, context );
+		}
+		// If text is not allowed, try autoparagraphing.
 		else {
 			this._tryAutoparagraphing( node, context );
 		}
@@ -237,7 +243,7 @@ class Insertion {
 	 */
 	_insert( node ) {
 		/* istanbul ignore if */
-		if ( !this._checkIsAllowed( node, [ this.position.parent ] ) ) {
+		if ( !this._checkIsAllowed( node, this.position ) ) {
 			// Algorithm's correctness check. We should never end up here but it's good to know that we did.
 			// Note that it would often be a silent issue if we insert node in a place where it's not allowed.
 			log.error(
@@ -256,7 +262,7 @@ class Insertion {
 		livePos.detach();
 
 		// The last inserted object should be selected because we can't put a collapsed selection after it.
-		if ( this._checkIsObject( node ) && !this.schema.check( { name: '$text', inside: [ this.position.parent ] } ) ) {
+		if ( this._checkIsObject( node ) && !this.schema.check( { name: '$text', inside: this.position } ) ) {
 			this.nodeToSelect = node;
 		} else {
 			this.nodeToSelect = null;
@@ -282,6 +288,11 @@ class Insertion {
 
 			this.batch.merge( mergePosLeft );
 
+			// We need to check and strip disallowed attributes in all nested nodes because after merge
+			// some attributes could end up in a path where are disallowed.
+			const parent = position.nodeBefore;
+			removeDisallowedAttributes( parent.getChildren(), Position.createAt( parent ), this.schema, this.batch );
+
 			this.position = Position.createFromPosition( position );
 			position.detach();
 		}
@@ -305,12 +316,22 @@ class Insertion {
 
 			this.batch.merge( mergePosRight );
 
+			// We need to check and strip disallowed attributes in all nested nodes because after merge
+			// some attributes could end up in a place where are disallowed.
+			removeDisallowedAttributes( position.parent.getChildren(), position, this.schema, this.batch );
+
 			this.position = Position.createFromPosition( position );
 			position.detach();
 		}
 
 		mergePosLeft.detach();
 		mergePosRight.detach();
+
+		// When there was no merge we need to check and strip disallowed attributes in all nested nodes of
+		// just inserted node because some attributes could end up in a place where are disallowed.
+		if ( !mergeLeft && !mergeRight ) {
+			removeDisallowedAttributes( node.getChildren(), Position.createAt( node ), this.schema, this.batch );
+		}
 	}
 
 	/**
@@ -325,10 +346,17 @@ class Insertion {
 		// Do not autoparagraph if the paragraph won't be allowed there,
 		// cause that would lead to an infinite loop. The paragraph would be rejected in
 		// the next _handleNode() call and we'd be here again.
-		if ( this._getAllowedIn( paragraph, this.position.parent ) && this._checkIsAllowed( node, [ paragraph ] ) ) {
-			paragraph.appendChildren( node );
+		if ( this._getAllowedIn( paragraph, this.position.parent ) ) {
+			// When node is a text and is disallowed by schema it means that contains disallowed attributes
+			// and we need to remove them.
+			if ( node.is( 'text' ) && !this._checkIsAllowed( node, [ paragraph ] ) ) {
+				removeDisallowedAttributes( [ node ], [ paragraph ], this.schema );
+			}
 
-			this._handleNode( paragraph, context );
+			if ( this._checkIsAllowed( node, [ paragraph ] ) ) {
+				paragraph.appendChildren( node );
+				this._handleNode( paragraph, context );
+			}
 		}
 	}
 
@@ -402,31 +430,59 @@ class Insertion {
 	 */
 	_checkIsAllowed( node, path ) {
 		return this.schema.check( {
-			name: this._getNodeSchemaName( node ),
+			name: getNodeSchemaName( node ),
 			attributes: Array.from( node.getAttributeKeys() ),
 			inside: path
 		} );
 	}
 
 	/**
-	 * Checks wether according to the schema this is an object type element.
+	 * Checks whether according to the schema this is an object type element.
 	 *
 	 * @param {module:engine/model/node~Node} node The node to check.
 	 */
 	_checkIsObject( node ) {
-		return this.schema.objects.has( this._getNodeSchemaName( node ) );
+		return this.schema.objects.has( getNodeSchemaName( node ) );
 	}
+}
 
-	/**
-	 * Gets a name under which we should check this node in the schema.
-	 *
-	 * @param {module:engine/model/node~Node} node The node.
-	 */
-	_getNodeSchemaName( node ) {
-		if ( node.is( 'text' ) ) {
-			return '$text';
+// Gets a name under which we should check this node in the schema.
+//
+// @param {module:engine/model/node~Node} node The node.
+// @returns {String} Node name.
+function getNodeSchemaName( node ) {
+	return node.is( 'text' ) ? '$text' : node.name;
+}
+
+// Removes disallowed by schema attributes from given nodes. When batch parameter is provided then
+// attributes will be removed by creating AttributeDeltas otherwise attributes will be removed
+// directly from provided nodes.
+//
+// @param {Array<module:engine/model/node~Node>} nodes Nodes that will be filtered.
+// @param {module:engine/model/schema~SchemaPath} inside Path inside which schema will be checked.
+// @param {module:engine/model/schema~Schema} schema Schema instance uses for element validation.
+// @param {module:engine/model/batch~Batch} [batch] Batch to which the deltas will be added.
+function removeDisallowedAttributes( nodes, inside, schema, batch ) {
+	for ( const node of nodes ) {
+		const name = getNodeSchemaName( node );
+
+		// When node with attributes is not allowed in current position.
+		if ( !schema.check( { name, inside, attributes: Array.from( node.getAttributeKeys() ) } ) ) {
+			// Let's remove attributes one by one.
+			// This should be improved to check all combination of attributes.
+			for ( const attribute of node.getAttributeKeys() ) {
+				if ( !schema.check( { name, inside, attributes: attribute } ) ) {
+					if ( batch ) {
+						batch.removeAttribute( node, attribute );
+					} else {
+						node.removeAttribute( attribute );
+					}
+				}
+			}
 		}
 
-		return node.name;
+		if ( node.is( 'element' ) ) {
+			removeDisallowedAttributes( node.getChildren(), Position.createAt( node ), schema, batch );
+		}
 	}
 }

+ 46 - 7
packages/ckeditor5-engine/tests/controller/deletecontent.js

@@ -155,9 +155,9 @@ describe( 'DataController', () => {
 
 				schema.registerItem( 'paragraph', '$block' );
 				schema.registerItem( 'heading1', '$block' );
+				schema.registerItem( 'image', '$inline' );
 				schema.registerItem( 'pchild' );
 				schema.registerItem( 'pparent' );
-				schema.registerItem( 'image', '$inline' );
 
 				schema.allow( { name: 'pchild', inside: 'paragraph' } );
 				schema.allow( { name: '$text', inside: 'pchild' } );
@@ -188,12 +188,6 @@ describe( 'DataController', () => {
 				{ leaveUnmerged: true }
 			);
 
-			test(
-				'merges second element into the first one (same name)',
-				'<paragraph>x</paragraph><paragraph>fo[o</paragraph><paragraph>b]ar</paragraph><paragraph>y</paragraph>',
-				'<paragraph>x</paragraph><paragraph>fo[]ar</paragraph><paragraph>y</paragraph>'
-			);
-
 			test(
 				'merges second element into the first one (different name)',
 				'<paragraph>x</paragraph><heading1>fo[o</heading1><paragraph>b]ar</paragraph><paragraph>y</paragraph>',
@@ -436,6 +430,51 @@ describe( 'DataController', () => {
 					'<paragraph>ba[]</paragraph><blockWidget><nestedEditable>oo</nestedEditable></blockWidget>'
 				);
 			} );
+
+			describe( 'filtering out', () => {
+				beforeEach( () => {
+					const schema = doc.schema;
+
+					schema.allow( { name: '$text', attributes: [ 'a', 'b' ], inside: 'paragraph' } );
+					schema.allow( { name: '$text', attributes: [ 'b', 'c' ], inside: 'pchild' } );
+					schema.allow( { name: 'pchild', inside: 'pchild' } );
+					schema.disallow( { name: '$text', attributes: [ 'c' ], inside: 'pchild pchild' } );
+				} );
+
+				test(
+					'filters out disallowed attributes after left merge',
+					'<paragraph>x<pchild>fo[o</pchild></paragraph><paragraph>y]<$text a="1" b="1">z</$text></paragraph>',
+					'<paragraph>x<pchild>fo[]<$text b="1">z</$text></pchild></paragraph>'
+				);
+
+				test(
+					'filters out disallowed attributes from nested nodes after left merge',
+					'<paragraph>' +
+						'x' +
+						'<pchild>fo[o</pchild>' +
+					'</paragraph>' +
+					'<paragraph>' +
+						'b]a<$text a="1" b="1">r</$text>' +
+						'<pchild>b<$text b="1" c="1">i</$text>z</pchild>' +
+						'y' +
+					'</paragraph>',
+
+					'<paragraph>' +
+						'x' +
+						'<pchild>' +
+							'fo[]a<$text b="1">r</$text>' +
+							'<pchild>b<$text b="1">i</$text>z</pchild>' +
+							'y' +
+						'</pchild>' +
+					'</paragraph>'
+				);
+
+				test(
+					'filters out disallowed attributes after right merge',
+					'<paragraph>fo[o</paragraph><paragraph><pchild>x<$text b="1" c="1">y]z</$text></pchild></paragraph>',
+					'<paragraph>fo[]<$text b="1">z</$text></paragraph>'
+				);
+			} );
 		} );
 
 		describe( 'in element selections scenarios', () => {

+ 77 - 1
packages/ckeditor5-engine/tests/controller/insertcontent.js

@@ -281,6 +281,17 @@ describe( 'DataController', () => {
 				expect( getData( doc ) ).to.equal( '<heading1>foxyz[]ar</heading1>' );
 			} );
 
+			it( 'not inserts autoparagraph when paragraph is disallowed at the current position', () => {
+				doc.schema.disallow( { name: 'paragraph', inside: '$root' } );
+				doc.schema.disallow( { name: 'heading2', inside: '$root' } );
+
+				const content = new Element( 'heading2', [], [ new Text( 'bar' ) ] );
+
+				setData( doc, '[<heading1>foo</heading1>]' );
+				insertContent( dataController, content, doc.selection );
+				expect( getData( doc ) ).to.equal( '[]' );
+			} );
+
 			describe( 'block to block handling', () => {
 				it( 'inserts one paragraph', () => {
 					setData( doc, '<paragraph>f[]oo</paragraph>' );
@@ -300,6 +311,12 @@ describe( 'DataController', () => {
 					expect( getData( doc ) ).to.equal( '<paragraph>xyz[]</paragraph>' );
 				} );
 
+				it( 'inserts one empty paragraph', () => {
+					setData( doc, '<paragraph>f[]oo</paragraph>' );
+					insertHelper( '<paragraph></paragraph>' );
+					expect( getData( doc ) ).to.equal( '<paragraph>f[]oo</paragraph>' );
+				} );
+
 				it( 'inserts one block into a fully selected content', () => {
 					setData( doc, '<heading1>[foo</heading1><paragraph>bar]</paragraph>' );
 					insertHelper( '<heading2>xyz</heading2>' );
@@ -576,8 +593,9 @@ describe( 'DataController', () => {
 				const schema = doc.schema;
 
 				schema.registerItem( 'paragraph', '$block' );
+				schema.registerItem( 'heading1', '$block' );
+				schema.registerItem( 'element', '$block' );
 
-				// Let's use table as an example of content which needs to be filtered out.
 				schema.registerItem( 'table' );
 				schema.registerItem( 'td' );
 				schema.registerItem( 'disallowedWidget' );
@@ -585,12 +603,22 @@ describe( 'DataController', () => {
 				schema.allow( { name: 'table', inside: '$clipboardHolder' } );
 				schema.allow( { name: 'td', inside: '$clipboardHolder' } );
 				schema.allow( { name: 'td', inside: 'table' } );
+				schema.allow( { name: 'element', inside: 'td' } );
 				schema.allow( { name: '$block', inside: 'td' } );
 				schema.allow( { name: '$text', inside: 'td' } );
+				schema.allow( { name: 'table', inside: 'element' } );
 
 				schema.allow( { name: 'disallowedWidget', inside: '$clipboardHolder' } );
 				schema.allow( { name: '$text', inside: 'disallowedWidget' } );
 				schema.objects.add( 'disallowedWidget' );
+
+				schema.allow( { name: 'element', inside: 'paragraph' } );
+				schema.allow( { name: 'element', inside: 'heading1' } );
+				schema.allow( { name: '$text', attributes: 'b', inside: 'paragraph' } );
+				schema.allow( { name: '$text', attributes: [ 'b' ], inside: 'paragraph element' } );
+				schema.allow( { name: '$text', attributes: [ 'a', 'b' ], inside: 'heading1 element' } );
+				schema.allow( { name: '$text', attributes: [ 'a', 'b' ], inside: 'td element' } );
+				schema.allow( { name: '$text', attributes: [ 'b' ], inside: 'element table td' } );
 			} );
 
 			it( 'filters out disallowed elements and leaves out the text', () => {
@@ -610,6 +638,54 @@ describe( 'DataController', () => {
 				insertHelper( '<disallowedWidget>xxx</disallowedWidget>' );
 				expect( getData( doc ) ).to.equal( '<paragraph>f[]oo</paragraph>' );
 			} );
+
+			it( 'filters out disallowed attributes when inserting text', () => {
+				setData( doc, '<paragraph>f[]oo</paragraph>' );
+				insertHelper( 'x<$text a="1" b="1">x</$text>xy<$text a="1">y</$text>y' );
+				expect( getData( doc ) ).to.equal( '<paragraph>fx<$text b="1">x</$text>xyyy[]oo</paragraph>' );
+			} );
+
+			it( 'filters out disallowed attributes when inserting nested elements', () => {
+				setData( doc, '<element>[]</element>' );
+				insertHelper( '<table><td>f<$text a="1" b="1" c="1">o</$text>o</td></table>' );
+				expect( getData( doc ) ).to.equal( '<element><table><td>f<$text b="1">o</$text>o</td></table>[]</element>' );
+			} );
+
+			it( 'filters out disallowed attributes when inserting text in disallowed elements', () => {
+				setData( doc, '<paragraph>f[]oo</paragraph>' );
+				insertHelper( '<table><td>x<$text a="1" b="1">x</$text>x</td><td>y<$text a="1">y</$text>y</td></table>' );
+				expect( getData( doc ) ).to.equal( '<paragraph>fx<$text b="1">x</$text>xyyy[]oo</paragraph>' );
+			} );
+
+			it( 'filters out disallowed attributes when merging #1', () => {
+				setData( doc, '<paragraph>[]foo</paragraph>' );
+				insertHelper( '<paragraph>x<$text a="1" b="1">x</$text>x</paragraph>' );
+				expect( getData( doc ) ).to.equal( '<paragraph>x<$text b="1">x</$text>x[]foo</paragraph>' );
+			} );
+
+			it( 'filters out disallowed attributes when merging #2', () => {
+				setData( doc, '<paragraph>f[]oo</paragraph>' );
+				insertHelper( '<paragraph>x<$text a="1" b="1">x</$text>x</paragraph>' );
+				expect( getData( doc ) ).to.equal( '<paragraph>fx<$text b="1">x</$text>x[]oo</paragraph>' );
+			} );
+
+			it( 'filters out disallowed attributes when merging #3', () => {
+				setData( doc, '<paragraph>foo[]</paragraph>' );
+				insertHelper( '<paragraph>x<$text a="1" b="1">x</$text>x</paragraph>' );
+				expect( getData( doc ) ).to.equal( '<paragraph>foox<$text b="1">x</$text>x[]</paragraph>' );
+			} );
+
+			it( 'filters out disallowed attributes from nested nodes when merging', () => {
+				setData( doc, '<paragraph>f[]oo</paragraph>' );
+				insertHelper( '<heading1>x<element>b<$text a="1" b="1">a</$text>r</element>x</heading1>' );
+				expect( getData( doc ) ).to.equal( '<paragraph>fx<element>b<$text b="1">a</$text>r</element>x[]oo</paragraph>' );
+			} );
+
+			it( 'filters out disallowed attributes when autoparagraphing', () => {
+				setData( doc, '<paragraph>f[]oo</paragraph>' );
+				insertHelper( '<paragraph>xxx</paragraph><$text a="1" b="1">yyy</$text>' );
+				expect( getData( doc ) ).to.equal( '<paragraph>fxxx</paragraph><paragraph><$text b="1">yyy[]</$text>oo</paragraph>' );
+			} );
 		} );
 	} );
 

+ 28 - 0
packages/ckeditor5-engine/tests/manual/tickets/1088/1.html

@@ -0,0 +1,28 @@
+<div style="padding: 20px;">
+	<div id="editor">
+		<h2>Heading 1 (disallowed: italic, link)</h2>
+		<p>This is a paragraph</p>
+		<h3>Heading 2 (disallowed: italic)</h3>
+		<p></p>
+		<blockquote>
+			<p>This is a paragraph in a blockQuote</p>
+			<p></p>
+		</blockquote>
+	</div>
+
+	<div>
+		<p><a href="https://ckeditor.com"><i>Paragraph with link and italic</i></a></p>
+
+		<ul>
+			<li><a href="https://ckeditor.com"><b>List item with link and bold</b></a></li>
+		</ul>
+
+		<h4><b><i>Heading 3 with bold and italic</i></b></h4>
+
+		<div><b>Just a text with bold</b></div>
+
+		<br>
+
+		<img src="sample.jpg" alt="Sample image" height="150">
+	</div>
+</div>

+ 32 - 0
packages/ckeditor5-engine/tests/manual/tickets/1088/1.js

@@ -0,0 +1,32 @@
+/**
+ * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+/* globals console, window, document */
+
+import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor';
+import ArticlePresets from '@ckeditor/ckeditor5-presets/src/article';
+
+ClassicEditor
+	.create( document.querySelector( '#editor' ), {
+		plugins: [ ArticlePresets ],
+		toolbar: [ 'headings', 'undo', 'redo' ],
+		image: {
+			toolbar: [ 'imageTextAlternative' ]
+		}
+	} )
+	.then( editor => {
+		window.editor = editor;
+
+		const schema = editor.document.schema;
+
+		schema.disallow( { name: '$text', attributes: [ 'linkHref', 'italic' ], inside: 'heading1' } );
+		schema.disallow( { name: '$text', attributes: [ 'italic' ], inside: 'heading2' } );
+		schema.disallow( { name: '$text', attributes: [ 'linkHref' ], inside: 'blockQuote listItem' } );
+		schema.disallow( { name: '$text', attributes: [ 'bold' ], inside: 'paragraph' } );
+		schema.disallow( { name: 'heading3', inside: '$root' } );
+	} )
+	.catch( err => {
+		console.error( err.stack );
+	} );

+ 31 - 0
packages/ckeditor5-engine/tests/manual/tickets/1088/1.md

@@ -0,0 +1,31 @@
+## Stripping disallowed attributes by `(insert|delete)Content` [#1088](https://github.com/ckeditor/ckeditor5-engine/issues/1088)
+
+### Simple scenario.
+
+1. Copy a paragraph with italic and link.
+2. Paste it to the Heading 1. Inserted text should be stripped
+3. Paste it to the Heading 2. Inserted text should be a link only.
+4. Paste it to paragraph. Inserted text should not be stripped.
+
+### Simple scenario (element).
+
+1. Copy image.
+2. Paste it to the editor. Image should be inserted with an alternative text "Sample image".
+
+### Nested nodes.
+
+1. Copy a list item with bold and link.
+2. Paste it into the empty block (directly to the root) . Inserted list item should be a bold link.
+2. Paste it into the empty block in BlockQuote. Inserted list item should be a bold only.
+
+### Auto paragraphing.
+
+1. Copy a text with bold.
+2. Select all content in the editor.
+3. Paste copied text. Inserted content should be a paragraph and should be stripped from bold.
+
+### Auto paragraphing (disallowed block).
+
+1. Copy Heading 3 with bold and italic.
+2. Select all content in the editor.
+3. Paste copied text. Inserted content should be a paragraph and should be stripped from bold.

BIN
packages/ckeditor5-engine/tests/manual/tickets/1088/sample.jpg