Merge pull request #7634 from ckeditor/i/7519

Fix (link): Manual and automatic decorators will work properly with a link on an image. Closes #7519.

Internal (link): An icon that indicates that the image is linked will not be displayed in the editor's data. Closes #7626.

packages/ckeditor5-link/src/linkcommand.js

@@ -12,6 +12,8 @@ import findLinkRange from './findlinkrange';
 import toMap from '@ckeditor/ckeditor5-utils/src/tomap';
 import Collection from '@ckeditor/ckeditor5-utils/src/collection';
 import first from '@ckeditor/ckeditor5-utils/src/first';
+import AutomaticDecorators from './utils/automaticdecorators';
+import { isImageAllowed } from './utils';
 
 /**
  * The link command. It is used by the {@link module:link/link~Link link feature}.
@@ -40,6 +42,15 @@ export default class LinkCommand extends Command {
 		 * @type {module:utils/collection~Collection}
 		 */
 		this.manualDecorators = new Collection();
+
+		/**
+		 * An instance of the helper that ties together all {@link module:link/link~LinkDecoratorAutomaticDefinition}
+		 * that are used by the {@glink features/link link} and the {@glink features/image#linking-images linking images} features.
+		 *
+		 * @readonly
+		 * @type {module:link/utils~AutomaticDecorators}
+		 */
+		this.automaticDecorators = new AutomaticDecorators();
 	}
 
 	/**
@@ -62,7 +73,7 @@ export default class LinkCommand extends Command {
 
 		// A check for the `LinkImage` plugin. If the selection contains an element, get values from the element.
 		// Currently the selection reads attributes from text nodes only. See #7429 and #7465.
-		if ( selectedElement && selectedElement.is( 'image' ) && model.schema.checkAttribute( 'image', 'linkHref' ) ) {
+		if ( isImageAllowed( selectedElement, model.schema ) ) {
 			this.value = selectedElement.getAttribute( 'linkHref' );
 			this.isEnabled = model.schema.checkAttribute( selectedElement, 'linkHref' );
 		} else {
@@ -248,7 +259,17 @@ export default class LinkCommand extends Command {
 	 * @returns {Boolean} The information whether a given decorator is currently present in the selection.
 	 */
 	_getDecoratorStateFromModel( decoratorName ) {
-		const doc = this.editor.model.document;
+		const model = this.editor.model;
+		const doc = model.document;
+
+		const selectedElement = first( doc.selection.getSelectedBlocks() );
+
+		// A check for the `LinkImage` plugin. If the selection contains an element, get values from the element.
+		// Currently the selection reads attributes from text nodes only. See #7429 and #7465.
+		if ( isImageAllowed( selectedElement, model.schema ) ) {
+			return selectedElement.getAttribute( decoratorName );
+		}
+
 		return doc.selection.getAttribute( decoratorName );
 	}
 

packages/ckeditor5-link/src/linkediting.js

@@ -14,7 +14,6 @@ import Input from '@ckeditor/ckeditor5-typing/src/input';
 import Clipboard from '@ckeditor/ckeditor5-clipboard/src/clipboard';
 import LinkCommand from './linkcommand';
 import UnlinkCommand from './unlinkcommand';
-import AutomaticDecorators from './utils/automaticdecorators';
 import ManualDecorator from './utils/manualdecorator';
 import findLinkRange from './findlinkrange';
 import { createLinkElement, ensureSafeUrl, getLocalizedDecorators, normalizeDecorators } from './utils';
@@ -132,7 +131,10 @@ export default class LinkEditing extends Plugin {
 	 */
 	_enableAutomaticDecorators( automaticDecoratorDefinitions ) {
 		const editor = this.editor;
-		const automaticDecorators = new AutomaticDecorators();
+		// Store automatic decorators in the command instance as we do the same with manual decorators.
+		// Thanks to that, `LinkImageEditing` plugin can re-use the same definitions.
+		const command = editor.commands.get( 'link' );
+		const automaticDecorators = command.automaticDecorators;
 
 		// Adds a default decorator for external links.
 		if ( editor.config.get( 'link.addTargetToExternalLinks' ) ) {

packages/ckeditor5-link/src/linkimageediting.js

@@ -9,6 +9,8 @@
 
 import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
 import ImageEditing from '@ckeditor/ckeditor5-image/src/image/imageediting';
+import Matcher from '@ckeditor/ckeditor5-engine/src/view/matcher';
+import toMap from '@ckeditor/ckeditor5-utils/src/tomap';
 import LinkEditing from './linkediting';
 
 import linkIcon from '../theme/icons/link.svg';
@@ -42,15 +44,53 @@ export default class LinkImageEditing extends Plugin {
 		editor.model.schema.extend( 'image', { allowAttributes: [ 'linkHref' ] } );
 
 		editor.conversion.for( 'upcast' ).add( upcastLink() );
-		editor.conversion.for( 'downcast' ).add( downcastImageLink() );
+		editor.conversion.for( 'editingDowncast' ).add( downcastImageLink( { attachIconIndicator: true } ) );
+		editor.conversion.for( 'dataDowncast' ).add( downcastImageLink( { attachIconIndicator: false } ) );
+
+		// Definitions for decorators are provided by the `link` command and the `LinkEditing` plugin.
+		this._enableAutomaticDecorators();
+		this._enableManualDecorators();
+	}
+
+	/**
+	 * Processes {@link module:link/link~LinkDecoratorAutomaticDefinition automatic decorators} definitions and
+	 * attaches proper converters that will work when linking an image.`
+	 *
+	 * @private
+	 */
+	_enableAutomaticDecorators() {
+		const editor = this.editor;
+		const command = editor.commands.get( 'link' );
+		const automaticDecorators = command.automaticDecorators;
+
+		if ( automaticDecorators.length ) {
+			editor.conversion.for( 'downcast' ).add( automaticDecorators.getDispatcherForLinkedImage() );
+		}
+	}
+
+	/**
+	 * Processes transformed {@link module:link/utils~ManualDecorator} instances and attaches proper converters
+	 * that will work when linking an image.
+	 *
+	 * @private
+	 */
+	_enableManualDecorators() {
+		const editor = this.editor;
+		const command = editor.commands.get( 'link' );
+		const manualDecorators = command.manualDecorators;
+
+		for ( const decorator of command.manualDecorators ) {
+			editor.model.schema.extend( 'image', { allowAttributes: decorator.id } );
+			editor.conversion.for( 'downcast' ).add( downcastImageLinkManualDecorator( manualDecorators, decorator ) );
+			editor.conversion.for( 'upcast' ).add( upcastImageLinkManualDecorator( manualDecorators, decorator ) );
+		}
 	}
 }
 
 // Returns a converter that consumes the 'href' attribute if a link contains an image.
 //
 // @private
 // @returns {Function}
-//
 function upcastLink() {
 	return dispatcher => {
 		dispatcher.on( 'element:a', ( evt, data, conversionApi ) => {
@@ -99,15 +139,18 @@ function upcastLink() {
 				conversionApi.writer.setAttribute( 'linkHref', linkHref, modelElement );
 			}
 		}, { priority: 'high' } );
+		// Using the same priority that `upcastImageLinkManualDecorator()` converter guarantees
+		// that manual decorators will decorate the proper element.
 	};
 }
 
 // Return a converter that adds the `<a>` element to data.
 //
 // @private
+// @params {Object} options
+// @params {Boolean} options.attachIconIndicator=false If set to `true`, an icon that informs about the linked image will be added.
 // @returns {Function}
-//
-function downcastImageLink() {
+function downcastImageLink( options ) {
 	return dispatcher => {
 		dispatcher.on( 'attribute:linkHref:image', ( evt, data, conversionApi ) => {
 			// The image will be already converted - so it will be present in the view.
@@ -117,13 +160,17 @@ function downcastImageLink() {
 			// But we need to check whether the link element exists.
 			const linkInImage = Array.from( viewFigure.getChildren() ).find( child => child.name === 'a' );
 
-			// Create an icon indicator for a linked image.
-			const linkIconIndicator = writer.createUIElement( 'span', { class: 'ck ck-link-image_icon' }, function( domDocument ) {
-				const domElement = this.toDomElement( domDocument );
-				domElement.innerHTML = linkIcon;
+			let linkIconIndicator;
 
-				return domElement;
-			} );
+			if ( options.attachIconIndicator ) {
+				// Create an icon indicator for a linked image.
+				linkIconIndicator = writer.createUIElement( 'span', { class: 'ck ck-link-image_icon' }, function( domDocument ) {
+					const domElement = this.toDomElement( domDocument );
+					domElement.innerHTML = linkIcon;
+
+					return domElement;
+				} );
+			}
 
 			// If so, update the attribute if it's defined or remove the entire link if the attribute is empty.
 			if ( linkInImage ) {
@@ -146,9 +193,65 @@ function downcastImageLink() {
 				// 3. Move the image to the link.
 				writer.move( writer.createRangeOn( viewFigure.getChild( 1 ) ), writer.createPositionAt( linkElement, 0 ) );
 
-				// 4. Inset the linked image icon indicator.
-				writer.insert( writer.createPositionAt( linkElement, 'end' ), linkIconIndicator );
+				// 4. Inset the linked image icon indicator while downcast to editing.
+				if ( linkIconIndicator ) {
+					writer.insert( writer.createPositionAt( linkElement, 'end' ), linkIconIndicator );
+				}
 			}
 		} );
 	};
 }
+
+// Returns a converter that decorates the `<a>` element when the image is the link label.
+//
+// @private
+// @returns {Function}
+function downcastImageLinkManualDecorator( manualDecorators, decorator ) {
+	return dispatcher => {
+		dispatcher.on( `attribute:${ decorator.id }:image`, ( evt, data, conversionApi ) => {
+			const attributes = manualDecorators.get( decorator.id ).attributes;
+
+			const viewFigure = conversionApi.mapper.toViewElement( data.item );
+			const linkInImage = Array.from( viewFigure.getChildren() ).find( child => child.name === 'a' );
+
+			for ( const [ key, val ] of toMap( attributes ) ) {
+				conversionApi.writer.setAttribute( key, val, linkInImage );
+			}
+		} );
+	};
+}
+
+// Returns a converter that checks whether manual decorators should be applied to the link.
+//
+// @private
+// @returns {Function}
+function upcastImageLinkManualDecorator( manualDecorators, decorator ) {
+	return dispatcher => {
+		dispatcher.on( 'element:a', ( evt, data, conversionApi ) => {
+			const viewLink = data.viewItem;
+
+			const consumableAttributes = {
+				attributes: manualDecorators.get( decorator.id ).attributes
+			};
+
+			const matcher = new Matcher( consumableAttributes );
+			const result = matcher.match( viewLink );
+
+			// The link element does not have required attributes or/and proper values.
+			if ( !result ) {
+				return;
+			}
+
+			// Check whether we can consume those attributes.
+			if ( !conversionApi.consumable.consume( viewLink, result.match ) ) {
+				return;
+			}
+
+			// At this stage we can assume that we have the `<image>` element.
+			const modelElement = data.modelCursor.parent;
+
+			conversionApi.writer.setAttribute( decorator.id, true, modelElement );
+		}, { priority: 'high' } );
+		// Using the same priority that `upcastLink()` converter guarantees that the linked image was properly converted.
+	};
+}

packages/ckeditor5-link/src/unlinkcommand.js

@@ -8,8 +8,9 @@
  */
 
 import Command from '@ckeditor/ckeditor5-core/src/command';
-import findLinkRange from './findlinkrange';
 import first from '@ckeditor/ckeditor5-utils/src/first';
+import findLinkRange from './findlinkrange';
+import { isImageAllowed } from './utils';
 
 /**
  * The unlink command. It is used by the {@link module:link/link~Link link plugin}.
@@ -28,7 +29,7 @@ export default class UnlinkCommand extends Command {
 
 		// A check for the `LinkImage` plugin. If the selection contains an image element, get values from the element.
 		// Currently the selection reads attributes from text nodes only. See #7429 and #7465.
-		if ( selectedElement && selectedElement.is( 'image' ) && model.schema.checkAttribute( 'image', 'linkHref' ) ) {
+		if ( isImageAllowed( selectedElement, model.schema ) ) {
 			this.isEnabled = model.schema.checkAttribute( selectedElement, 'linkHref' );
 		} else {
 			this.isEnabled = model.schema.checkAttributeInSelection( doc.selection, 'linkHref' );

packages/ckeditor5-link/src/utils.js

@@ -119,3 +119,18 @@ export function normalizeDecorators( decorators ) {
 
 	return retArray;
 }
+
+/**
+ * Returns `true` if the specified `element` is an image and it can be linked (the element allows having the `linkHref` attribute).
+ *
+ * @params {module:engine/model/element~Element|null} element
+ * @params {module:engine/model/schema~Schema} schema
+ * @returns {Boolean}
+ */
+export function isImageAllowed( element, schema ) {
+	if ( !element ) {
+		return false;
+	}
+
+	return element.is( 'image' ) && schema.checkAttribute( 'image', 'linkHref' );
+}

packages/ckeditor5-link/src/utils/automaticdecorators.js

@@ -7,9 +7,11 @@
  * @module link/utils
  */
 
+import toMap from '@ckeditor/ckeditor5-utils/src/tomap';
+
 /**
  * Helper class that ties together all {@link module:link/link~LinkDecoratorAutomaticDefinition} and provides
- * a {@link module:engine/conversion/downcasthelpers~DowncastHelpers#attributeToElement downcast dispatcher} for them.
+ * the {@link module:engine/conversion/downcasthelpers~DowncastHelpers#attributeToElement downcast dispatchers} for them.
  */
 export default class AutomaticDecorators {
 	constructor() {
@@ -85,4 +87,42 @@ export default class AutomaticDecorators {
 			}, { priority: 'high' } );
 		};
 	}
+
+	/**
+	 * Provides the conversion helper used in the {@link module:engine/conversion/downcasthelpers~DowncastHelpers#add} method
+	 * when linking images.
+	 *
+	 * @returns {Function} A dispatcher function used as conversion helper
+	 * in {@link module:engine/conversion/downcasthelpers~DowncastHelpers#add}.
+	 */
+	getDispatcherForLinkedImage() {
+		return dispatcher => {
+			dispatcher.on( 'attribute:linkHref:image', ( evt, data, conversionApi ) => {
+				const viewFigure = conversionApi.mapper.toViewElement( data.item );
+				const linkInImage = Array.from( viewFigure.getChildren() ).find( child => child.name === 'a' );
+
+				for ( const item of this._definitions ) {
+					const attributes = toMap( item.attributes );
+
+					if ( item.callback( data.attributeNewValue ) ) {
+						for ( const [ key, val ] of attributes ) {
+							if ( key === 'class' ) {
+								conversionApi.writer.addClass( val, linkInImage );
+							} else {
+								conversionApi.writer.setAttribute( key, val, linkInImage );
+							}
+						}
+					} else {
+						for ( const [ key, val ] of attributes ) {
+							if ( key === 'class' ) {
+								conversionApi.writer.removeClass( val, linkInImage );
+							} else {
+								conversionApi.writer.removeAttribute( key, linkInImage );
+							}
+						}
+					}
+				}
+			} );
+		};
+	}
 }

packages/ckeditor5-link/tests/linkcommand.js

@@ -7,6 +7,7 @@ import ModelTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/modeltestedit
 import LinkCommand from '../src/linkcommand';
 import ManualDecorator from '../src/utils/manualdecorator';
 import { setData, getData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
+import AutomaticDecorators from '../src/utils/automaticdecorators';
 
 describe( 'LinkCommand', () => {
 	let editor, model, command;
@@ -440,6 +441,13 @@ describe( 'LinkCommand', () => {
 						allowAttributes: [ 'linkHref', 'linkIsFoo', 'linkIsBar', 'linkIsSth' ]
 					} );
 
+					model.schema.register( 'image', {
+						allowIn: '$root',
+						isObject: true,
+						isBlock: true,
+						allowAttributes: [ 'linkHref', 'linkIsFoo', 'linkIsBar', 'linkIsSth' ]
+					} );
+
 					model.schema.register( 'p', { inheritAllFrom: '$block' } );
 				} );
 		} );
@@ -565,6 +573,19 @@ describe( 'LinkCommand', () => {
 				expect( command._getDecoratorStateFromModel( 'linkIsFoo' ) ).to.be.undefined;
 				expect( command._getDecoratorStateFromModel( 'linkIsBar' ) ).to.be.true;
 			} );
+
+			it( 'obtain current values from the image element', () => {
+				setData( model, '[<image linkHref="url" linkIsBar="true"></image>]' );
+
+				expect( command._getDecoratorStateFromModel( 'linkIsFoo' ) ).to.be.undefined;
+				expect( command._getDecoratorStateFromModel( 'linkIsBar' ) ).to.be.true;
+			} );
+		} );
+	} );
+
+	describe( '#automaticDecorators', () => {
+		it( 'is defined', () => {
+			expect( command.automaticDecorators ).to.be.an.instanceOf( AutomaticDecorators );
 		} );
 	} );
 } );

packages/ckeditor5-link/tests/linkediting.js

@@ -724,6 +724,10 @@ describe( 'LinkEditing', () => {
 						expect( editor.getData() ).to.equal( `<p><a ${ reducedAttr }href="${ link.url }">foo</a>bar</p>` );
 					} );
 				} );
+
+				it( 'stores decorators in LinkCommand#automaticDecorators collection', () => {
+					expect( editor.commands.get( 'link' ).automaticDecorators.length ).to.equal( 3 );
+				} );
 			} );
 		} );