Explorar o código

Internal: Removed autohoisting converters in favor of conversion builder.

Oskar Wróbel %!s(int64=8) %!d(string=hai) anos
pai
achega
7337fc032c

+ 11 - 176
packages/ckeditor5-image/src/image/converters.js

@@ -7,7 +7,9 @@
  * @module image/image/converters
  */
 
-import ModelDocumentFragment from '@ckeditor/ckeditor5-engine/src/model/documentfragment';
+import ModelPosition from '@ckeditor/ckeditor5-engine/src/model/position';
+import ModelRange from '@ckeditor/ckeditor5-engine/src/model/range';
+import first from '@ckeditor/ckeditor5-utils/src/first';
 
 /**
  * Returns a function that converts the image view representation:
@@ -30,8 +32,8 @@ export function viewFigureToModel() {
 			return;
 		}
 
-		// Do not convert if image cannot be placed in model at this context.
-		if ( !conversionApi.schema.checkChild( data.context, 'image' ) ) {
+		// Do not convert if image cannot be placed in model at current position.
+		if ( !conversionApi.schema.checkChild( data.position, 'image' ) ) {
 			return;
 		}
 
@@ -44,21 +46,16 @@ export function viewFigureToModel() {
 		}
 
 		// Convert view image to model image.
-		const modelImage = conversionApi.convertItem( viewImage, consumable, data );
+		const output = conversionApi.convertItem( viewImage, consumable, data.position );
 
-		// 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 );
+		// Get image element from conversion output.
+		const modelImage = first( output.getItems() );
 
-		const modelChildren = conversionApi.convertChildren( data.input, consumable, data );
-
-		data.context.pop();
-
-		// Add converted children to model image.
-		conversionApi.writer.insert( modelChildren, modelImage );
+		// Convert rest of the figure element's children as an image children.
+		conversionApi.convertChildren( data.input, consumable, ModelPosition.createAt( modelImage ) );
 
 		// Set model image as conversion result.
-		data.output = modelImage;
+		data.output = ModelRange.createOn( modelImage );
 	};
 }
 
@@ -144,165 +141,3 @@ function modelToViewAttributeConverter() {
 		}
 	};
 }
-
-// Holds all images that were converted for autohoisting.
-const autohoistedImages = new WeakSet();
-
-/**
- * A converter which converts `<img>` {@link module:engine/view/element~Element view elements} that can be hoisted.
- *
- * If an `<img>` view element has not been converted, this converter checks if that element could be converted in any
- * context "above". If it could, the converter converts the `<img>` element even though it is not allowed in the current
- * context and marks it to be autohoisted. Then {@link module:image/image/converters~hoistImageThroughElement another converter}
- * moves the converted element to the correct location.
- */
-export function convertHoistableImage( evt, data, consumable, conversionApi ) {
-	const img = data.input;
-
-	// If the image has not been consumed (converted)...
-	if ( !consumable.test( img, { name: true, attribute: [ 'src' ] } ) ) {
-		return;
-	}
-	// At this point the image has not been converted because it was not allowed by schema. It might be in wrong
-	// context or missing an attribute, but above we already checked whether the image has mandatory src attribute.
-
-	// If the image would be allowed if it was in one of its ancestors...
-	const allowedContext = _findAllowedContext( { name: 'image', attributes: [ 'src' ] }, data.context, conversionApi.schema );
-
-	if ( !allowedContext ) {
-		return;
-	}
-
-	// Convert it in that context...
-	const newData = Object.assign( {}, data );
-	newData.context = allowedContext;
-
-	data.output = conversionApi.convertItem( img, consumable, newData );
-
-	// And mark that image to be hoisted.
-	autohoistedImages.add( data.output );
-}
-
-// Basing on passed `context`, searches for "closest" context in which model element represented by `modelData`
-// would be allowed by `schema`.
-//
-// @private
-// @param {Object} modelData Object describing model element to check. Has two properties: `name` with model element name
-// and `attributes` with keys of attributes of that model element.
-// @param {Array} context Context in which original conversion was supposed to take place.
-// @param {module:engine/model/schema~Schema} schema Schema to check with.
-// @returns {Array|null} Context in which described model element would be allowed by `schema` or `null` if such context
-// could not been found.
-function _findAllowedContext( modelData, context, schema ) {
-	// Copy context array so we won't modify original array.
-	context = context.slice();
-
-	// Try out all possible contexts.
-	while ( context.length && !schema.checkChild( context, modelData.name ) ) {
-		const parent = context.pop();
-		const parentName = typeof parent === 'string' ? parent : parent.name;
-
-		// Do not try to autohoist "above" limiting element.
-		if ( schema.isLimit( parentName ) ) {
-			return null;
-		}
-	}
-
-	// If `context` has any items it means that image is allowed in that context. Return that context.
-	// If `context` has no items it means that image was not allowed in any of possible contexts. Return `null`.
-	return context.length ? context : null;
-}
-
-/**
- * A converter which hoists `<image>` {@link module:engine/model/element~Element model elements} to allowed context.
- *
- * It looks through all children of the converted {@link module:engine/view/element~Element view element} if it
- * was converted to a model element. It breaks the model element if an `<image>` to-be-hoisted is found.
- *
- *		<div><paragraph>x<image src="foo.jpg"></image>x</paragraph></div> ->
- *		<div><paragraph>x</paragraph></div><image src="foo.jpg"></image><div><paragraph>x</paragraph></div>
- *
- * This works deeply, as shown in the example. This converter added for the `<paragraph>` element will break the `<paragraph>`
- *  element and pass the {@link module:engine/model/documentfragment~DocumentFragment document fragment} in `data.output`.
- *  Then, the `<div>` will be handled by this converter and will be once again broken to hoist the `<image>` up to the root.
- *
- * **Note:** This converter should be executed only after the view element has already been converted, which means that
- * `data.output` for that view element should be already generated when this converter is fired.
- */
-export function hoistImageThroughElement( evt, data ) {
-	// If this element has been properly converted...
-	if ( !data.output ) {
-		return;
-	}
-
-	// And it is an element...
-	// (If it is document fragment autohoisting does not have to break anything anyway.)
-	// (And if it is text there are no children here.)
-	if ( !data.output.is( 'element' ) ) {
-		return;
-	}
-
-	// This will hold newly generated output. At the beginning it is only the original element.
-	const newOutput = [];
-
-	// Check if any of its children is to be hoisted...
-	// Start from the last child - it is easier to break that way.
-	for ( let i = data.output.childCount - 1; i >= 0; i-- ) {
-		const child = data.output.getChild( i );
-
-		if ( autohoistedImages.has( child ) ) {
-			// Break autohoisted element's parent:
-			// <parent>{ left-children... }<authoistedElement />{ right-children... }</parent>   --->
-			// <parent>{ left-children... }</parent><autohoistedElement /><parent>{ right-children... }</parent>
-			//
-			// or
-			//
-			// <parent>{ left-children... }<autohoistedElement /></parent> --->
-			// <parent>{ left-children... }</parent><autohoistedElement />
-			//
-			// or
-			//
-			// <parent><autohoistedElement />{ right-children... }</parent> --->
-			// <autohoistedElement /><parent>{ right-children... }</parent>
-			//
-			// or
-			//
-			// <parent><autohoistedElement /></parent> ---> <autohoistedElement />
-
-			// Check how many right-children there are.
-			const rightChildrenCount = data.output.childCount - i - 1;
-			let rightParent = null;
-
-			// If there are any right-children, clone the prent element and insert those children there.
-			if ( rightChildrenCount > 0 ) {
-				rightParent = data.output.clone( false );
-				rightParent.appendChildren( data.output.removeChildren( i + 1, rightChildrenCount ) );
-			}
-
-			// Remove the autohoisted element from its parent.
-			child.remove();
-
-			// Break "leading" `data.output` in `newOutput` into one or more pieces:
-			// Remove "leading" `data.output` (note that `data.output` is always first item in `newOutput`).
-			newOutput.shift();
-
-			// Add the newly created parent of the right-children at the beginning.
-			if ( rightParent ) {
-				newOutput.unshift( rightParent );
-			}
-
-			// Add autohoisted element at the beginning.
-			newOutput.unshift( child );
-
-			// Add `data.output` at the beginning, if there is anything left in it.
-			if ( data.output.childCount > 0 ) {
-				newOutput.unshift( data.output );
-			}
-		}
-	}
-
-	// If the output has changed pass it further.
-	if ( newOutput.length ) {
-		data.output = new ModelDocumentFragment( newOutput );
-	}
-}

+ 0 - 7
packages/ckeditor5-image/src/image/imageengine.js

@@ -13,8 +13,6 @@ import buildViewConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildv
 import {
 	viewFigureToModel,
 	createImageAttributeConverter,
-	convertHoistableImage,
-	hoistImageThroughElement,
 	srcsetAttributeConverter
 } from './converters';
 import { toImageWidget } from './utils';
@@ -69,12 +67,7 @@ export default class ImageEngine extends Plugin {
 			.from( { name: 'img', attribute: { src: true } } )
 			.toElement( viewImage => new ModelElement( 'image', { src: viewImage.getAttribute( 'src' ) } ) );
 
-		data.viewToModel.on( 'element:img', convertHoistableImage, { priority: 'low' } );
-		data.viewToModel.on( 'element', hoistImageThroughElement, { priority: 'low' } );
-
 		// Build converter for alt attribute.
-		// Note that by default attribute converters are added with `low` priority.
-		// This converter will be thus fired after `convertHoistableImage` converter.
 		buildViewConverter().for( data.viewToModel )
 			.from( { name: 'img', attribute: { alt: true } } )
 			.consuming( { attribute: [ 'alt' ] } )

+ 17 - 33
packages/ckeditor5-image/src/imagestyle/converters.js

@@ -7,8 +7,6 @@
  * @module image/imagestyle/converters
  */
 
-import { isImage } from '../image/utils';
-
 /**
  * Returns a converter for the `imageStyle` attribute. It can be used for adding, changing and removing the attribute.
  *
@@ -49,41 +47,27 @@ export function viewToModelStyleAttribute( styles ) {
 	const filteredStyles = styles.filter( style => !style.isDefault );
 
 	return ( evt, data, consumable, conversionApi ) => {
-		for ( const style of filteredStyles ) {
-			viewToModelImageStyle( style, data, consumable, conversionApi );
+		if ( !data.output ) {
+			return;
 		}
-	};
-}
-
-// Converter from view to model converting single style.
-// For more information see {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher};
-//
-// @param {module:image/imagestyle/imagestyleengine~ImageStyleFormat} style
-// @param {Object} data
-// @param {module:engine/conversion/viewconsumable~ViewConsumable} consumable
-// @param {Object} conversionApi
-function viewToModelImageStyle( style, data, consumable, conversionApi ) {
-	const viewFigureElement = data.input;
-	const modelImageElement = data.output;
-
-	// *** Step 1: Validate conversion.
-	// Check if view element has proper class to consume.
-	if ( !consumable.test( viewFigureElement, { class: style.className } ) ) {
-		return;
-	}
 
-	// Check if figure is converted to image.
-	if ( !isImage( modelImageElement ) ) {
-		return;
-	}
+		const viewFigureElement = data.input;
+		const modelImageElement = data.output.getItems().next().value;
 
-	if ( !conversionApi.schema.checkAttribute( [ ...data.context, 'image' ], 'imageStyle' ) ) {
-		return;
-	}
+		// Check if `imageStyle` attribute is allowed for current element.
+		if ( !conversionApi.schema.checkAttribute( modelImageElement, 'imageStyle' ) ) {
+			return;
+		}
 
-	// *** Step2: Convert to model.
-	consumable.consume( viewFigureElement, { class: style.className } );
-	modelImageElement.setAttribute( 'imageStyle', style.name );
+		// Convert style one by one.
+		for ( const style of filteredStyles ) {
+			// Try to consume class corresponding with style.
+			if ( consumable.consume( viewFigureElement, { class: style.className } ) ) {
+				// And convert this style to model attribute.
+				conversionApi.writer.setAttribute( 'imageStyle', style.name, modelImageElement );
+			}
+		}
+	};
 }
 
 // Returns style with given `name` from array of styles.

+ 15 - 144
packages/ckeditor5-image/tests/image/converters.js

@@ -6,18 +6,13 @@
 import {
 	viewFigureToModel,
 	createImageAttributeConverter,
-	convertHoistableImage,
-	hoistImageThroughElement
 } 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 ViewEmptyElement from '@ckeditor/ckeditor5-engine/src/view/emptyelement';
-import ViewContainerElement from '@ckeditor/ckeditor5-engine/src/view/containerelement';
-import { convertToModelFragment } from '@ckeditor/ckeditor5-engine/src/conversion/view-to-model-converters';
+import ModelRange from '@ckeditor/ckeditor5-engine/src/model/range';
 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';
 
@@ -65,9 +60,13 @@ describe( 'Image converters', () => {
 
 			dispatcher = editor.data.viewToModel;
 			dispatcher.on( 'element:figure', viewFigureToModel() );
-			dispatcher.on( 'element:img', ( evt, data, consumable ) => {
+			dispatcher.on( 'element:img', ( evt, data, consumable, conversionApi ) => {
 				if ( consumable.consume( data.input, { name: true, attribute: 'src' } ) ) {
-					data.output = new ModelElement( 'image', { src: data.input.getAttribute( 'src' ) } );
+					const image = conversionApi.writer.createElement( 'image', { src: data.input.getAttribute( 'src' ) } );
+
+					conversionApi.writer.insert( image, data.position );
+
+					data.output = ModelRange.createOn( image );
 
 					imgConverterCalled = true;
 				}
@@ -97,11 +96,17 @@ describe( 'Image converters', () => {
 		} );
 
 		it( 'should be possible to overwrite', () => {
-			dispatcher.on( 'element:figure', ( evt, data, consumable ) => {
+			dispatcher.on( 'element:figure', ( evt, data, consumable, conversionApi ) => {
 				consumable.consume( data.input, { name: true } );
 				consumable.consume( data.input.getChild( 0 ), { name: true } );
 
-				data.output = new ModelElement( 'myImage', { data: { src: data.input.getChild( 0 ).getAttribute( 'src' ) } } );
+				const element = conversionApi.writer.createElement( 'myImage', {
+					data: {
+						src: data.input.getChild( 0 ).getAttribute( 'src' )
+					}
+				} );
+				conversionApi.writer.insert( element, data.position );
+				data.output = ModelRange.createOn( element );
 			}, { priority: 'high' } );
 
 			editor.setData( '<figure class="image"><img src="foo.png" />xyz</figure>' );
@@ -186,138 +191,4 @@ describe( 'Image converters', () => {
 			);
 		} );
 	} );
-
-	// More integration tests are in imageengine.js.
-	describe( 'autohoist converters', () => {
-		let dispatcher, schema, imgConverterCalled, viewImg, modelDiv, modelParagraph, modelLimit;
-
-		beforeEach( () => {
-			imgConverterCalled = false;
-
-			schema = model.schema;
-
-			dispatcher = editor.data.viewToModel;
-			dispatcher.on( 'element:img', ( evt, data, consumable ) => {
-				if ( !schema.checkChild( data.context, 'image' ) ) {
-					return;
-				}
-
-				if ( consumable.consume( data.input, { name: true, attribute: 'src' } ) ) {
-					data.output = new ModelElement( 'image', { src: data.input.getAttribute( 'src' ) } );
-
-					imgConverterCalled = true;
-				}
-			} );
-
-			// Make sure to pass document element to the converter and fire this callback after "normal" image callback.
-			dispatcher.on( 'element:img', convertHoistableImage, { priority: 'low' } );
-
-			viewImg = new ViewEmptyElement( 'img', { src: 'foo.jpg' } );
-
-			modelDiv = new ModelElement( 'div' );
-			modelParagraph = new ModelElement( 'paragraph' );
-			modelLimit = new ModelElement( 'limit' );
-		} );
-
-		describe( 'convertHoistableImage', () => {
-			it( 'should convert img element using already added converters if it is allowed in any point of given context #1', () => {
-				const result = dispatcher.convert( viewImg, { context: [ '$root', 'div', 'paragraph' ] } );
-
-				// `result` is a model document fragment.
-				expect( result.childCount ).to.equal( 1 );
-				expect( result.getChild( 0 ).is( 'image' ) ).to.be.true;
-				expect( result.getChild( 0 ).getAttribute( 'src' ) ).to.equal( 'foo.jpg' );
-				expect( imgConverterCalled ).to.be.true;
-			} );
-
-			it( 'should convert img element using already added converters if it is allowed in any point of given context #2', () => {
-				const result = dispatcher.convert( viewImg, { context: [ '$root', modelDiv, modelParagraph ] } );
-
-				// `result` is a model document fragment.
-				expect( result.childCount ).to.equal( 1 );
-				expect( result.getChild( 0 ).is( 'image' ) ).to.be.true;
-				expect( result.getChild( 0 ).getAttribute( 'src' ) ).to.equal( 'foo.jpg' );
-				expect( imgConverterCalled ).to.be.true;
-			} );
-
-			it( 'should not convert img element if there is no allowed context #1', () => {
-				const result = dispatcher.convert( viewImg, { context: [ 'div', 'paragraph' ] } );
-
-				// `result` is an empty model document fragment.
-				expect( result.childCount ).to.equal( 0 );
-				expect( imgConverterCalled ).to.be.false;
-			} );
-
-			it( 'should not convert img element if there is no allowed context #2', () => {
-				const result = dispatcher.convert( viewImg, { context: [ modelDiv, modelParagraph ] } );
-
-				// `result` is an empty model document fragment.
-				expect( result.childCount ).to.equal( 0 );
-				expect( imgConverterCalled ).to.be.false;
-			} );
-
-			it( 'should not convert img element if allowed context is "above" limiting element #1', () => {
-				schema.register( 'limit', { isLimit: true } );
-
-				const result = dispatcher.convert( viewImg, { context: [ '$root', 'limit', 'div', 'paragraph' ] } );
-
-				// `result` is an empty model document fragment.
-				expect( result.childCount ).to.equal( 0 );
-				expect( imgConverterCalled ).to.be.false;
-			} );
-
-			it( 'should not convert img element if allowed context is "above" limiting element #2', () => {
-				schema.register( 'limit', { isLimit: true } );
-
-				const result = dispatcher.convert( viewImg, { context: [ '$root', modelLimit, modelDiv, modelParagraph ] } );
-
-				// `result` is an empty model document fragment.
-				expect( result.childCount ).to.equal( 0 );
-				expect( imgConverterCalled ).to.be.false;
-			} );
-		} );
-
-		describe( 'hoistImageThroughElement', () => {
-			it( 'should hoist img element that was converted by convertHoistableImage', () => {
-				schema.register( 'div', { inheritAllFrom: '$block' } );
-
-				buildModelConverter().for( editor.data.modelToView, editor.editing.modelToView ).fromElement( 'div' ).toElement( 'div' );
-				buildViewConverter().for( editor.data.viewToModel ).fromElement( 'div' ).toElement( 'div' );
-
-				// Make sure to fire this callback after "normal" div callback.
-				dispatcher.on( 'element:div', hoistImageThroughElement, { priority: 'low' } );
-
-				// If img view element is converted it must have been converted thanks to convertHoistableImage,
-				// because image is not allowed in div.
-				const viewDiv = new ViewContainerElement( 'div', null, [ 'foo', viewImg, 'bar' ] );
-
-				const result = dispatcher.convert( viewDiv, { context: [ '$root' ] } );
-
-				// `result` is a model document fragment.
-				expect( result.childCount ).to.equal( 3 );
-				expect( result.getChild( 0 ).is( 'div' ) ).to.be.true;
-				expect( result.getChild( 1 ).is( 'image' ) ).to.be.true;
-				expect( result.getChild( 2 ).is( 'div' ) ).to.be.true;
-			} );
-
-			it( 'should work fine with elements that are converted to document fragments', () => {
-				// The example here is <p> that contains some text and an <img> and is converted in $root > div context.
-				// This way we enable autohoisting for <img> and test how it will work when <p> is converted to model document fragment.
-				schema.register( 'div', { inheritAllFrom: '$block' } );
-
-				dispatcher.on( 'element:p', convertToModelFragment() );
-				dispatcher.on( 'element:p', hoistImageThroughElement, { priority: 'low' } );
-
-				const viewDiv = new ViewContainerElement( 'p', null, [ 'foo', viewImg, 'bar' ] );
-
-				const result = dispatcher.convert( viewDiv, { context: [ '$root', 'div' ] } );
-
-				// `result` is a model document fragment.
-				expect( result.childCount ).to.equal( 3 );
-				expect( result.getChild( 0 ).is( 'text' ) ).to.be.true;
-				expect( result.getChild( 1 ).is( 'image' ) ).to.be.true;
-				expect( result.getChild( 2 ).is( 'text' ) ).to.be.true;
-			} );
-		} );
-	} );
 } );