diff --git a/packages/ckeditor5-link/src/linkcommand.js b/packages/ckeditor5-link/src/linkcommand.js index f9703437541..6932ae5e16e 100644 --- a/packages/ckeditor5-link/src/linkcommand.js +++ b/packages/ckeditor5-link/src/linkcommand.js @@ -187,7 +187,7 @@ export default class LinkCommand extends Command { writer.setSelection( writer.createPositionAfter( linkRange.end.nodeBefore ) ); } // If not then insert text node with `linkHref` attribute in place of caret. - // However, since selection in collapsed, attribute value will be used as data for text node. + // However, since selection is collapsed, attribute value will be used as data for text node. // So, if `href` is empty, do not create text node. else if ( href !== '' ) { const attributes = toMap( selection.getAttributes() ); diff --git a/packages/ckeditor5-link/src/linkimageediting.js b/packages/ckeditor5-link/src/linkimageediting.js index 0a36adafaff..abf165038b3 100644 --- a/packages/ckeditor5-link/src/linkimageediting.js +++ b/packages/ckeditor5-link/src/linkimageediting.js @@ -214,6 +214,13 @@ function downcastImageLinkManualDecorator( manualDecorators, decorator ) { const viewFigure = conversionApi.mapper.toViewElement( data.item ); const linkInImage = Array.from( viewFigure.getChildren() ).find( child => child.name === 'a' ); + // The element was removed by the time this converter is executed. + // It may happen when the base `linkHref` and decorator attributes are removed + // at the same time (see #8401). + if ( !linkInImage ) { + return; + } + for ( const [ key, val ] of toMap( attributes ) ) { conversionApi.writer.setAttribute( key, val, linkInImage ); } diff --git a/packages/ckeditor5-link/tests/linkimageediting.js b/packages/ckeditor5-link/tests/linkimageediting.js index 51c980fa025..f0c9e16c7ba 100644 --- a/packages/ckeditor5-link/tests/linkimageediting.js +++ b/packages/ckeditor5-link/tests/linkimageediting.js @@ -826,6 +826,81 @@ describe( 'LinkImageEditing', () => { '

' ); } ); + + // See #8401. + it( 'should downcast without error if the image already has no link', () => { + setModelData( model, + '[bar]' + ); + + editor.execute( 'link', 'https://cksource.com', { + linkIsDownloadable: true, + linkIsExternal: true, + linkIsGallery: true + } ); + + // Attributes will be removed along with the link, but the downcast will be fired. + // The lack of link should not affect the downcasting. + expect( () => { + editor.execute( 'unlink', 'https://cksource.com', { + linkIsDownloadable: true, + linkIsExternal: true, + linkIsGallery: true + } ); + } ).to.not.throw(); + + expect( editor.getData() ).to.equal( + '
' + + 'bar' + + '
' + ); + } ); + + // See #8401. + describe( 'order of model updates', () => { + it( 'should not affect converters - base link attributes first', () => { + setModelData( model, + '[]' + ); + + model.change( writer => { + const ranges = model.schema.getValidRanges( model.document.selection.getRanges(), 'linkIsDownloadable' ); + + for ( const range of ranges ) { + // The `linkHref` should be processed first - this is the default order of `LinkCommand`. + writer.setAttribute( 'linkHref', 'url', range ); + writer.setAttribute( 'linkIsDownloadable', true, range ); + } + } ); + + expect( getModelData( model, { withoutSelection: true } ) ).to.equal( + '' + ); + } ); + + it( 'should not affect converters - decorators first', () => { + setModelData( model, + '[]' + ); + + model.change( writer => { + const ranges = model.schema.getValidRanges( model.document.selection.getRanges(), 'linkIsDownloadable' ); + + for ( const range of ranges ) { + // Here we force attributes to be set on a model in a different order + // to force unusual order of downcast converters down the line. + // Normally, the `linkHref` gets processed first, as it is just the first property assigned + // to the model by `LinkCommand`. + writer.setAttribute( 'linkIsDownloadable', true, range ); + writer.setAttribute( 'linkHref', 'url', range ); + } + } ); + + expect( getModelData( model, { withoutSelection: true } ) ).to.equal( + '' + ); + } ); + } ); } ); } ); } );