diff --git a/src/image/converters.js b/src/image/converters.js index 8821caba..e4897ae9 100644 --- a/src/image/converters.js +++ b/src/image/converters.js @@ -7,7 +7,6 @@ * @module image/image/converters */ -import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element'; import ModelPosition from '@ckeditor/ckeditor5-engine/src/model/position'; import modelWriter from '@ckeditor/ckeditor5-engine/src/model/writer'; @@ -20,52 +19,46 @@ import modelWriter from '@ckeditor/ckeditor5-engine/src/model/writer'; * * ... * + * The entire contents of `
` except the first `` is being converted as children + * of the `` model element. + * * @returns {Function} */ -export function viewToModelImage() { +export function viewFigureToModel() { return ( evt, data, consumable, conversionApi ) => { - const viewFigureElement = data.input; - - // *** Step 1: Validate conversion. - // Check if figure element can be consumed. - if ( !consumable.test( viewFigureElement, { name: true, class: 'image' } ) ) { + // Do not convert if this is not an "image figure". + if ( !consumable.test( data.input, { name: true, class: 'image' } ) ) { return; } - // Check if image element can be converted in current context. + // Do not convert if image cannot be placed in model at this context. if ( !conversionApi.schema.check( { name: 'image', inside: data.context, attributes: 'src' } ) ) { return; } - // Check if img element is placed inside figure element and can be consumed with `src` attribute. - const viewImg = viewFigureElement.getChild( 0 ); + // Find an image element inside the figure element. + const viewImage = Array.from( data.input.getChildren() ).find( viewChild => viewChild.is( 'img' ) ); - if ( !viewImg || viewImg.name != 'img' || !consumable.test( viewImg, { name: true, attribute: 'src' } ) ) { + // Do not convert if image element is absent, is missing src attribute or was already converted. + if ( !viewImage || !viewImage.hasAttribute( 'src' ) || !consumable.test( viewImage, { name: true } ) ) { return; } - // *** Step2: Convert to model. - consumable.consume( viewFigureElement, { name: true, class: 'image' } ); - consumable.consume( viewImg, { name: true, attribute: 'src' } ); + // Convert view image to model image. + const modelImage = conversionApi.convertItem( viewImage, consumable, data ); - // Create model element. - const modelImage = new ModelElement( 'image', { - src: viewImg.getAttribute( 'src' ) - } ); + // Convert rest of figure element's children, but in the context of model image, because those converted + // children will be added as model image children. + data.context.push( modelImage ); - // Convert `alt` attribute if present. - if ( consumable.consume( viewImg, { attribute: [ 'alt' ] } ) ) { - modelImage.setAttribute( 'alt', viewImg.getAttribute( 'alt' ) ); - } + const modelChildren = conversionApi.convertChildren( data.input, consumable, data ); - // Convert children of converted view element and append them to `modelImage`. - // TODO https://github.com/ckeditor/ckeditor5-engine/issues/736. - data.context.push( modelImage ); - const modelChildren = conversionApi.convertChildren( viewFigureElement, consumable, data ); - const insertPosition = ModelPosition.createAt( modelImage, 'end' ); - modelWriter.insert( insertPosition, modelChildren ); data.context.pop(); + // Add converted children to model image. + modelWriter.insert( ModelPosition.createAt( modelImage ), modelChildren ); + + // Set model image as conversion result. data.output = modelImage; }; } diff --git a/src/image/imageengine.js b/src/image/imageengine.js index c88b0a01..8654d38f 100644 --- a/src/image/imageengine.js +++ b/src/image/imageengine.js @@ -9,8 +9,10 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import buildModelConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildmodelconverter'; -import { viewToModelImage, createImageAttributeConverter } from './converters'; +import buildViewConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildviewconverter'; +import { viewFigureToModel, createImageAttributeConverter } from './converters'; import { toImageWidget } from './utils'; +import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element'; import ViewContainerElement from '@ckeditor/ckeditor5-engine/src/view/containerelement'; import ViewEmptyElement from '@ckeditor/ckeditor5-engine/src/view/emptyelement'; @@ -52,8 +54,19 @@ export default class ImageEngine extends Plugin { createImageAttributeConverter( [ editing.modelToView, data.modelToView ], 'src' ); createImageAttributeConverter( [ editing.modelToView, data.modelToView ], 'alt' ); + // Build converter for view img element to model image element. + buildViewConverter().for( data.viewToModel ) + .from( { name: 'img', attribute: { src: /./ } } ) + .toElement( ( viewImage ) => new ModelElement( 'image', { src: viewImage.getAttribute( 'src' ) } ) ); + + // Build converter for alt attribute. + buildViewConverter().for( data.viewToModel ) + .from( { name: 'img', attribute: { alt: /./ } } ) + .consuming( { attribute: [ 'alt' ] } ) + .toAttribute( ( viewImage ) => ( { key: 'alt', value: viewImage.getAttribute( 'alt' ) } ) ); + // Converter for figure element from view to model. - data.viewToModel.on( 'element:figure', viewToModelImage() ); + data.viewToModel.on( 'element:figure', viewFigureToModel() ); } } diff --git a/src/image/ui/imageballoonpanelview.js b/src/image/ui/imageballoonpanelview.js index 6bd8939b..32bd19fd 100644 --- a/src/image/ui/imageballoonpanelview.js +++ b/src/image/ui/imageballoonpanelview.js @@ -95,7 +95,7 @@ export default class ImageBalloonPanelView extends BalloonPanelView { const defaultPositions = BalloonPanelView.defaultPositions; this.attachTo( { - target: editingView.domConverter.viewRangeToDom( editingView.selection.getFirstRange() ), + target: editingView.domConverter.viewToDom( editingView.selection.getSelectedElement() ), positions: [ defaultPositions.northArrowSouth, defaultPositions.southArrowNorth ] } ); } diff --git a/tests/image/converters.js b/tests/image/converters.js index 89e27542..e88f5507 100644 --- a/tests/image/converters.js +++ b/tests/image/converters.js @@ -3,11 +3,12 @@ * For licensing, see LICENSE.md. */ -import { viewToModelImage, createImageAttributeConverter } from '../../src/image/converters'; +import { viewFigureToModel, createImageAttributeConverter } from '../../src/image/converters'; import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor'; import { createImageViewElement } from '../../src/image/imageengine'; import { toImageWidget } from '../../src/image/utils'; import buildModelConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildmodelconverter'; +import buildViewConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildviewconverter'; import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element'; import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; import { setData as setModelData, getData as getModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; @@ -28,10 +29,6 @@ describe( 'Image converters', () => { schema.allow( { name: 'image', attributes: [ 'alt', 'src' ], inside: '$root' } ); schema.objects.add( 'image' ); - buildModelConverter().for( ) - .fromElement( 'image' ) - .toElement( () => toImageWidget( createImageViewElement() ) ); - buildModelConverter().for( editor.editing.modelToView ) .fromElement( 'image' ) .toElement( () => toImageWidget( createImageViewElement() ) ); @@ -41,46 +38,87 @@ describe( 'Image converters', () => { } ); } ); - describe( 'viewToModelImage', () => { - let dispatcher, schema; + describe( 'viewFigureToModel', () => { + function expectModel( model ) { + expect( getModelData( document, { withoutSelection: true } ) ).to.equal( model ); + } + + let dispatcher, schema, imgConverterCalled; beforeEach( () => { + imgConverterCalled = false; + schema = document.schema; + schema.allow( { name: '$text', inside: 'image' } ); + dispatcher = editor.data.viewToModel; - dispatcher.on( 'element:figure', viewToModelImage() ); + dispatcher.on( 'element:figure', viewFigureToModel() ); + dispatcher.on( 'element:img', ( evt, data, consumable ) => { + if ( consumable.consume( data.input, { name: true, attribute: 'src' } ) ) { + data.output = new ModelElement( 'image', { src: data.input.getAttribute( 'src' ) } ); + + imgConverterCalled = true; + } + } ); } ); - it( 'should convert view figure element', () => { - editor.setData( '
bar baz
' ); - expect( getModelData( document, { withoutSelection: true } ) ).to.equal( 'bar baz' ); + it( 'should find img element among children and convert it using already defined converters', () => { + editor.setData( '
' ); + + expectModel( '' ); + expect( imgConverterCalled ).to.be.true; } ); - it( 'should convert without alt', () => { - editor.setData( '
' ); - expect( getModelData( document, { withoutSelection: true } ) ).to.equal( '' ); + it( 'should convert non-img children in image context and append them to model image element', () => { + buildViewConverter().for( editor.data.viewToModel ).fromElement( 'foo' ).toElement( 'foo' ); + buildViewConverter().for( editor.data.viewToModel ).fromElement( 'bar' ).toElement( 'bar' ); + + schema.registerItem( 'foo' ); + schema.registerItem( 'bar' ); + + schema.allow( { name: 'foo', inside: 'image' } ); + + editor.setData( '
xy
' ); + + // Element bar not converted because schema does not allow it. + expectModel( 'xy' ); } ); - it( 'should not convert if figure element is already consumed', () => { + it( 'should be possible to overwrite', () => { dispatcher.on( 'element:figure', ( evt, data, consumable ) => { - consumable.consume( data.input, { name: true, class: 'image' } ); + consumable.consume( data.input, { name: true } ); + consumable.consume( data.input.getChild( 0 ), { name: true } ); - data.output = new ModelElement( 'not-image' ); + data.output = new ModelElement( 'myImage', { data: { src: data.input.getChild( 0 ).getAttribute( 'src' ) } } ); }, { priority: 'high' } ); - editor.setData( '
bar baz
' ); - expect( getModelData( document, { withoutSelection: true } ) ).to.equal( '' ); + editor.setData( '
xyz
' ); + + expectModel( '' ); } ); - it( 'should not convert image if schema disallows it', () => { - schema.disallow( { name: 'image', attributes: [ 'alt', 'src' ], inside: '$root' } ); + // Test exactly what figure converter does, which is putting it's children element to image element. + // If this has not been done, it means that figure converter was not used. + it( 'should not convert if figure do not have class="image" attribute', () => { + editor.setData( '
xyz
' ); - editor.setData( '
' ); - expect( getModelData( document, { withoutSelection: true } ) ).to.equal( '' ); + // Default image converter will be fired. + expectModel( '' ); } ); - it( 'should not convert image if there is no img element', () => { - editor.setData( '
' ); - expect( getModelData( document, { withoutSelection: true } ) ).to.equal( '' ); + it( 'should not convert if there is no img element among children', () => { + editor.setData( '
xyz
' ); + + // Figure converter outputs nothing and text is disallowed in root. + expectModel( '' ); + } ); + + it( 'should not convert if img element was not converted', () => { + // Image element missing src attribute. + editor.setData( '
abcxyz
' ); + + // Figure converter outputs nothing and text is disallowed in root. + expectModel( '' ); } ); } ); diff --git a/tests/image/imageengine.js b/tests/image/imageengine.js index 745b8fa6..adf38f8f 100644 --- a/tests/image/imageengine.js +++ b/tests/image/imageengine.js @@ -58,7 +58,7 @@ describe( 'ImageEngine', () => { } ); it( 'should not convert if there is no image class', () => { - editor.setData( '
alt text
' ); + editor.setData( '
My quote
' ); expect( getModelData( document, { withoutSelection: true } ) ) .to.equal( '' ); @@ -140,6 +140,36 @@ describe( 'ImageEngine', () => { sinon.assert.calledOnce( conversionSpy ); } ); + + it( 'should convert bare img element', () => { + editor.setData( 'alt text' ); + + expect( getModelData( document, { withoutSelection: true } ) ) + .to.equal( 'alt text' ); + } ); + + it( 'should not convert alt attribute on non-img element', () => { + const data = editor.data; + const editing = editor.editing; + + document.schema.registerItem( 'div', '$block' ); + document.schema.allow( { name: 'div', attributes: 'alt', inside: '$root' } ); + + buildModelConverter().for( data.modelToView, editing.modelToView ).fromElement( 'div' ).toElement( 'div' ); + buildViewConverter().for( data.viewToModel ).fromElement( 'div' ).toElement( 'div' ); + + editor.setData( '
' ); + + expect( getModelData( document, { withoutSelection: true } ) ).to.equal( '
' ); + } ); + + it( 'should handle figure with two images', () => { + document.schema.allow( { name: '$text', inside: 'image' } ); + + editor.setData( '
abc
' ); + + expect( getModelData( document, { withoutSelection: true } ) ).to.equal( 'abc' ); + } ); } ); } ); diff --git a/tests/image/ui/imageballoonpanelview.js b/tests/image/ui/imageballoonpanelview.js index 790f18d8..8ce7a255 100644 --- a/tests/image/ui/imageballoonpanelview.js +++ b/tests/image/ui/imageballoonpanelview.js @@ -67,6 +67,7 @@ describe( 'ImageBalloonPanel', () => { it( 'should detach when image element is no longer selected', () => { const spy = sinon.spy( panel, 'detach' ); + setData( document, '[]' ); panel.attach(); @@ -79,13 +80,17 @@ describe( 'ImageBalloonPanel', () => { it( 'should attach panel correctly', () => { const spy = sinon.spy( panel, 'attachTo' ); + + setData( document, '[]' ); panel.attach(); testPanelAttach( spy ); } ); it( 'should calculate panel position on scroll event', () => { + setData( document, '[]' ); panel.attach(); + const spy = sinon.spy( panel, 'attachTo' ); global.window.dispatchEvent( new Event( 'scroll' ) ); @@ -94,7 +99,9 @@ describe( 'ImageBalloonPanel', () => { } ); it( 'should calculate panel position on resize event event', () => { + setData( document, '[]' ); panel.attach(); + const spy = sinon.spy( panel, 'attachTo' ); global.window.dispatchEvent( new Event( 'resize' ) ); @@ -103,7 +110,9 @@ describe( 'ImageBalloonPanel', () => { } ); it( 'should hide panel on detach', () => { + setData( document, '[]' ); panel.attach(); + const spy = sinon.spy( panel, 'hide' ); panel.detach(); @@ -112,7 +121,9 @@ describe( 'ImageBalloonPanel', () => { } ); it( 'should not reposition panel after detaching', () => { + setData( document, '[]' ); panel.attach(); + const spy = sinon.spy( panel, 'attachTo' ); panel.detach(); @@ -124,16 +135,11 @@ describe( 'ImageBalloonPanel', () => { // Tests if panel.attachTo() was called correctly. function testPanelAttach( spy ) { - const domRange = editor.editing.view.domConverter.viewRangeToDom( editingView.selection.getFirstRange() ); - sinon.assert.calledOnce( spy ); const options = spy.firstCall.args[ 0 ]; - // Check if proper range was used. - expect( options.target.startContainer ).to.equal( domRange.startContainer ); - expect( options.target.startOffset ).to.equal( domRange.startOffset ); - expect( options.target.endContainer ).to.equal( domRange.endContainer ); - expect( options.target.endOffset ).to.equal( domRange.endOffset ); + // Check if proper target element was used. + expect( options.target.tagName.toLowerCase() ).to.equal( 'figure' ); // Check if correct positions are used. const [ north, south ] = options.positions;