diff --git a/src/controller/deletecontent.js b/src/controller/deletecontent.js index b260e7392..7a9a91e32 100644 --- a/src/controller/deletecontent.js +++ b/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

+ //

Fo{o

b}ar

->

Fo{}ar

->

Fo{}ar

. + 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} 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 ); + } + } } diff --git a/src/controller/insertcontent.js b/src/controller/insertcontent.js index c2443a8b1..c8e24150e 100644 --- a/src/controller/insertcontent.js +++ b/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} 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 ); + } } } diff --git a/tests/controller/deletecontent.js b/tests/controller/deletecontent.js index bb21eab97..78a537a4f 100644 --- a/tests/controller/deletecontent.js +++ b/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)', - 'xfo[ob]ary', - 'xfo[]ary' - ); - test( 'merges second element into the first one (different name)', 'xfo[ob]ary', @@ -436,6 +430,51 @@ describe( 'DataController', () => { 'ba[]oo' ); } ); + + 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', + 'xfo[oy]<$text a="1" b="1">z', + 'xfo[]<$text b="1">z' + ); + + test( + 'filters out disallowed attributes from nested nodes after left merge', + '' + + 'x' + + 'fo[o' + + '' + + '' + + 'b]a<$text a="1" b="1">r' + + 'b<$text b="1" c="1">iz' + + 'y' + + '', + + '' + + 'x' + + '' + + 'fo[]a<$text b="1">r' + + 'b<$text b="1">iz' + + 'y' + + '' + + '' + ); + + test( + 'filters out disallowed attributes after right merge', + 'fo[ox<$text b="1" c="1">y]z', + 'fo[]<$text b="1">z' + ); + } ); } ); describe( 'in element selections scenarios', () => { diff --git a/tests/controller/insertcontent.js b/tests/controller/insertcontent.js index 7c44e8b9a..06ad07961 100644 --- a/tests/controller/insertcontent.js +++ b/tests/controller/insertcontent.js @@ -281,6 +281,17 @@ describe( 'DataController', () => { expect( getData( doc ) ).to.equal( 'foxyz[]ar' ); } ); + 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, '[foo]' ); + insertContent( dataController, content, doc.selection ); + expect( getData( doc ) ).to.equal( '[]' ); + } ); + describe( 'block to block handling', () => { it( 'inserts one paragraph', () => { setData( doc, 'f[]oo' ); @@ -300,6 +311,12 @@ describe( 'DataController', () => { expect( getData( doc ) ).to.equal( 'xyz[]' ); } ); + it( 'inserts one empty paragraph', () => { + setData( doc, 'f[]oo' ); + insertHelper( '' ); + expect( getData( doc ) ).to.equal( 'f[]oo' ); + } ); + it( 'inserts one block into a fully selected content', () => { setData( doc, '[foobar]' ); insertHelper( 'xyz' ); @@ -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( 'xxx' ); expect( getData( doc ) ).to.equal( 'f[]oo' ); } ); + + it( 'filters out disallowed attributes when inserting text', () => { + setData( doc, 'f[]oo' ); + insertHelper( 'x<$text a="1" b="1">xxy<$text a="1">yy' ); + expect( getData( doc ) ).to.equal( 'fx<$text b="1">xxyyy[]oo' ); + } ); + + it( 'filters out disallowed attributes when inserting nested elements', () => { + setData( doc, '[]' ); + insertHelper( '
f<$text a="1" b="1" c="1">oo
' ); + expect( getData( doc ) ).to.equal( '
f<$text b="1">oo
[]
' ); + } ); + + it( 'filters out disallowed attributes when inserting text in disallowed elements', () => { + setData( doc, 'f[]oo' ); + insertHelper( '
x<$text a="1" b="1">xxy<$text a="1">yy
' ); + expect( getData( doc ) ).to.equal( 'fx<$text b="1">xxyyy[]oo' ); + } ); + + it( 'filters out disallowed attributes when merging #1', () => { + setData( doc, '[]foo' ); + insertHelper( 'x<$text a="1" b="1">xx' ); + expect( getData( doc ) ).to.equal( 'x<$text b="1">xx[]foo' ); + } ); + + it( 'filters out disallowed attributes when merging #2', () => { + setData( doc, 'f[]oo' ); + insertHelper( 'x<$text a="1" b="1">xx' ); + expect( getData( doc ) ).to.equal( 'fx<$text b="1">xx[]oo' ); + } ); + + it( 'filters out disallowed attributes when merging #3', () => { + setData( doc, 'foo[]' ); + insertHelper( 'x<$text a="1" b="1">xx' ); + expect( getData( doc ) ).to.equal( 'foox<$text b="1">xx[]' ); + } ); + + it( 'filters out disallowed attributes from nested nodes when merging', () => { + setData( doc, 'f[]oo' ); + insertHelper( 'xb<$text a="1" b="1">arx' ); + expect( getData( doc ) ).to.equal( 'fxb<$text b="1">arx[]oo' ); + } ); + + it( 'filters out disallowed attributes when autoparagraphing', () => { + setData( doc, 'f[]oo' ); + insertHelper( 'xxx<$text a="1" b="1">yyy' ); + expect( getData( doc ) ).to.equal( 'fxxx<$text b="1">yyy[]oo' ); + } ); } ); } ); diff --git a/tests/manual/tickets/1088/1.html b/tests/manual/tickets/1088/1.html new file mode 100644 index 000000000..efcb091f2 --- /dev/null +++ b/tests/manual/tickets/1088/1.html @@ -0,0 +1,28 @@ +
+
+

Heading 1 (disallowed: italic, link)

+

This is a paragraph

+

Heading 2 (disallowed: italic)

+

+
+

This is a paragraph in a blockQuote

+

+
+
+ +
+

Paragraph with link and italic

+ + + +

Heading 3 with bold and italic

+ +
Just a text with bold
+ +
+ + Sample image +
+
diff --git a/tests/manual/tickets/1088/1.js b/tests/manual/tickets/1088/1.js new file mode 100644 index 000000000..3f333da74 --- /dev/null +++ b/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 ); + } ); diff --git a/tests/manual/tickets/1088/1.md b/tests/manual/tickets/1088/1.md new file mode 100644 index 000000000..0acf53214 --- /dev/null +++ b/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. diff --git a/tests/manual/tickets/1088/sample.jpg b/tests/manual/tickets/1088/sample.jpg new file mode 100644 index 000000000..b77d07e7b Binary files /dev/null and b/tests/manual/tickets/1088/sample.jpg differ