Merge branch 'master' into t/1791

src/model/schema.js

@@ -733,14 +733,23 @@ export default class Schema {
 	 */
 	removeDisallowedAttributes( nodes, writer ) {
 		for ( const node of nodes ) {
-			for ( const attribute of node.getAttributeKeys() ) {
-				if ( !this.checkAttribute( node, attribute ) ) {
-					writer.removeAttribute( attribute, node );
-				}
+			// When node is a `Text` it has no children, so just filter it out.
+			if ( node.is( 'text' ) ) {
+				removeDisallowedAttributeFromNode( this, node, writer );
 			}
-
-			if ( node.is( 'element' ) ) {
-				this.removeDisallowedAttributes( node.getChildren(), 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();
+
+				for ( const position of positionsInRange ) {
+					const item = position.nodeBefore || position.parent;
+
+					removeDisallowedAttributeFromNode( this, item, writer );
+				}
 			}
 		}
 	}
@@ -1592,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 );
+		}
+	}
+}

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( '<div a="1"></div>' );
+			} );
+		} );
+
 		it( 'should filter out disallowed attributes from all descendants of given nodes', () => {
 			schema.addAttributeCheck( ( ctx, attributeName ) => {
 				// Allow 'a' on div>$text.
@@ -1790,39 +1805,86 @@ 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 ] );
 
 			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(
 						'<div>' +
-						'<paragraph>' +
-						'<$text b="1">foo</$text>' +
-						'<image b="1"></image>' +
-						'</paragraph>' +
-						'<$text a="1">bar</$text>' +
-						'<image a="1"></image>' +
+							'<paragraph a="1">' +
+								'<$text b="1">foo</$text>' +
+								'<image b="1"></image>' +
+							'</paragraph>' +
+							'<$text a="1">bar</$text>' +
+							'<image a="1"></image>' +
 						'</div>'
 					);
 			} );
 		} );
+
+		it( 'should filter out disallowed attributes from parent node and all descendants nodes', () => {
+			schema.extend( 'div', { allowAttributes: 'a' } );
+			schema.extend( '$text', { allowAttributes: 'b' } );
+
+			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( root.getChildren(), writer );
+
+				expect( getData( model, { withoutSelection: true } ) )
+					.to.equal( '<div a="1"><$text b="1">foo</$text></div>' );
+			} );
+		} );
+
+		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 } );
+			const div = new Element( 'div', [], [ a, b, c ] );
+
+			root._appendChild( [ div ] );
+
+			model.change( writer => {
+				schema.removeDisallowedAttributes( [ div ], writer );
+
+				expect( getData( model, { withoutSelection: true } ) ).to.equal( '<div>abc</div>' );
+			} );
+		} );
+
+		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( '<div><$text a="1">foo</$text>bar<$text a="1">biz</$text></div>' );
+			} );
+		} );
 	} );
 
 	describe( 'definitions compilation', () => {