Merge pull request #11 from ckeditor/t/10

Fix: Block quote should not be applied to image's caption. Closes: #10.

package.json

@@ -15,6 +15,7 @@
     "@ckeditor/ckeditor5-dev-lint": "^2.0.2",
     "@ckeditor/ckeditor5-editor-classic": "^0.7.2",
     "@ckeditor/ckeditor5-enter": "^0.9.0",
+    "@ckeditor/ckeditor5-image": "^0.5.0",
     "@ckeditor/ckeditor5-list": "^0.6.0",
     "@ckeditor/ckeditor5-paragraph": "^0.7.0",
     "@ckeditor/ckeditor5-presets": "^0.2.0",

src/blockquotecommand.js

@@ -64,14 +64,21 @@ export default class BlockQuoteCommand extends Command {
 	 */
 	_doExecute( options = {} ) {
 		const doc = this.editor.document;
+		const schema = doc.schema;
 		const batch = options.batch || doc.batch();
 		const blocks = Array.from( doc.selection.getSelectedBlocks() );
 
 		doc.enqueueChanges( () => {
 			if ( this.value ) {
 				this._removeQuote( batch, blocks.filter( findQuote ) );
 			} else {
-				this._applyQuote( batch, blocks );
+				const blocksToQuote = blocks.filter( block => {
+					// Already quoted blocks needs to be considered while quoting too
+					// in order to reuse their <bQ> elements.
+					return findQuote( block ) || checkCanBeQuoted( schema, block );
+				} );
+
+				this._applyQuote( batch, blocksToQuote );
 			}
 		} );
 	}
@@ -93,18 +100,7 @@ export default class BlockQuoteCommand extends Command {
 			return false;
 		}
 
-		const isMQAllowed = schema.check( {
-			name: 'blockQuote',
-			inside: Position.createBefore( firstBlock )
-		} );
-		const isBlockAllowed = schema.check( {
-			name: firstBlock.name,
-			attributes: Array.from( firstBlock.getAttributeKeys() ),
-			inside: 'blockQuote'
-		} );
-
-		// Whether <mQ> can wrap the block.
-		return isMQAllowed && isBlockAllowed;
+		return checkCanBeQuoted( schema, firstBlock );
 	}
 
 	/**
@@ -126,7 +122,7 @@ export default class BlockQuoteCommand extends Command {
 				return;
 			}
 
-			// The group of blocks are at the beginning of an <mQ> so let's move them left (out of the <mQ>).
+			// The group of blocks are at the beginning of an <bQ> so let's move them left (out of the <bQ>).
 			if ( groupRange.start.isAtStart ) {
 				const positionBefore = Position.createBefore( groupRange.start.parent );
 
@@ -135,7 +131,7 @@ export default class BlockQuoteCommand extends Command {
 				return;
 			}
 
-			// The blocks are in the middle of an <mQ> so we need to split the <mQ> after the last block
+			// The blocks are in the middle of an <bQ> so we need to split the <bQ> after the last block
 			// so we move the items there.
 			if ( !groupRange.end.isAtEnd ) {
 				batch.split( groupRange.end );
@@ -171,9 +167,9 @@ export default class BlockQuoteCommand extends Command {
 			quotesToMerge.push( quote );
 		} );
 
-		// Merge subsequent <mQ> elements. Reverse the order again because this time we want to go through
-		// the <mQ> elements in the source order (due to how merge works – it moves the right element's content
-		// to the first element and removes the right one. Since we may need to merge a couple of subsequent `<mQ>` elements
+		// Merge subsequent <bQ> elements. Reverse the order again because this time we want to go through
+		// the <bQ> elements in the source order (due to how merge works – it moves the right element's content
+		// to the first element and removes the right one. Since we may need to merge a couple of subsequent `<bQ>` elements
 		// we want to keep the reference to the first (furthest left) one.
 		quotesToMerge.reverse().reduce( ( currentQuote, nextQuote ) => {
 			if ( currentQuote.nextSibling == nextQuote ) {
@@ -222,3 +218,18 @@ function getRangesOfBlockGroups( blocks ) {
 
 	return ranges;
 }
+
+// Checks whether <bQ> can wrap the block.
+function checkCanBeQuoted( schema, block ) {
+	const isBQAllowed = schema.check( {
+		name: 'blockQuote',
+		inside: Position.createBefore( block )
+	} );
+	const isBlockAllowedInBQ = schema.check( {
+		name: block.name,
+		attributes: Array.from( block.getAttributeKeys() ),
+		inside: 'blockQuote'
+	} );
+
+	return isBQAllowed && isBlockAllowedInBQ;
+}

tests/blockquotecommand.js

@@ -367,6 +367,69 @@ describe( 'BlockQuoteCommand', () => {
 					'<p>y</p>'
 				);
 			} );
+
+			it( 'should not wrap a block which can not be in a quote', () => {
+				// blockQuote is allowed in root, but fooBlock can not be inside blockQuote.
+				doc.schema.registerItem( 'fooBlock', '$block' );
+				doc.schema.disallow( { name: 'fooBlock', inside: 'blockQuote' } );
+				buildModelConverter().for( editor.editing.modelToView )
+					.fromElement( 'fooBlock' )
+					.toElement( 'fooblock' );
+
+				setModelData(
+					doc,
+					'<paragraph>a[bc</paragraph>' +
+					'<fooBlock>xx</fooBlock>' +
+					'<paragraph>de]f</paragraph>'
+				);
+
+				editor.execute( 'blockQuote' );
+
+				expect( getModelData( doc ) ).to.equal(
+					'<blockQuote>' +
+						'<paragraph>a[bc</paragraph>' +
+					'</blockQuote>' +
+					'<fooBlock>xx</fooBlock>' +
+					'<blockQuote>' +
+						'<paragraph>de]f</paragraph>' +
+					'</blockQuote>'
+				);
+			} );
+
+			it( 'should not wrap a block which parent does not allow quote inside itself', () => {
+				// blockQuote is not be allowed in fooWrapper, but fooBlock can be inside blockQuote.
+				doc.schema.registerItem( 'fooWrapper' );
+				doc.schema.registerItem( 'fooBlock', '$block' );
+
+				doc.schema.allow( { name: 'fooWrapper', inside: '$root' } );
+				doc.schema.allow( { name: 'fooBlock', inside: 'fooWrapper' } );
+
+				buildModelConverter().for( editor.editing.modelToView )
+					.fromElement( 'fooWrapper' )
+					.toElement( 'foowrapper' );
+				buildModelConverter().for( editor.editing.modelToView )
+					.fromElement( 'fooBlock' )
+					.toElement( 'fooblock' );
+
+				setModelData(
+					doc,
+					'<paragraph>a[bc</paragraph>' +
+					'<fooWrapper><fooBlock>xx</fooBlock></fooWrapper>' +
+					'<paragraph>de]f</paragraph>'
+				);
+
+				editor.execute( 'blockQuote' );
+
+				expect( getModelData( doc ) ).to.equal(
+					'<blockQuote>' +
+						'<paragraph>a[bc</paragraph>' +
+					'</blockQuote>' +
+					'<fooWrapper><fooBlock>xx</fooBlock></fooWrapper>' +
+					'<blockQuote>' +
+						'<paragraph>de]f</paragraph>' +
+					'</blockQuote>'
+				);
+			} );
 		} );
 
 		describe( 'removing quote', () => {

tests/integration.js

@@ -7,6 +7,8 @@
 
 import BlockQuote from '../src/blockquote';
 import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
+import Image from '@ckeditor/ckeditor5-image/src/image';
+import ImageCaption from '@ckeditor/ckeditor5-image/src/imagecaption';
 import List from '@ckeditor/ckeditor5-list/src/list';
 import Enter from '@ckeditor/ckeditor5-enter/src/enter';
 import Delete from '@ckeditor/ckeditor5-typing/src/delete';
@@ -22,7 +24,7 @@ describe( 'BlockQuote', () => {
 		document.body.appendChild( element );
 
 		return ClassicTestEditor.create( element, {
-			plugins: [ BlockQuote, Paragraph, List, Enter, Delete ]
+			plugins: [ BlockQuote, Paragraph, Image, ImageCaption, List, Enter, Delete ]
 		} )
 		.then( newEditor => {
 			editor = newEditor;
@@ -289,4 +291,74 @@ describe( 'BlockQuote', () => {
 			);
 		} );
 	} );
+
+	describe( 'compatibility with images', () => {
+		it( 'does not quote a simple image', () => {
+			const element = document.createElement( 'div' );
+			document.body.appendChild( element );
+
+			// We can't load ImageCaption in this test because it adds <caption> to all images automatically.
+			return ClassicTestEditor.create( element, {
+					plugins: [ BlockQuote, Paragraph, Image ]
+				} )
+				.then( ( editor ) => {
+					setModelData( editor.document,
+						'<paragraph>fo[o</paragraph>' +
+						'<image src="foo.png"></image>' +
+						'<paragraph>b]ar</paragraph>'
+					);
+
+					editor.execute( 'blockQuote' );
+
+					expect( getModelData( editor.document ) ).to.equal(
+						'<blockQuote><paragraph>fo[o</paragraph></blockQuote>' +
+						'<image src="foo.png"></image>' +
+						'<blockQuote><paragraph>b]ar</paragraph></blockQuote>'
+					);
+
+					element.remove();
+					editor.destroy();
+				} );
+		} );
+
+		it( 'does not quote an image with caption', () => {
+			setModelData( doc,
+				'<paragraph>fo[o</paragraph>' +
+				'<image src="foo.png">' +
+					'<caption>xxx</caption>' +
+				'</image>' +
+				'<paragraph>b]ar</paragraph>'
+			);
+
+			editor.execute( 'blockQuote' );
+
+			expect( getModelData( doc ) ).to.equal(
+				'<blockQuote><paragraph>fo[o</paragraph></blockQuote>' +
+				'<image src="foo.png">' +
+					'<caption>xxx</caption>' +
+				'</image>' +
+				'<blockQuote><paragraph>b]ar</paragraph></blockQuote>'
+			);
+		} );
+
+		it( 'does not add an image to existing quote', () => {
+			setModelData( doc,
+				'<paragraph>fo[o</paragraph>' +
+				'<image src="foo.png">' +
+					'<caption>xxx</caption>' +
+				'</image>' +
+				'<blockQuote><paragraph>b]ar</paragraph></blockQuote>'
+			);
+
+			editor.execute( 'blockQuote' );
+
+			expect( getModelData( doc ) ).to.equal(
+				'<blockQuote><paragraph>fo[o</paragraph></blockQuote>' +
+				'<image src="foo.png">' +
+					'<caption>xxx</caption>' +
+				'</image>' +
+				'<blockQuote><paragraph>b]ar</paragraph></blockQuote>'
+			);
+		} );
+	} );
 } );