Crash when unlinking a linked image

[Reinmar]

📝 Provide detailed reproduction steps (if any)

1. Click a linked image.
2. Press Cmd+K.
3. Press Esc or click "Cancel".
4. Click "Unlink".

✔️ Expected result

All works fine.

❌ Actual result

downcastwriter.js:323 Uncaught TypeError: Cannot read property '_setAttribute' of undefined
    at DowncastWriter.setAttribute (downcastwriter.js:323)
    at DowncastDispatcher.<anonymous> (linkimageediting.js:218)
    at DowncastDispatcher.fire (emittermixin.js:209)
    at DowncastDispatcher._testAndFire (downcastdispatcher.js:560)
    at DowncastDispatcher.convertAttribute (downcastdispatcher.js:249)
    at DowncastDispatcher.convertChanges (downcastdispatcher.js:161)
    at editingcontroller.js:94
    at View.change (view.js:477)
    at Document.EditingController.listenTo.priority (editingcontroller.js:93)
    at Document.fire (emittermixin.js:209)
setAttribute @ downcastwriter.js:323
(anonymous) @ linkimageediting.js:218
fire @ emittermixin.js:209
_testAndFire @ downcastdispatcher.js:560
convertAttribute @ downcastdispatcher.js:249
convertChanges @ downcastdispatcher.js:161
(anonymous) @ editingcontroller.js:94
change @ view.js:477
EditingController.listenTo.priority @ editingcontroller.js:93
fire @ emittermixin.js:209
_handleChangeBlock @ document.js:309
_runPendingChanges @ model.js:833
change @ model.js:175
execute @ unlinkcommand.js:58
(anonymous) @ observablemixin.js:255
fire @ emittermixin.js:209
<computed> @ observablemixin.js:259
execute @ commandcollection.js:69
execute @ editor.js:291
(anonymous) @ linkui.js:136
fire @ emittermixin.js:209
fireDelegatedEvents @ emittermixin.js:625
fire @ emittermixin.js:232
(anonymous) @ buttonview.js:162
callback @ template.js:956
fire @ emittermixin.js:209
domListener @ emittermixin.js:247

---

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

[Reinmar]

[pomek]

Related to manual decorators.

[pomek]

ckeditor5/packages/ckeditor5-link/src/linkimageediting.js

Lines 209 to 222 in dec11ed

	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 );
	}
	} );
	};
	}

In TC described in the issue, linkInImage in such a case is undefined.

[maxbarnas]

While investigating this bug, I have found that viewFigure in the code from @pomek already has no link in it. 

Unlink command removes the link first, then removes decorator attributes: eb16078/packages/ckeditor5-link/src/unlinkcommand.js#L71

By the time the downcastImageLinkManualDecorator() is ran, there's no link in the image to work on.

[maxbarnas]

More details regarding the Unlink command behavior:

1. Unlink command changes the model of the selected link.
   1. Removes linkHref attribute.
   2. Removes manual decorators, if present.
2. Next, the change event is emitted for attribute:*:image.
   1. No matter what order the listeners were set in, the first event is always attribute:linkHref:image.
   2. After that, other attribute change events are handled, e.g. attribute:linkIsExternal:image.

This becomes a problem if we look at the downcast converter for attribute:linkHref:image:

https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-link/src/linkimageediting.js#L183

It actually removes the link, leaving nothing for downcastImageLinkManualDecorator().

Seems like the reason for that particular order is tied with the order in which attributes are applied to the element. The linkHref being a standard part of linking functionality is always applied first - which translates to its event being emitted first.

Would be great to prevent this kind of conflicting changes. For now, I'll try to add tests checking the execution order of downcaster converters to prevent regression.