From d523d6d2020b96b153d15e97b6c51cda7b55ca88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Thu, 5 Sep 2019 00:06:45 +0200 Subject: [PATCH 1/4] Improved filtering out disallowed attributes on child elements. --- src/model/schema.js | 2 +- tests/model/schema.js | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/model/schema.js b/src/model/schema.js index 311a43d41..ccf35f6b6 100644 --- a/src/model/schema.js +++ b/src/model/schema.js @@ -732,7 +732,7 @@ export default class Schema { * @param {module:engine/model/writer~Writer} writer */ removeDisallowedAttributes( nodes, writer ) { - for ( const node of nodes ) { + for ( const node of Array.from( nodes ) ) { for ( const attribute of node.getAttributeKeys() ) { if ( !this.checkAttribute( node, attribute ) ) { writer.removeAttribute( attribute, node ); diff --git a/tests/model/schema.js b/tests/model/schema.js index ce52e35ef..295e3023a 100644 --- a/tests/model/schema.js +++ b/tests/model/schema.js @@ -1823,6 +1823,33 @@ describe( 'Schema', () => { ); } ); } ); + + it( 'should filter out all attributes from descendants that are merged while clearing', () => { + schema.addAttributeCheck( ( ctx, attributeName ) => { + // Disallow `a` in div>$text. + if ( ctx.endsWith( 'div $text' ) && attributeName == 'a' ) { + return false; + } + } ); + + const a = new Text( 'a', { a: 1 } ); + const b = new Text( 'b', { a: 2 } ); + const c = new Text( 'c', { a: 3 } ); + const div = new Element( 'div', [], [ a, b, c ] ); + + root._appendChild( [ div ] ); + + model.change( writer => { + schema.removeDisallowedAttributes( [ div ], writer ); + + expect( writer.batch.operations ).to.length( 3 ); + expect( writer.batch.operations[ 0 ] ).to.instanceof( AttributeOperation ); + expect( writer.batch.operations[ 1 ] ).to.instanceof( AttributeOperation ); + expect( writer.batch.operations[ 2 ] ).to.instanceof( AttributeOperation ); + + expect( getData( model, { withoutSelection: true } ) ).to.equal( '
abc
' ); + } ); + } ); } ); describe( 'definitions compilation', () => { From 2a033f5cfdd4f0cb54d8745d83530dc4d8d8736a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Thu, 5 Sep 2019 08:27:16 +0200 Subject: [PATCH 2/4] Merging nodes can create really nasty situations. --- tests/model/schema.js | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/model/schema.js b/tests/model/schema.js index 295e3023a..22ac587e6 100644 --- a/tests/model/schema.js +++ b/tests/model/schema.js @@ -1850,6 +1850,35 @@ describe( 'Schema', () => { expect( getData( model, { withoutSelection: true } ) ).to.equal( '
abc
' ); } ); } ); + + it( 'should filter out all attributes from descendants that are merged while clearing', () => { + schema.addAttributeCheck( ( ctx, attributeName ) => { + // Disallow `a` in div>$text. + if ( ctx.endsWith( 'div $text' ) && ( attributeName == 'a' || attributeName == 'b' ) ) { + return false; + } + } ); + + const a = new Text( 'a', { a: 1, b: 1 } ); + const b = new Text( 'b', { b: 1 } ); + const c = new Text( 'c', { a: 1, b: 1 } ); + const div = new Element( 'div', [], [ a, b, c ] ); + + root._appendChild( [ div ] ); + + model.change( writer => { + schema.removeDisallowedAttributes( [ div ], writer ); + + expect( writer.batch.operations ).to.length( 5 ); + expect( writer.batch.operations[ 0 ] ).to.instanceof( AttributeOperation ); + expect( writer.batch.operations[ 1 ] ).to.instanceof( AttributeOperation ); + expect( writer.batch.operations[ 2 ] ).to.instanceof( AttributeOperation ); + expect( writer.batch.operations[ 3 ] ).to.instanceof( AttributeOperation ); + expect( writer.batch.operations[ 4 ] ).to.instanceof( AttributeOperation ); + + expect( getData( model, { withoutSelection: true } ) ).to.equal( '
abc
' ); + } ); + } ); } ); describe( 'definitions compilation', () => { From 95ee5e6bd66430a57331f39af0a39abbe92d10e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 11 Sep 2019 14:47:01 +0200 Subject: [PATCH 3/4] More improvements for filtering out disallowed attributes on nodes. --- src/model/schema.js | 31 +++++++++++--- tests/model/schema.js | 98 +++++++++++++++++++++++-------------------- 2 files changed, 77 insertions(+), 52 deletions(-) diff --git a/src/model/schema.js b/src/model/schema.js index ccf35f6b6..b710234c2 100644 --- a/src/model/schema.js +++ b/src/model/schema.js @@ -732,16 +732,35 @@ export default class Schema { * @param {module:engine/model/writer~Writer} writer */ removeDisallowedAttributes( nodes, writer ) { - for ( const node of Array.from( nodes ) ) { + for ( const node of nodes ) { + // When node is a `Text` it has no children, so just filter it out. + if ( node.is( 'text' ) ) { + removeDisallowedAttributeFromNode( this, node ); + + continue; + } + + // In a case of `Element` iterates through positions between nodes inside this element + // and filter out node before the current position, or position parent when position + // is at start of an element. Using positions prevent from omitting merged nodes + // see https://github.com/ckeditor/ckeditor5-engine/issues/1789. + + const rangeInNode = Range._createIn( node ); + const positionsInRange = rangeInNode.getPositions(); + + for ( const position of positionsInRange ) { + const item = position.nodeBefore || position.parent; + + removeDisallowedAttributeFromNode( this, item ); + } + } + + function removeDisallowedAttributeFromNode( schema, node ) { for ( const attribute of node.getAttributeKeys() ) { - if ( !this.checkAttribute( node, attribute ) ) { + if ( !schema.checkAttribute( node, attribute ) ) { writer.removeAttribute( attribute, node ); } } - - if ( node.is( 'element' ) ) { - this.removeDisallowedAttributes( node.getChildren(), writer ); - } } } diff --git a/tests/model/schema.js b/tests/model/schema.js index 22ac587e6..6c7622778 100644 --- a/tests/model/schema.js +++ b/tests/model/schema.js @@ -1769,6 +1769,21 @@ describe( 'Schema', () => { } ); } ); + it( 'should filter out disallowed attributes from empty element', () => { + schema.extend( 'div', { allowAttributes: 'a' } ); + + const div = new Element( 'div', { a: 1, b: 1 } ); + + root._appendChild( [ div ] ); + + model.change( writer => { + schema.removeDisallowedAttributes( [ div ], writer ); + + expect( getData( model, { withoutSelection: true } ) ) + .to.equal( '
' ); + } ); + } ); + it( 'should filter out disallowed attributes from all descendants of given nodes', () => { schema.addAttributeCheck( ( ctx, attributeName ) => { // Allow 'a' on div>$text. @@ -1790,13 +1805,18 @@ describe( 'Schema', () => { if ( ctx.endsWith( 'div paragraph image' ) && attributeName == 'b' ) { return true; } + + // Allow 'a' on div>paragraph. + if ( ctx.endsWith( 'div paragraph' ) && attributeName == 'a' ) { + return true; + } } ); const foo = new Text( 'foo', { a: 1, b: 1 } ); const bar = new Text( 'bar', { a: 1, b: 1 } ); const imageInDiv = new Element( 'image', { a: 1, b: 1 } ); const imageInParagraph = new Element( 'image', { a: 1, b: 1 } ); - const paragraph = new Element( 'paragraph', [], [ foo, imageInParagraph ] ); + const paragraph = new Element( 'paragraph', { a: 1, b: 1 }, [ foo, imageInParagraph ] ); const div = new Element( 'div', [], [ paragraph, bar, imageInDiv ] ); root._appendChild( [ div ] ); @@ -1804,61 +1824,38 @@ describe( 'Schema', () => { model.change( writer => { schema.removeDisallowedAttributes( root.getChildren(), writer ); - expect( writer.batch.operations ).to.length( 4 ); - expect( writer.batch.operations[ 0 ] ).to.instanceof( AttributeOperation ); - expect( writer.batch.operations[ 1 ] ).to.instanceof( AttributeOperation ); - expect( writer.batch.operations[ 2 ] ).to.instanceof( AttributeOperation ); - expect( writer.batch.operations[ 3 ] ).to.instanceof( AttributeOperation ); - expect( getData( model, { withoutSelection: true } ) ) .to.equal( '
' + - '' + - '<$text b="1">foo' + - '' + - '' + - '<$text a="1">bar' + - '' + + '' + + '<$text b="1">foo' + + '' + + '' + + '<$text a="1">bar' + + '' + '
' ); } ); } ); - it( 'should filter out all attributes from descendants that are merged while clearing', () => { - schema.addAttributeCheck( ( ctx, attributeName ) => { - // Disallow `a` in div>$text. - if ( ctx.endsWith( 'div $text' ) && attributeName == 'a' ) { - return false; - } - } ); + it( 'should filter out disallowed attributes from parent node and all descendants nodes', () => { + schema.extend( 'div', { allowAttributes: 'a' } ); + schema.extend( '$text', { allowAttributes: 'b' } ); - const a = new Text( 'a', { a: 1 } ); - const b = new Text( 'b', { a: 2 } ); - const c = new Text( 'c', { a: 3 } ); - const div = new Element( 'div', [], [ a, b, c ] ); + const foo = new Text( 'foo', { a: 1, b: 1 } ); + const div = new Element( 'div', { a: 1, b: 1 }, [ foo ] ); root._appendChild( [ div ] ); model.change( writer => { - schema.removeDisallowedAttributes( [ div ], writer ); - - expect( writer.batch.operations ).to.length( 3 ); - expect( writer.batch.operations[ 0 ] ).to.instanceof( AttributeOperation ); - expect( writer.batch.operations[ 1 ] ).to.instanceof( AttributeOperation ); - expect( writer.batch.operations[ 2 ] ).to.instanceof( AttributeOperation ); + schema.removeDisallowedAttributes( root.getChildren(), writer ); - expect( getData( model, { withoutSelection: true } ) ).to.equal( '
abc
' ); + expect( getData( model, { withoutSelection: true } ) ) + .to.equal( '
<$text b="1">foo
' ); } ); } ); - it( 'should filter out all attributes from descendants that are merged while clearing', () => { - schema.addAttributeCheck( ( ctx, attributeName ) => { - // Disallow `a` in div>$text. - if ( ctx.endsWith( 'div $text' ) && ( attributeName == 'a' || attributeName == 'b' ) ) { - return false; - } - } ); - + it( 'should filter out all attributes from nodes that are merged while clearing', () => { const a = new Text( 'a', { a: 1, b: 1 } ); const b = new Text( 'b', { b: 1 } ); const c = new Text( 'c', { a: 1, b: 1 } ); @@ -1869,16 +1866,25 @@ describe( 'Schema', () => { model.change( writer => { schema.removeDisallowedAttributes( [ div ], writer ); - expect( writer.batch.operations ).to.length( 5 ); - expect( writer.batch.operations[ 0 ] ).to.instanceof( AttributeOperation ); - expect( writer.batch.operations[ 1 ] ).to.instanceof( AttributeOperation ); - expect( writer.batch.operations[ 2 ] ).to.instanceof( AttributeOperation ); - expect( writer.batch.operations[ 3 ] ).to.instanceof( AttributeOperation ); - expect( writer.batch.operations[ 4 ] ).to.instanceof( AttributeOperation ); - expect( getData( model, { withoutSelection: true } ) ).to.equal( '
abc
' ); } ); } ); + + it( 'should do not filter out sibling nodes', () => { + const foo = new Text( 'foo', { a: 1 } ); + const bar = new Text( 'bar', { a: 1, b: 1 } ); + const biz = new Text( 'biz', { a: 1 } ); + const div = new Element( 'div', [], [ foo, bar, biz ] ); + + root._appendChild( [ div ] ); + + model.change( writer => { + schema.removeDisallowedAttributes( [ bar ], writer ); + + expect( getData( model, { withoutSelection: true } ) ) + .to.equal( '
<$text a="1">foobar<$text a="1">biz
' ); + } ); + } ); } ); describe( 'definitions compilation', () => { From f6a31f8eb2bbb6d23db292ddb7943fe34192f335 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Fri, 13 Sep 2019 10:01:04 +0200 Subject: [PATCH 4/4] Refactoring. Avoid using continue and locally defined functions. --- src/model/schema.js | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/src/model/schema.js b/src/model/schema.js index b710234c2..5c9d9cb52 100644 --- a/src/model/schema.js +++ b/src/model/schema.js @@ -735,30 +735,20 @@ export default class Schema { for ( const node of nodes ) { // When node is a `Text` it has no children, so just filter it out. if ( node.is( 'text' ) ) { - removeDisallowedAttributeFromNode( this, node ); - - continue; + removeDisallowedAttributeFromNode( this, node, writer ); } - // In a case of `Element` iterates through positions between nodes inside this element // and filter out node before the current position, or position parent when position // is at start of an element. Using positions prevent from omitting merged nodes // see https://github.com/ckeditor/ckeditor5-engine/issues/1789. + else { + const rangeInNode = Range._createIn( node ); + const positionsInRange = rangeInNode.getPositions(); - const rangeInNode = Range._createIn( node ); - const positionsInRange = rangeInNode.getPositions(); - - for ( const position of positionsInRange ) { - const item = position.nodeBefore || position.parent; - - removeDisallowedAttributeFromNode( this, item ); - } - } + for ( const position of positionsInRange ) { + const item = position.nodeBefore || position.parent; - function removeDisallowedAttributeFromNode( schema, node ) { - for ( const attribute of node.getAttributeKeys() ) { - if ( !schema.checkAttribute( node, attribute ) ) { - writer.removeAttribute( attribute, node ); + removeDisallowedAttributeFromNode( this, item, writer ); } } } @@ -1611,3 +1601,11 @@ function* convertToMinimalFlatRanges( ranges ) { yield* range.getMinimalFlatRanges(); } } + +function removeDisallowedAttributeFromNode( schema, node, writer ) { + for ( const attribute of node.getAttributeKeys() ) { + if ( !schema.checkAttribute( node, attribute ) ) { + writer.removeAttribute( attribute, node ); + } + } +}