From 641725d9ed9253fae1b038ba33da1cb35db634c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Thu, 25 Jan 2018 18:05:42 +0100 Subject: [PATCH 01/42] Allowed SchemaContext as a checkChild and checkAttribute context. --- src/model/schema.js | 17 ++++++++++++----- tests/model/schema.js | 19 +++++++++++++++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/model/schema.js b/src/model/schema.js index f10021d06..07093e3c2 100644 --- a/src/model/schema.js +++ b/src/model/schema.js @@ -193,11 +193,16 @@ export default class Schema { this.decorate( 'checkAttribute' ); this.on( 'checkAttribute', ( evt, args ) => { - args[ 0 ] = new SchemaContext( args[ 0 ] ); + if ( !( args[ 0 ] instanceof SchemaContext ) ) { + args[ 0 ] = new SchemaContext( args[ 0 ] ); + } }, { priority: 'highest' } ); this.on( 'checkChild', ( evt, args ) => { - args[ 0 ] = new SchemaContext( args[ 0 ] ); + if ( !( args[ 0 ] instanceof SchemaContext ) ) { + args[ 0 ] = new SchemaContext( args[ 0 ] ); + } + args[ 1 ] = this.getDefinition( args[ 1 ] ); }, { priority: 'highest' } ); } @@ -375,8 +380,9 @@ export default class Schema { * schema.checkChild( model.document.getRoot(), paragraph ); // -> true * * @fires checkChild - * @param {module:engine/model/schema~SchemaContextDefinition} context Context in which the child will be checked. - * @param {module:engine/model/node~Node|String} child The child to check. + * @param {module:engine/model/schema~SchemaContextDefinition|module:engine/model/schema~SchemaContext} context + * Context in which the child will be checked. + * @param {module:engine/model/node~Node|String} def The child to check. */ checkChild( context, def ) { // Note: context and child are already normalized here to a SchemaContext and SchemaCompiledItemDefinition. @@ -399,7 +405,8 @@ export default class Schema { * schema.checkAttribute( textNode, 'bold' ); // -> true * * @fires checkAttribute - * @param {module:engine/model/schema~SchemaContextDefinition} context + * @param {module:engine/model/schema~SchemaContextDefinition|module:engine/model/schema~SchemaContext} context + * Context in which the attribute will be checked. * @param {String} attributeName */ checkAttribute( context, attributeName ) { diff --git a/tests/model/schema.js b/tests/model/schema.js index 32b3ef716..e835991e4 100644 --- a/tests/model/schema.js +++ b/tests/model/schema.js @@ -377,6 +377,17 @@ describe( 'Schema', () => { expect( schema.checkChild( root1, '$text' ) ).to.be.false; } ); + it( 'accepts a schemaContext instance as a context', () => { + const rootContext = new SchemaContext( Position.createAt( root1 ) ); + const paragraphContext = new SchemaContext( Position.createAt( r1p1 ) ); + + expect( schema.checkChild( rootContext, 'paragraph' ) ).to.be.true; + expect( schema.checkChild( rootContext, '$text' ) ).to.be.false; + + expect( schema.checkChild( paragraphContext, '$text' ) ).to.be.true; + expect( schema.checkChild( paragraphContext, 'paragraph' ) ).to.be.false; + } ); + it( 'accepts a position as a context', () => { const posInRoot = Position.createAt( root1 ); const posInParagraph = Position.createAt( r1p1 ); @@ -466,6 +477,14 @@ describe( 'Schema', () => { expect( schema.checkAttribute( posInParagraph, 'align' ) ).to.be.true; } ); + it( 'accepts a schemaContext instance as a context', () => { + const rootContext = new SchemaContext( Position.createAt( root1 ) ); + const paragraphContext = new SchemaContext( Position.createAt( r1p1 ) ); + + expect( schema.checkAttribute( rootContext, 'align' ) ).to.be.false; + expect( schema.checkAttribute( paragraphContext, 'align' ) ).to.be.true; + } ); + it( 'accepts an array of node names as a context', () => { const contextInRoot = [ '$root' ]; const contextInParagraph = [ '$root', 'paragraph' ]; From 33854314f171d8325191cf9465084942ea442bd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Thu, 25 Jan 2018 18:19:31 +0100 Subject: [PATCH 02/42] Added `SchemaContext#addItem` method. --- src/model/schema.js | 9 +++++++++ tests/model/schema.js | 30 ++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/src/model/schema.js b/src/model/schema.js index 07093e3c2..0f0e27f59 100644 --- a/src/model/schema.js +++ b/src/model/schema.js @@ -1090,6 +1090,15 @@ export class SchemaContext { return this._items[ Symbol.iterator ](); } + /** + * Adds new item at the to of the context. + * + * @param {module:engine/model/node~Node|String} item Item to add. + */ + addItem( item ) { + this._items.push( mapContextItem( item ) ); + } + /** * Gets an item on the given index. * diff --git a/tests/model/schema.js b/tests/model/schema.js index e835991e4..b9ef922d0 100644 --- a/tests/model/schema.js +++ b/tests/model/schema.js @@ -2459,6 +2459,36 @@ describe( 'SchemaContext', () => { } ); } ); + describe( 'addItem()', () => { + it( 'adds new item at the top of the context #text', () => { + const node = new Text( 'd' ); + + const ctx = new SchemaContext( [ 'a', 'b', 'c' ] ); + + ctx.addItem( node ); + + expect( Array.from( ctx ).map( item => item.name ) ).to.deep.equal( [ 'a', 'b', 'c', '$text' ] ); + } ); + + it( 'adds new item at the top of the context #string', () => { + const ctx = new SchemaContext( [ 'a', 'b', 'c' ] ); + + ctx.addItem( 'd' ); + + expect( Array.from( ctx ).map( item => item.name ) ).to.deep.equal( [ 'a', 'b', 'c', 'd' ] ); + } ); + + it( 'adds new item at the top of the context #node', () => { + const node = new Element( 'd' ); + + const ctx = new SchemaContext( [ 'a', 'b', 'c' ] ); + + ctx.addItem( node ); + + expect( Array.from( ctx ).map( item => item.name ) ).to.deep.equal( [ 'a', 'b', 'c', 'd' ] ); + } ); + } ); + describe( 'getNames()', () => { it( 'returns an iterator', () => { const ctx = new SchemaContext( [ 'a', 'b', 'c' ] ); From 85cb06c247377decac6f9efa143d058604df7bf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Fri, 26 Jan 2018 11:18:49 +0100 Subject: [PATCH 03/42] Added Schema#findAllowedParent method. --- src/model/schema.js | 33 +++++++++++++++++++++ tests/model/schema.js | 67 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+) diff --git a/src/model/schema.js b/src/model/schema.js index 0f0e27f59..cb8f679b3 100644 --- a/src/model/schema.js +++ b/src/model/schema.js @@ -674,6 +674,39 @@ export default class Schema { return validRanges; } + /** + * Tries to find position ancestors that allows to insert given node. + * It starts searching from the given position and goes node by node to the top of the model tree + * as long as {@link module:engine/model/schema~Schema#isLimit limit element} or top-most ancestor won't be reached. + * + * @params {module:engine/model/node~Node} node Node for which allowed parent should be found. + * @params {module:engine/model/position~Position} position Position from searching will start. + * @params {Function} [limitChecker] When function is defined and returns true + * then stops searching and returns null as a search result. Function gets current parent as a parameter. + * @returns {module:engine/model/element~Element|null} element Allowed parent or null if nothing was found. + */ + findAllowedParent( node, position, limitChecker ) { + let parent = position.parent; + + while ( parent ) { + if ( this.checkChild( parent, node ) ) { + return parent; + } + + if ( this.isLimit( parent ) ) { + return null; + } + + if ( typeof limitChecker == 'function' && limitChecker( parent ) ) { + return null; + } + + parent = parent.parent; + } + + return null; + } + /** * Removes attributes disallowed by the schema. * diff --git a/tests/model/schema.js b/tests/model/schema.js index b9ef922d0..55123fe00 100644 --- a/tests/model/schema.js +++ b/tests/model/schema.js @@ -1137,6 +1137,73 @@ describe( 'Schema', () => { } ); } ); + describe( 'findAllowedParent', () => { + beforeEach( () => { + schema.register( '$root' ); + schema.register( 'blockQuote', { + allowIn: '$root' + } ); + schema.register( 'paragraph', { + allowIn: 'blockQuote' + } ); + schema.register( '$text', { + allowIn: 'paragraph' + } ); + } ); + + it( 'should return position ancestor that allows to insert given note to it', () => { + const node = new Element( 'paragraph' ); + + const allowedParent = schema.findAllowedParent( node, Position.createAt( r1bQp ) ); + + expect( allowedParent ).to.equal( r1bQ ); + } ); + + it( 'should return position ancestor that allows to insert given note to it when position is already i such an element', () => { + const node = new Text( 'text' ); + + const parent = schema.findAllowedParent( node, Position.createAt( r1bQp ) ); + + expect( parent ).to.equal( r1bQp ); + } ); + + it( 'should return null when limit element will be reached before allowed parent', () => { + schema.extend( 'blockQuote', { + isLimit: true + } ); + schema.register( 'div', { + allowIn: '$root' + } ); + const node = new Element( 'div' ); + + const parent = schema.findAllowedParent( node, Position.createAt( r1bQp ) ); + + expect( parent ).to.null; + } ); + + it( 'should return null when there is no allowed ancestor for given position', () => { + const node = new Element( 'section' ); + + const parent = schema.findAllowedParent( node, Position.createAt( r1bQp ) ); + + expect( parent ).to.null; + } ); + + it( 'should use custom limit checker nad return null if returns true', () => { + // $root is allowed ancestor for blockQuote. + const node = new Element( 'blockQuote' ); + + const parent = schema.findAllowedParent( + node, + Position.createAt( r1bQp ), + // However lest stop searching when blockQuote is reached. + element => element.name == 'blockQuote' + ); + + expect( parent ).to.null; + } ); + } ); + describe( 'removeDisallowedAttributes()', () => { let model, doc, root; From b1f5c3e88ea780e04d32c89d57c6ec313dfee533 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Fri, 26 Jan 2018 13:30:02 +0100 Subject: [PATCH 04/42] Enhanced model/Writer#split to split more than one element at once. --- src/model/writer.js | 90 +++++++++++++++++++++++++++++++------------ tests/model/writer.js | 40 ++++++++++++++++++- 2 files changed, 103 insertions(+), 27 deletions(-) diff --git a/src/model/writer.js b/src/model/writer.js index 7507358fd..80b75a994 100644 --- a/src/model/writer.js +++ b/src/model/writer.js @@ -573,20 +573,20 @@ export default class Writer { } /** - * Splits an element at the given position. + * Splits elements start from the given position and goes to the top of the model tree as long as given + * `limitElement` won't be reached. When limitElement is not defined then only a parent of given position will be split. * * The element needs to have a parent. It cannot be a root element nor document fragment. * The `writer-split-element-no-parent` error will be thrown if you try to split an element with no parent. * * @param {module:engine/model/position~Position} position Position of split. + * @param {module:engine/model/node~Node} [limitElement] Stop splitting when this element will be reached. + * @returns {~SplitResult} */ - split( position ) { + split( position, limitElement ) { this._assertWriterUsedCorrectly(); - const delta = new SplitDelta(); - this.batch.addDelta( delta ); - - const splitElement = position.parent; + let splitElement = position.parent; if ( !splitElement.parent ) { /** @@ -597,30 +597,63 @@ export default class Writer { throw new CKEditorError( 'writer-split-element-no-parent: Element with no parent can not be split.' ); } - const copy = new Element( splitElement.name, splitElement.getAttributes() ); - const insertVersion = splitElement.root.document ? splitElement.root.document.version : null; + // When limit element is not defined lets set splitElement parent as limit. + if ( !limitElement ) { + limitElement = splitElement.parent; + } - const insert = new InsertOperation( - Position.createAfter( splitElement ), - copy, - insertVersion - ); + if ( !position.parent.getAncestors( { includeSelf: true } ).includes( limitElement ) ) { + throw new CKEditorError( 'writer-split-invalid-limit-element: Limit element is not a position ancestor.' ); + } - delta.addOperation( insert ); - this.model.applyOperation( insert ); + // We need to cache elements that will be created as a result of the first split because + // we need to create a range from the end of the first split element to the beginning of the + // first copy element. This should be handled by LiveRange but it doesn't work on detached nodes. + let firstSplitElement, firstCopyElement; - const moveVersion = insertVersion !== null ? insertVersion + 1 : null; + do { + const delta = new SplitDelta(); + this.batch.addDelta( delta ); - const move = new MoveOperation( - position, - splitElement.maxOffset - position.offset, - Position.createFromParentAndOffset( copy, 0 ), - moveVersion - ); - move.isSticky = true; + const copy = new Element( splitElement.name, splitElement.getAttributes() ); + const insertVersion = splitElement.root.document ? splitElement.root.document.version : null; - delta.addOperation( move ); - this.model.applyOperation( move ); + const insert = new InsertOperation( + Position.createAfter( splitElement ), + copy, + insertVersion + ); + + delta.addOperation( insert ); + this.model.applyOperation( insert ); + + const moveVersion = insertVersion !== null ? insertVersion + 1 : null; + + const move = new MoveOperation( + position, + splitElement.maxOffset - position.offset, + Position.createFromParentAndOffset( copy, 0 ), + moveVersion + ); + move.isSticky = true; + + delta.addOperation( move ); + this.model.applyOperation( move ); + + // Cache result of the first split. + if ( !firstSplitElement && !firstCopyElement ) { + firstSplitElement = splitElement; + firstCopyElement = copy; + } + + position = Position.createBefore( copy ); + splitElement = position.parent; + } while ( splitElement !== limitElement ); + + return { + position, + range: new Range( Position.createAt( firstSplitElement, 'end' ), Position.createAt( firstCopyElement ) ) + }; } /** @@ -1141,3 +1174,10 @@ function isSameTree( rootA, rootB ) { return false; } + +/** + * @typedef {Object} SplitResult + * @property {module:engine/model/position~Position} position between split elements. + * @property {module:engine/model/range~Range} range Range that stars from the end of the first split element and ands + * at the beginning of the first copy element. + */ diff --git a/tests/model/writer.js b/tests/model/writer.js index b1bfa23ff..1b9650373 100644 --- a/tests/model/writer.js +++ b/tests/model/writer.js @@ -1790,6 +1790,42 @@ describe( 'Writer', () => { } ).to.throw( CKEditorError, /^writer-split-element-no-parent/ ); } ); + it( 'should split elements to limitElement', () => { + const div = new Element( 'div', null, p ); + const section = new Element( 'section', null, div ); + + root.insertChildren( 0, section ); + + split( new Position( p, [ 3 ] ), section ); + + expect( root.maxOffset ).to.equal( 1 ); + expect( section.maxOffset ).to.equal( 2 ); + + expect( section.getChild( 0 ).name ).to.equal( 'div' ); + expect( section.getChild( 0 ).getChild( 0 ).name ).to.equal( 'p' ); + expect( section.getChild( 0 ).getChild( 0 ).getAttribute( 'key' ) ).to.equal( 'value' ); + expect( count( section.getChild( 0 ).getChild( 0 ).getAttributes() ) ).to.equal( 1 ); + expect( section.getChild( 0 ).getChild( 0 ).getChild( 0 ).data ).to.equal( 'foo' ); + + expect( section.getChild( 1 ).name ).to.equal( 'div' ); + expect( section.getChild( 1 ).getChild( 0 ).name ).to.equal( 'p' ); + expect( section.getChild( 1 ).getChild( 0 ).getAttribute( 'key' ) ).to.equal( 'value' ); + expect( count( section.getChild( 1 ).getChild( 0 ).getAttributes() ) ).to.equal( 1 ); + expect( section.getChild( 1 ).getChild( 0 ).getChild( 0 ).data ).to.equal( 'bar' ); + } ); + + it( 'should throw when limitElement is not a position ancestor', () => { + const div = new Element( 'div', null, p ); + const section = new Element( 'section', null, div ); + + root.insertChildren( 0, div ); + root.insertChildren( 1, section ); + + expect( () => { + split( new Position( p, [ 3 ] ), section ); + } ).to.throw( CKEditorError, /^writer-split-invalid-limit-element/ ); + } ); + it( 'should throw when trying to use detached writer', () => { const writer = new Writer( model, batch ); @@ -2299,9 +2335,9 @@ describe( 'Writer', () => { } ); } - function split( position ) { + function split( position, limitElement ) { model.enqueueChange( batch, writer => { - writer.split( position ); + writer.split( position, limitElement ); } ); } From 9a0ee46863724176f09948afe3633bb3e339e51b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Fri, 26 Jan 2018 14:16:14 +0100 Subject: [PATCH 05/42] Made dev-utils working with new view->model conversion. --- src/dev-utils/model.js | 35 ++++++++++++++++++++++------------- tests/dev-utils/model.js | 13 ++++--------- 2 files changed, 26 insertions(+), 22 deletions(-) diff --git a/src/dev-utils/model.js b/src/dev-utils/model.js index 306e78d0e..9c2a3ff60 100644 --- a/src/dev-utils/model.js +++ b/src/dev-utils/model.js @@ -277,8 +277,12 @@ export function parse( data, schema, options = {} ) { viewToModel.on( 'element', convertToModelElement() ); viewToModel.on( 'text', convertToModelText() ); + viewToModel.isDebug = true; + // Convert view to model. - let model = viewToModel.convert( viewDocumentFragment.root, { context: options.context || [ '$root' ] } ); + let model = viewToModel.convert( viewDocumentFragment.root, options.context || [ '$root' ] ); + + mapper.bindElements( model, viewDocumentFragment.root ); // If root DocumentFragment contains only one element - return that element. if ( model.childCount == 1 ) { @@ -317,8 +321,9 @@ export function parse( data, schema, options = {} ) { function convertToModelFragment() { return ( evt, data, consumable, conversionApi ) => { - data.output = conversionApi.convertChildren( data.input, consumable, data ); - conversionApi.mapper.bindElements( data.output, data.input ); + data.output = conversionApi.convertChildren( data.input, consumable, data.position ); + + conversionApi.mapper.bindElements( data.position.parent, data.input ); evt.stop(); }; @@ -328,20 +333,22 @@ function convertToModelElement() { return ( evt, data, consumable, conversionApi ) => { const elementName = data.input.name; - if ( !conversionApi.schema.checkChild( data.context, elementName ) ) { - throw new Error( `Element '${ elementName }' was not allowed in context ${ JSON.stringify( data.context ) }.` ); + if ( !conversionApi.schema.checkChild( data.position, elementName ) ) { + throw new Error( `Element '${ elementName }' was not allowed in given position.`, { position: data.position } ); } // View attribute value is a string so we want to typecast it to the original type. // E.g. `bold="true"` - value will be parsed from string `"true"` to boolean `true`. const attributes = convertAttributes( data.input.getAttributes(), parseAttributeValue ); + const element = conversionApi.writer.createElement( data.input.name, attributes ); - data.output = conversionApi.writer.createElement( data.input.name, attributes ); - conversionApi.mapper.bindElements( data.output, data.input ); + conversionApi.writer.insert( element, data.position ); - data.context.push( data.output ); - data.output.appendChildren( conversionApi.convertChildren( data.input, consumable, data ) ); - data.context.pop(); + conversionApi.mapper.bindElements( element, data.input ); + + conversionApi.convertChildren( data.input, consumable, ModelPosition.createAt( element ) ); + + data.output = ModelRange.createOn( element ); evt.stop(); }; @@ -349,8 +356,8 @@ function convertToModelElement() { function convertToModelText( withAttributes = false ) { return ( evt, data, consumable, conversionApi ) => { - if ( !conversionApi.schema.checkChild( data.context, '$text' ) ) { - throw new Error( `Text was not allowed in context ${ JSON.stringify( data.context ) }.` ); + if ( !conversionApi.schema.checkChild( data.position, '$text' ) ) { + throw new Error( 'Text was not allowed in given position.', { position: data.position } ); } let node; @@ -365,7 +372,9 @@ function convertToModelText( withAttributes = false ) { node = conversionApi.writer.createText( data.input.data ); } - data.output = node; + conversionApi.writer.insert( node, data.position ); + + data.output = ModelRange.createFromPositionAndShift( data.position, node.offsetSize ); evt.stop(); }; diff --git a/tests/dev-utils/model.js b/tests/dev-utils/model.js index 689603243..a9894e742 100644 --- a/tests/dev-utils/model.js +++ b/tests/dev-utils/model.js @@ -432,13 +432,7 @@ describe( 'model test utils', () => { } ); test( 'creates empty DocumentFragment with selection', { - data: '[]', - check( fragment, selection ) { - expect( fragment ).to.be.instanceOf( DocumentFragment ); - expect( fragment.childCount ).to.equal( 0 ); - expect( selection.rangeCount ).to.equal( 1 ); - expect( selection.getFirstRange().isEqual( Range.createFromParentsAndOffsets( fragment, 0, fragment, 0 ) ) ).to.be.true; - } + data: '[]' } ); test( 'returns Element if range is around single element', { @@ -529,7 +523,7 @@ describe( 'model test utils', () => { it( 'throws when try to set element not registered in schema', () => { expect( () => { parse( '', model.schema ); - } ).to.throw( Error, 'Element \'xyz\' was not allowed in context ["$root"].' ); + } ).to.throw( Error, 'Element \'xyz\' was not allowed in given position.' ); } ); it( 'throws when try to set text directly to $root without registering it', () => { @@ -537,7 +531,7 @@ describe( 'model test utils', () => { expect( () => { parse( 'text', model.schema ); - } ).to.throw( Error, 'Text was not allowed in context ["$root"].' ); + } ).to.throw( Error, 'Text was not allowed in given position.' ); } ); it( 'converts data in the specified context', () => { @@ -652,6 +646,7 @@ describe( 'model test utils', () => { function test( title, options ) { it( title, () => { const output = options.output || options.data; + const data = parse( options.data, model.schema ); let converted, selection; From f82af42360c269b96dc30e72e57ed63ddd1fade7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Mon, 29 Jan 2018 10:10:25 +0100 Subject: [PATCH 06/42] Removed advanced-converters tests. --- tests/conversion/advanced-converters.js | 553 ------------------------ 1 file changed, 553 deletions(-) delete mode 100644 tests/conversion/advanced-converters.js diff --git a/tests/conversion/advanced-converters.js b/tests/conversion/advanced-converters.js deleted file mode 100644 index 3be34288b..000000000 --- a/tests/conversion/advanced-converters.js +++ /dev/null @@ -1,553 +0,0 @@ -/** - * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved. - * For licensing, see LICENSE.md. - */ - -import Model from '../../src/model/model'; -import ModelElement from '../../src/model/element'; -import ModelText from '../../src/model/text'; -import ModelTextProxy from '../../src/model/textproxy'; -import ModelRange from '../../src/model/range'; -import ModelPosition from '../../src/model/position'; -import ModelWalker from '../../src/model/treewalker'; - -import ViewElement from '../../src/view/element'; -import ViewContainerElement from '../../src/view/containerelement'; -import ViewAttributeElement from '../../src/view/attributeelement'; -import ViewText from '../../src/view/text'; -import viewWriter from '../../src/view/writer'; -import ViewPosition from '../../src/view/position'; -import ViewRange from '../../src/view/range'; - -import EditingController from '../../src/controller/editingcontroller'; - -import ViewConversionDispatcher from '../../src/conversion/viewconversiondispatcher'; - -import { - insertElement, - changeAttribute, - wrap -} from '../../src/conversion/model-to-view-converters'; -import { convertToModelFragment, convertText } from '../../src/conversion/view-to-model-converters'; - -describe( 'advanced-converters', () => { - let model, modelDoc, modelRoot, viewRoot, modelDispatcher, viewDispatcher; - - beforeEach( () => { - model = new Model(); - modelDoc = model.document; - modelRoot = modelDoc.createRoot(); - - const editing = new EditingController( model ); - - viewRoot = editing.view.getRoot(); - - // Set name of view root the same as dom root. - // This is a mock of attaching view root to dom root. - viewRoot._name = 'div'; - - viewDispatcher = new ViewConversionDispatcher( model, { schema: { checkChild: () => true } } ); - viewDispatcher.on( 'text', convertText() ); - viewDispatcher.on( 'documentFragment', convertToModelFragment() ); - - modelDispatcher = editing.modelToView; - } ); - - function viewAttributesToString( item ) { - let result = ''; - - for ( const key of item.getAttributeKeys() ) { - const value = item.getAttribute( key ); - - if ( value ) { - result += ' ' + key + '="' + value + '"'; - } - } - - return result; - } - - function viewToString( item ) { - let result = ''; - - if ( item instanceof ViewText ) { - result = item.data; - } else { - // ViewElement or ViewDocumentFragment. - for ( const child of item.getChildren() ) { - result += viewToString( child ); - } - - if ( item instanceof ViewElement ) { - result = '<' + item.name + viewAttributesToString( item ) + '>' + result + ''; - } - } - - return result; - } - - function modelAttributesToString( item ) { - let result = ''; - - for ( const attr of item.getAttributes() ) { - result += ' ' + attr[ 0 ] + '="' + attr[ 1 ] + '"'; - } - - return result; - } - - function modelToString( item ) { - let result = ''; - - if ( item instanceof ModelTextProxy ) { - const attributes = modelAttributesToString( item ); - - result = attributes ? '<$text' + attributes + '>' + item.data + '' : item.data; - } else { - const walker = new ModelWalker( { boundaries: ModelRange.createIn( item ), shallow: true } ); - - for ( const value of walker ) { - result += modelToString( value.item ); - } - - if ( item instanceof ModelElement ) { - const attributes = modelAttributesToString( item ); - - result = '<' + item.name + attributes + '>' + result + ''; - } - } - - return result; - } - - // Converter overwrites default attribute converter for `linkHref` and `linkTitle` attribute is set on `quote` element. - // - // Model: - // - // [quote {linkHref='foo.html' linkTitle='Foo source'}] - // ├─ f - // ├─ o - // └─ o - // - // foo {linkHref='foo.html' linkTitle='Foo title'} - // - // View: - // - //
- // ├─ foo - // └─ - // └─ see source - // - // - // └─ foo - describe( 'custom attribute handling for given element', () => { - beforeEach( () => { - // Normal model-to-view converters for links. - modelDispatcher.on( 'attribute:linkHref', wrap( value => value ? new ViewAttributeElement( 'a', { href: value } ) : null ) ); - modelDispatcher.on( 'attribute:linkTitle', wrap( value => value ? new ViewAttributeElement( 'a', { title: value } ) : null ) ); - - // Normal view-to-model converters for links. - viewDispatcher.on( 'element:a', ( evt, data, consumable, conversionApi ) => { - if ( consumable.consume( data.input, { name: true, attribute: 'href' } ) ) { - if ( !data.output ) { - data.output = conversionApi.convertChildren( data.input, consumable ); - } - - for ( const child of data.output ) { - child.setAttribute( 'linkHref', data.input.getAttribute( 'href' ) ); - } - } - } ); - - viewDispatcher.on( 'element:a', ( evt, data, consumable, conversionApi ) => { - if ( consumable.consume( data.input, { attribute: 'title' } ) ) { - if ( !data.output ) { - data.output = conversionApi.convertChildren( data.input, consumable ); - } - - for ( const child of data.output ) { - child.setAttribute( 'linkTitle', data.input.getAttribute( 'title' ) ); - } - } - } ); - - // Model-to-view converter for quote element. - modelDispatcher.on( 'insert:quote', ( evt, data, consumable, conversionApi ) => { - consumable.consume( data.item, 'insert' ); - - const viewPosition = conversionApi.mapper.toViewPosition( data.range.start ); - const viewElement = new ViewContainerElement( 'blockquote' ); - - conversionApi.mapper.bindElements( data.item, viewElement ); - viewWriter.insert( viewPosition, viewElement ); - }, { priority: 'high' } ); - - modelDispatcher.on( 'attribute:linkHref:quote', linkHrefOnQuoteConverter, { priority: 'high' } ); - modelDispatcher.on( 'attribute:linkTitle:quote', linkTitleOnQuoteConverter, { priority: 'high' } ); - - function linkHrefOnQuoteConverter( evt, data, consumable, conversionApi ) { - if ( !consumable.consume( data.item, 'attribute:linkHref' ) ) { - return; - } - - const viewQuote = conversionApi.mapper.toViewElement( data.item ); - - if ( data.attributeNewValue === null ) { - // Attribute was removed -> remove the view link. - const viewLink = viewQuote.getChild( viewQuote.childCount - 1 ); - - viewWriter.remove( ViewRange.createOn( viewLink ) ); - - consumable.consume( data.item, 'attribute:linkTitle' ); - } else if ( data.attributeOldValue === null ) { - // Attribute was added -> add the view link. - const viewLink = new ViewAttributeElement( - 'a', { href: data.item.getAttribute( 'linkHref' ) }, new ViewText( 'see source' ) - ); - - if ( consumable.consume( data.item, 'attribute:linkTitle' ) && data.item.getAttribute( 'linkTitle' ) !== null ) { - viewLink.setAttribute( 'title', data.item.getAttribute( 'linkTitle' ) ); - } - - viewWriter.insert( new ViewPosition( viewQuote, viewQuote.childCount ), viewLink ); - } else { - // Attribute has changed -> change the existing view link. - const viewLink = viewQuote.getChild( viewQuote.childCount - 1 ); - viewLink.setAttribute( 'href', data.attributeNewValue ); - } - } - - function linkTitleOnQuoteConverter( evt, data, consumable, conversionApi ) { - if ( !consumable.consume( data.item, 'attribute:linkTitle' ) ) { - return; - } - - const viewQuote = conversionApi.mapper.toViewElement( data.item ); - const viewLink = viewQuote.getChild( viewQuote.childCount - 1 ); - - if ( !viewLink ) { - return; - } - - if ( data.attributeNewValue === null ) { - viewLink.removeAttribute( 'title' ); - } else { - viewLink.setAttribute( 'title', data.attributeNewValue ); - } - } - - // View-to-model converter for quote element. - viewDispatcher.on( 'element:blockquote', ( evt, data, consumable, conversionApi ) => { - if ( consumable.consume( data.input, { name: true } ) ) { - data.output = new ModelElement( 'quote' ); - - const viewA = data.input.getChild( data.input.childCount - 1 ); - - // Convert the special "a" first, before converting all children. - if ( viewA instanceof ViewElement && viewA.name == 'a' && consumable.consume( viewA, { name: true } ) ) { - if ( consumable.consume( viewA, { attribute: 'href' } ) ) { - data.output.setAttribute( 'linkHref', viewA.getAttribute( 'href' ) ); - } - - if ( consumable.consume( viewA, { attribute: 'title' } ) ) { - data.output.setAttribute( 'linkTitle', viewA.getAttribute( 'title' ) ); - } - } - - const children = conversionApi.convertChildren( data.input, consumable ); - data.output.appendChildren( children ); - } - } ); - } ); - - it( 'should convert model text with linkHref and linkTitle to view', () => { - const modelText = new ModelText( 'foo', { linkHref: 'foo.html', linkTitle: 'Foo title' } ); - - // Let's insert text with link attributes. - model.change( writer => { - writer.insert( - modelText, - new ModelPosition( modelRoot, [ 0 ] ) - ); - } ); - - let range = ModelRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 3 ); - - expect( viewToString( viewRoot ) ).to.equal( '' ); - - // Let's change link's attributes. - model.change( writer => { - writer.setAttribute( 'linkHref', 'bar.html', range ); - writer.setAttribute( 'linkTitle', 'Bar title', range ); - } ); - - expect( viewToString( viewRoot ) ).to.equal( '' ); - - // Let's remove a letter from the link. - model.change( writer => { - writer.remove( ModelRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 1 ) ); - } ); - - expect( viewToString( viewRoot ) ).to.equal( '' ); - - // Let's remove just one attribute. - model.change( writer => { - range = ModelRange.createIn( modelRoot ); - writer.removeAttribute( 'linkTitle', range ); - } ); - - expect( viewToString( viewRoot ) ).to.equal( '' ); - - // Let's remove the other attribute. - model.change( writer => { - range = ModelRange.createIn( modelRoot ); - writer.removeAttribute( 'linkHref', range ); - } ); - - expect( viewToString( viewRoot ) ).to.equal( '
oo
' ); - } ); - - it( 'should convert a view element to model', () => { - const viewElement = new ViewAttributeElement( 'a', { href: 'foo.html', title: 'Foo title' }, new ViewText( 'foo' ) ); - - const modelText = viewDispatcher.convert( viewElement ).getChild( 0 ); - - expect( modelText ).to.be.instanceof( ModelText ); - expect( modelText.data ).to.equal( 'foo' ); - expect( modelText.getAttribute( 'linkHref' ) ).to.equal( 'foo.html' ); - expect( modelText.getAttribute( 'linkTitle' ) ).to.equal( 'Foo title' ); - } ); - - it( 'should convert quote model element with linkHref and linkTitle attribute to view', () => { - modelDispatcher.on( 'attribute:bold', wrap( new ViewAttributeElement( 'strong' ) ) ); - - const modelElement = new ModelElement( 'quote', { linkHref: 'foo.html', linkTitle: 'Foo source' }, new ModelText( 'foo' ) ); - - // Let's insert a quote element with link attribute. - model.change( writer => { - writer.insert( - modelElement, - new ModelPosition( modelRoot, [ 0 ] ) - ); - } ); - - let expected = ''; - expect( viewToString( viewRoot ) ).to.equal( expected ); - - // And insert some additional content into it. - model.change( writer => { - writer.insert( - new ModelText( 'bar', { bold: true } ), - new ModelPosition( modelRoot, [ 0, 3 ] ) - ); - } ); - - expected = ''; - expect( viewToString( viewRoot ) ).to.equal( expected ); - - // Let's change some attributes. - model.change( writer => { - writer.removeAttribute( 'linkTitle', modelElement ); - writer.setAttribute( 'linkHref', 'bar.html', modelElement ); - } ); - - expected = ''; - expect( viewToString( viewRoot ) ).to.equal( expected ); - - // Let's remove the only attribute connected with link. - model.change( writer => { - writer.removeAttribute( 'linkHref', modelElement ); - } ); - - expected = '
foobar
'; - expect( viewToString( viewRoot ) ).to.equal( expected ); - } ); - - it( 'should convert view blockquote with a element to model', () => { - const viewElement = new ViewContainerElement( - 'blockquote', - null, - [ - new ViewText( 'foo' ), - new ViewAttributeElement( - 'a', - { - href: 'foo.html', - title: 'Foo source' - }, - new ViewText( 'see source' ) - ) - ] - ); - - const modelElement = viewDispatcher.convert( viewElement ); - - expect( modelToString( modelElement ) ).to.equal( 'foo' ); - } ); - } ); - - // Default view converter for tables that will convert table structure into paragraphs if tables are not supported. - // TRs are supposed to become paragraphs and TDs content should be separated using space. - it( 'default table view to model converter', () => { - viewDispatcher.on( 'element:a', ( evt, data, consumable, conversionApi ) => { - if ( consumable.consume( data.input, { name: true, attribute: 'href' } ) ) { - if ( !data.output ) { - data.output = conversionApi.convertChildren( data.input, consumable ); - } - - for ( const child of data.output ) { - child.setAttribute( 'linkHref', data.input.getAttribute( 'href' ) ); - } - } - } ); - - viewDispatcher.on( 'element:tr', ( evt, data, consumable, conversionApi ) => { - if ( consumable.consume( data.input, { name: true } ) ) { - data.output = new ModelElement( 'paragraph' ); - - const children = conversionApi.convertChildren( data.input, consumable ); - - for ( let i = 1; i < children.childCount; i++ ) { - const child = children.getChild( i ); - - if ( child instanceof ModelText && child.previousSibling instanceof ModelText ) { - children.insertChildren( i, new ModelText( ' ' ) ); - i++; - } - } - - data.output.appendChildren( children ); - } - } ); - - viewDispatcher.on( 'element:table', ( evt, data, consumable, conversionApi ) => { - if ( consumable.consume( data.input, { name: true } ) ) { - data.output = conversionApi.convertChildren( data.input, consumable ); - } - } ); - - viewDispatcher.on( 'element:td', ( evt, data, consumable, conversionApi ) => { - if ( consumable.consume( data.input, { name: true } ) ) { - data.output = conversionApi.convertChildren( data.input, consumable ); - } - } ); - - const viewTable = new ViewContainerElement( 'table', null, [ - new ViewContainerElement( 'tr', null, [ - new ViewContainerElement( 'td', null, new ViewText( 'foo' ) ), - new ViewContainerElement( 'td', null, new ViewAttributeElement( 'a', { href: 'bar.html' }, new ViewText( 'bar' ) ) ) - ] ), - new ViewContainerElement( 'tr', null, [ - new ViewContainerElement( 'td' ), - new ViewContainerElement( 'td', null, new ViewText( 'abc' ) ) - ] ) - ] ); - - expect( modelToString( viewDispatcher.convert( viewTable ) ) ) - .to.equal( 'foo <$text linkHref="bar.html">barabc' ); - } ); - - // Model converter that converts any non-converted elements and attributes into view elements and attributes. - // View converter that converts any non-converted elements and attributes into model elements and attributes. - describe( 'universal converter', () => { - beforeEach( () => { - // "Universal" converters - modelDispatcher.on( 'insert', insertElement( data => new ViewContainerElement( data.item.name ) ), { priority: 'lowest' } ); - modelDispatcher.on( 'attribute', changeAttribute(), { priority: 'lowest' } ); - - viewDispatcher.on( 'element', ( evt, data, consumable, conversionApi ) => { - if ( consumable.consume( data.input, { name: true } ) ) { - data.output = new ModelElement( data.input.name ); - - for ( const key of data.input.getAttributeKeys() ) { - if ( consumable.consume( data.input, { attribute: key } ) ) { - data.output.setAttribute( key, data.input.getAttribute( key ) ); - } - } - - data.output.appendChildren( conversionApi.convertChildren( data.input, consumable ) ); - } - }, { priority: 'lowest' } ); - - // "Real" converters -- added with higher priority. Should overwrite the "universal" converters. - modelDispatcher.on( 'insert:image', insertElement( new ViewContainerElement( 'img' ) ) ); - modelDispatcher.on( 'attribute:bold', wrap( new ViewAttributeElement( 'strong' ) ) ); - - viewDispatcher.on( 'element:img', ( evt, data, consumable ) => { - if ( consumable.consume( data.input, { name: true } ) ) { - const modelImage = new ModelElement( 'image' ); - - for ( const attributeKey of data.input.getAttributeKeys() ) { - modelImage.setAttribute( attributeKey, data.input.getAttribute( attributeKey ) ); - } - - data.output = modelImage; - } - } ); - viewDispatcher.on( 'element:strong', ( evt, data, consumable, conversionApi ) => { - if ( consumable.consume( data.input, { name: true } ) ) { - if ( !data.output ) { - data.output = conversionApi.convertChildren( data.input, consumable ); - } - - for ( const child of data.output ) { - child.setAttribute( 'bold', true ); - } - } - } ); - } ); - - it( 'should convert model to view', () => { - const modelElement = new ModelElement( 'table', { cellpadding: 5, cellspacing: 5 }, [ - new ModelElement( 'tr', null, [ - new ModelElement( 'td', null, [ - new ModelText( 'foo ' ), - new ModelText( 'abc', { bold: true } ), - new ModelText( ' bar' ) - ] ), - new ModelElement( 'td', null, [ - new ModelElement( 'foo', { foo: 'bar' }, new ModelText( 'bar' ) ) - ] ) - ] ) - ] ); - - modelRoot.appendChildren( modelElement ); - modelDispatcher.convertInsert( ModelRange.createIn( modelRoot ) ); - - expect( viewToString( viewRoot ) ).to.equal( - '
' + - '' + - '' + - '' + - '' + - '' + - '
foo abc barbar
' + - '
' - ); - } ); - - it( 'should convert view to model', () => { - const viewElement = new ViewContainerElement( 'table', { cellpadding: 5, cellspacing: 5 }, [ - new ViewContainerElement( 'tr', null, [ - new ViewContainerElement( 'td', null, [ - new ViewText( 'foo ' ), - new ViewAttributeElement( 'strong', null, new ViewText( 'abc' ) ), - new ViewText( ' bar' ) - ] ), - new ViewContainerElement( 'td', null, new ViewContainerElement( 'foo', { foo: 'bar' }, new ViewText( 'bar' ) ) ) - ] ) - ] ); - - const modelElement = viewDispatcher.convert( viewElement ); - - expect( modelToString( modelElement ) ).to.equal( - '' + - '' + - '' + - '' + - '' + - '
foo <$text bold="true">abc barbar
' - ); - } ); - } ); -} ); From 99c7139586b608c13f24ee389770a2e77d62654d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Mon, 29 Jan 2018 10:16:24 +0100 Subject: [PATCH 07/42] Changed default context used in DataController from String to ContextDefinition. --- src/controller/datacontroller.js | 14 +++++++------- tests/controller/datacontroller.js | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/controller/datacontroller.js b/src/controller/datacontroller.js index 5b126d602..09c304218 100644 --- a/src/controller/datacontroller.js +++ b/src/controller/datacontroller.js @@ -208,11 +208,11 @@ export default class DataController { * * @see #set * @param {String} data Data to parse. - * @param {String} [context='$root'] Base context in which the view will be converted to the model. See: - * {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#convert}. + * @param {module:engine/model/schema~SchemaContextDefinition} [context=['$root']] Base context in which the view will + * be converted to the model. See: {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#convert}. * @returns {module:engine/model/documentfragment~DocumentFragment} Parsed data. */ - parse( data, context = '$root' ) { + parse( data, context = [ '$root' ] ) { // data -> view const viewDocumentFragment = this.processor.toView( data ); @@ -231,12 +231,12 @@ export default class DataController { * * @param {module:engine/view/element~Element|module:engine/view/documentfragment~DocumentFragment} viewElementOrFragment * Element or document fragment which content will be converted. - * @param {String} [context='$root'] Base context in which the view will be converted to the model. See: - * {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#convert}. + * @param {module:engine/model/schema~SchemaContextDefinition} [context=['$root']] Base context in which the view will + * be converted to the model. See: {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#convert}. * @returns {module:engine/model/documentfragment~DocumentFragment} Output document fragment. */ - toModel( viewElementOrFragment, context = '$root' ) { - return this.viewToModel.convert( viewElementOrFragment, { context: [ context ] } ); + toModel( viewElementOrFragment, context = [ '$root' ] ) { + return this.viewToModel.convert( viewElementOrFragment, context ); } /** diff --git a/tests/controller/datacontroller.js b/tests/controller/datacontroller.js index d3a1f945f..341a8c71e 100644 --- a/tests/controller/datacontroller.js +++ b/tests/controller/datacontroller.js @@ -97,7 +97,7 @@ describe( 'DataController', () => { } ); it( 'should accept parsing context', () => { - const output = data.parse( 'foo', '$block' ); + const output = data.parse( 'foo', [ '$block' ] ); expect( stringify( output ) ).to.equal( 'foo' ); } ); @@ -138,7 +138,7 @@ describe( 'DataController', () => { expect( stringify( data.toModel( viewFragment ) ) ).to.equal( '' ); // Model fragment in inline root. - expect( stringify( data.toModel( viewFragment, 'inlineRoot' ) ) ).to.equal( 'foo' ); + expect( stringify( data.toModel( viewFragment, [ 'inlineRoot' ] ) ) ).to.equal( 'foo' ); } ); } ); From 866ea63ff0d3147c2e9c0a852765fec58f7cc72e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Mon, 29 Jan 2018 10:17:58 +0100 Subject: [PATCH 08/42] Refactored view->model conversion to use target position. --- src/conversion/buildviewconverter.js | 97 ++++++----- src/conversion/view-to-model-converters.js | 12 +- src/conversion/viewconversiondispatcher.js | 178 ++++++++++++++++----- 3 files changed, 202 insertions(+), 85 deletions(-) diff --git a/src/conversion/buildviewconverter.js b/src/conversion/buildviewconverter.js index f3dcd908a..88742bb78 100644 --- a/src/conversion/buildviewconverter.js +++ b/src/conversion/buildviewconverter.js @@ -9,7 +9,8 @@ import Matcher from '../view/matcher'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; -import isIterable from '@ckeditor/ckeditor5-utils/src/isiterable'; +import Position from '../model/position'; +import Range from '../model/range'; /** * Provides chainable, high-level API to easily build basic view-to-model converters that are appended to given @@ -291,31 +292,62 @@ class ViewConverterBuilder { continue; } - if ( !conversionApi.schema.checkChild( data.context, modelElement ) ) { + // When element was already consumed then skip it. + if ( !consumable.test( data.input, from.consume || match.match ) ) { continue; } - // Try to consume appropriate values from consumable values list. - if ( !consumable.consume( data.input, from.consume || match.match ) ) { + // Find allowed parent for element that we are going to insert. + // If current parent does not allow to insert element but one of the ancestors does + // then split nodes to allowed parent. + const splitResult = conversionApi.splitToAllowedParent( modelElement, data.position, conversionApi ); + + // When there is no split result it means that we can't insert element to model tree, so let's skip it. + if ( !splitResult ) { continue; } - // If everything is fine, we are ready to start the conversion. - // Add newly created `modelElement` to the parents stack. - data.context.push( modelElement ); - - // Convert children of converted view element and append them to `modelElement`. - const modelChildren = conversionApi.convertChildren( data.input, consumable, data ); - - for ( const child of Array.from( modelChildren ) ) { - writer.append( child, modelElement ); + // Insert element on allowed position. + conversionApi.writer.insert( modelElement, splitResult.position ); + + // Convert children and insert to element. + const childrenOutput = conversionApi.convertChildren( data.input, consumable, Position.createAt( modelElement ) ); + + // Consume appropriate value from consumable values list. + consumable.consume( data.input, from.consume || match.match ); + + // Prepare conversion output range, we know that range will start before inserted element. + const output = new Range( Position.createBefore( modelElement ) ); + + // Now we need to check where the output should end. + + // If we had to split parent to insert our element the we want to continue conversion + // inside split parent. + // + // before: [] + // after: [] + if ( splitResult.endElement ) { + output.end = Position.createAt( splitResult.endElement ); + + // When element is converted on target position (without splitting) we need to move range + // after this element but we need to take into consideration that children could split our + // element, so we need to move range after parent of the last converted child. + // + // after: [] + // before: [] + } else if ( childrenOutput.end.parent !== childrenOutput.end.root ) { + output.end = Position.createAfter( childrenOutput.end.parent ); + + // Finally when we are sure that element and its parent was not split we need to put selection + // after element. + // + // after: [] + // before: [] + } else { + output.end = Position.createAfter( modelElement ); } - // Remove created `modelElement` from the parents stack. - data.context.pop(); - - // Add `modelElement` as a result. - data.output = modelElement; + data.output = output; // Prevent multiple conversion if there are other correct matches. break; @@ -365,7 +397,7 @@ class ViewConverterBuilder { // Since we are converting to attribute we need an output on which we will set the attribute. // If the output is not created yet, we will create it. if ( !data.output ) { - data.output = conversionApi.convertChildren( data.input, consumable, data ); + data.output = conversionApi.convertChildren( data.input, consumable, data.position ); } // Use attribute creator function, if provided. @@ -384,8 +416,12 @@ class ViewConverterBuilder { }; } - // Set attribute on current `output`. `Schema` is checked inside this helper function. - setAttributeOn( data.output, attribute, data, conversionApi ); + // Set attribute on each item in range according to Schema. + for ( const node of Array.from( data.output.getItems() ) ) { + if ( conversionApi.schema.checkAttribute( node, attribute.key ) ) { + conversionApi.writer.setAttribute( attribute.key, attribute.value, node ); + } + } // Prevent multiple conversion if there are other correct matches. break; @@ -467,7 +503,8 @@ class ViewConverterBuilder { continue; } - data.output = modelElement; + writer.insert( modelElement, data.position ); + data.output = Range.createOn( modelElement ); // Prevent multiple conversion if there are other correct matches. break; @@ -504,22 +541,6 @@ class ViewConverterBuilder { } } -// Helper function that sets given attributes on given `module:engine/model/node~Node` or -// `module:engine/model/documentfragment~DocumentFragment`. -function setAttributeOn( toChange, attribute, data, conversionApi ) { - if ( isIterable( toChange ) ) { - for ( const node of toChange ) { - setAttributeOn( node, attribute, data, conversionApi ); - } - - return; - } - - if ( conversionApi.schema.checkAttribute( toChange, attribute.key ) ) { - conversionApi.writer.setAttribute( attribute.key, attribute.value, toChange ); - } -} - /** * Entry point for view-to-model converters builder. This chainable API makes it easy to create basic, most common * view-to-model converters and attach them to provided dispatchers. The method returns an instance of diff --git a/src/conversion/view-to-model-converters.js b/src/conversion/view-to-model-converters.js index 0f71c7cdc..8a002434b 100644 --- a/src/conversion/view-to-model-converters.js +++ b/src/conversion/view-to-model-converters.js @@ -3,6 +3,8 @@ * For licensing, see LICENSE.md. */ +import Range from '../model/range'; + /** * Contains {@link module:engine/view/view view} to {@link module:engine/model/model model} converters for * {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher}. @@ -29,7 +31,7 @@ export function convertToModelFragment() { return ( evt, data, consumable, conversionApi ) => { // Second argument in `consumable.consume` is discarded for ViewDocumentFragment but is needed for ViewElement. if ( !data.output && consumable.consume( data.input, { name: true } ) ) { - data.output = conversionApi.convertChildren( data.input, consumable, data ); + data.output = conversionApi.convertChildren( data.input, consumable, data.position ); } }; } @@ -41,9 +43,13 @@ export function convertToModelFragment() { */ export function convertText() { return ( evt, data, consumable, conversionApi ) => { - if ( conversionApi.schema.checkChild( data.context, '$text' ) ) { + if ( conversionApi.schema.checkChild( data.position, '$text' ) ) { if ( consumable.consume( data.input ) ) { - data.output = conversionApi.writer.createText( data.input.data ); + const text = conversionApi.writer.createText( data.input.data ); + + conversionApi.writer.insert( text, data.position ); + + data.output = Range.createFromPositionAndShift( data.position, text.offsetSize ); } } }; diff --git a/src/conversion/viewconversiondispatcher.js b/src/conversion/viewconversiondispatcher.js index ed1abb4b6..eaf652f32 100644 --- a/src/conversion/viewconversiondispatcher.js +++ b/src/conversion/viewconversiondispatcher.js @@ -10,9 +10,7 @@ import ViewConsumable from './viewconsumable'; import ModelRange from '../model/range'; import ModelPosition from '../model/position'; -import ModelTreeWalker from '../model/treewalker'; -import ModelNode from '../model/node'; -import ModelDocumentFragment from '../model/documentfragment'; +import { SchemaContext } from '../model/schema'; import EmitterMixin from '@ckeditor/ckeditor5-utils/src/emittermixin'; import mix from '@ckeditor/ckeditor5-utils/src/mix'; @@ -127,6 +125,7 @@ export default class ViewConversionDispatcher { // set on `conversionApi`. This way only a part of `ViewConversionDispatcher` API is exposed. this.conversionApi.convertItem = this._convertItem.bind( this ); this.conversionApi.convertChildren = this._convertChildren.bind( this ); + this.conversionApi.splitToAllowedParent = this._splitToAllowedParent.bind( this ); } /** @@ -137,43 +136,62 @@ export default class ViewConversionDispatcher { * @fires documentFragment * @param {module:engine/view/documentfragment~DocumentFragment|module:engine/view/element~Element} viewItem * Part of the view to be converted. - * @param {Object} additionalData Additional data to be passed in `data` argument when firing `ViewConversionDispatcher` - * events. See also {@link ~ViewConversionDispatcher#event:element element event}. + * @param {module:engine/model/schema~SchemaContextDefinition} [context=['$root']] Elements will be converted according to this context. * @returns {module:engine/model/documentfragment~DocumentFragment} Model data that is a result of the conversion process * wrapped in `DocumentFragment`. Converted marker elements will be set as that document fragment's * {@link module:engine/model/documentfragment~DocumentFragment#markers static markers map}. */ - convert( viewItem, additionalData ) { + convert( viewItem, context = [ '$root' ] ) { return this._model.change( writer => { + this.fire( 'viewCleanup', viewItem ); + + const consumable = ViewConsumable.createFrom( viewItem ); + + // Create model tree according to given context. Elements will be converted to the top element of this tree. + // Thanks to this schema will be able check items precisely. + // Top element of context tree is marked by a `isContextTree` attribute. + const position = contextToPosition( context, writer ); + // Store writer in current conversion as a conversion API. this.conversionApi.writer = writer; - this.fire( 'viewCleanup', viewItem ); + // Create set for split elements. We need to remember this elements, because at the end of conversion + // we want to remove all empty elements that was created as a result of the split. + this.conversionApi.splitElements = new Set(); - const consumable = ViewConsumable.createFrom( viewItem ); - let conversionResult = this._convertItem( viewItem, consumable, additionalData ); + // Additional date available between conversions. + // Needed when one converter needs to leave some data for oder converters. + this.conversionApi.data = {}; - // Remove writer from conversion API after conversion. - this.conversionApi.writer = null; + // Do the conversion. + const range = this._convertItem( viewItem, consumable, position ); - // We can get a null here if conversion failed (see _convertItem()) - // or simply if an item could not be converted (e.g. due to the schema). - if ( !conversionResult ) { - return writer.createDocumentFragment(); - } + // Conversion result is always a document fragment so let's create this fragment. + const documentFragment = writer.createDocumentFragment(); + + // When there is a conversion result. + if ( range ) { + // Remove each empty element that was created as a result of split. + for ( const item of this.conversionApi.splitElements ) { + removeEmptySplitResult( item, this.conversionApi ); + } - // When conversion result is not a document fragment we need to wrap it in document fragment. - if ( !conversionResult.is( 'documentFragment' ) ) { - const docFrag = writer.createDocumentFragment(); + // Move all items that was converted to context tree to document fragment. + for ( const item of Array.from( position.parent.getChildren() ) ) { + writer.append( item, documentFragment ); + } - writer.append( conversionResult, docFrag ); - conversionResult = docFrag; + // Extract temporary markers elements from model and set as static markers collection. + documentFragment.markers = extractMarkersFromModelFragment( documentFragment, writer ); } - // Extract temporary markers elements from model and set as static markers collection. - conversionResult.markers = extractMarkersFromModelFragment( conversionResult, writer ); + // Clear conversion API. + this.conversionApi.writer = null; + this.conversionApi.splitElements = null; + this.conversionApi.data = null; - return conversionResult; + // Return fragment as conversion result. + return documentFragment; } ); } @@ -181,11 +199,8 @@ export default class ViewConversionDispatcher { * @private * @see module:engine/conversion/viewconversiondispatcher~ViewConversionApi#convertItem */ - _convertItem( input, consumable, additionalData = {} ) { - const data = Object.assign( {}, additionalData, { - input, - output: null - } ); + _convertItem( input, consumable, position ) { + const data = Object.assign( { input, position, output: null } ); if ( input.is( 'element' ) ) { this.fire( 'element:' + input.name, data, consumable, this.conversionApi ); @@ -196,7 +211,7 @@ export default class ViewConversionDispatcher { } // Handle incorrect `data.output`. - if ( data.output && !( data.output instanceof ModelNode || data.output instanceof ModelDocumentFragment ) ) { + if ( data.output && !( data.output instanceof ModelRange ) ) { /** * Incorrect conversion result was dropped. * @@ -217,19 +232,52 @@ export default class ViewConversionDispatcher { * @private * @see module:engine/conversion/viewconversiondispatcher~ViewConversionApi#convertChildren */ - _convertChildren( input, consumable, additionalData ) { - const writer = this.conversionApi.writer; - const documentFragment = writer.createDocumentFragment(); + _convertChildren( input, consumable, position ) { + const output = new ModelRange( position ); for ( const viewChild of Array.from( input.getChildren() ) ) { - const modelChild = this._convertItem( viewChild, consumable, additionalData ); + const range = this._convertItem( viewChild, consumable, output.end ); - if ( modelChild instanceof ModelNode || modelChild instanceof ModelDocumentFragment ) { - writer.append( modelChild, documentFragment ); + if ( range instanceof ModelRange ) { + output.end = range.end; } } - return documentFragment; + return output; + } + + /** + * TODO + * + * @private + */ + _splitToAllowedParent( element, position, conversionApi ) { + function checkLimit( node ) { + return node.hasAttribute( 'isContextTree' ); + } + + const allowedParent = conversionApi.schema.findAllowedParent( element, position, checkLimit ); + + if ( !allowedParent ) { + return; + } + + if ( allowedParent === position.parent ) { + return { position }; + } + + const data = conversionApi.writer.split( position, allowedParent ); + + for ( const pos of data.range.getPositions() ) { + if ( !pos.isEqual( data.position ) ) { + conversionApi.splitElements.add( pos.parent ); + } + } + + return { + position: data.position, + endElement: data.range.end.parent + }; } /** @@ -292,16 +340,13 @@ function extractMarkersFromModelFragment( modelItem, writer ) { const markers = new Map(); // Create ModelTreeWalker. - const walker = new ModelTreeWalker( { - startPosition: ModelPosition.createAt( modelItem, 0 ), - ignoreElementEnd: true - } ); + const range = ModelRange.createIn( modelItem ).getItems(); // Walk through DocumentFragment and collect marker elements. - for ( const value of walker ) { + for ( const item of range ) { // Check if current element is a marker. - if ( value.item.name == '$marker' ) { - markerElements.add( value.item ); + if ( item.name == '$marker' ) { + markerElements.add( item ); } } @@ -325,6 +370,43 @@ function extractMarkersFromModelFragment( modelItem, writer ) { return markers; } +function contextToPosition( contextDefinition, writer ) { + let position; + + for ( const item of new SchemaContext( contextDefinition ) ) { + const attributes = {}; + + for ( const key of item.getAttributeKeys() ) { + attributes[ key ] = item.getAttribute( key ); + } + + const current = writer.createElement( item.name, attributes ); + + if ( position ) { + writer.append( current, position ); + } + + position = ModelPosition.createAt( current ); + } + + position.parent.setAttribute( 'isContextTree', true ); + + return position; +} + +function removeEmptySplitResult( item, conversionApi ) { + if ( item.isEmpty ) { + const parent = item.parent; + + conversionApi.writer.remove( item ); + conversionApi.splitElements.delete( item ); + + if ( conversionApi.splitElements.has( parent ) ) { + removeEmptySplitResult( parent, conversionApi ); + } + } +} + /** * Conversion interface that is registered for given {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher} * and is passed as one of parameters when {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher dispatcher} @@ -351,6 +433,7 @@ function extractMarkersFromModelFragment( modelItem, writer ) { * @param {module:engine/view/documentfragment~DocumentFragment|module:engine/view/element~Element|module:engine/view/text~Text} * input Item to convert. * @param {module:engine/conversion/viewconsumable~ViewConsumable} consumable Values to consume. + * @param {module:engine/model/position~Position} position Position of conversion. * @param {Object} [additionalData] Additional data to be passed in `data` argument when firing `ViewConversionDispatcher` * events. See also {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:element element event}. * @returns {module:engine/model/node~Node|module:engine/model/documentfragment~DocumentFragment|null} The result of item conversion, @@ -367,8 +450,15 @@ function extractMarkersFromModelFragment( modelItem, writer ) { * @param {module:engine/view/documentfragment~DocumentFragment|module:engine/view/element~Element} * input Item which children will be converted. * @param {module:engine/conversion/viewconsumable~ViewConsumable} consumable Values to consume. + * @param {module:engine/model/position~Position} position Position of conversion. * @param {Object} [additionalData] Additional data to be passed in `data` argument when firing `ViewConversionDispatcher` * events. See also {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:element element event}. * @returns {module:engine/model/documentfragment~DocumentFragment} Model document fragment containing results of conversion * of all children of given item. */ + +/** + * Custom data stored by converter for conversion process. + * + * @param {Object} #data + */ From 2d2a96e4d1d58aca1a1b4bdfe73e086af030ab65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Mon, 29 Jan 2018 10:22:31 +0100 Subject: [PATCH 09/42] Aligned most of tests to new view->model conversion. --- tests/conversion/buildviewconverter.js | 83 ++++++++++--------- .../conversion/definition-based-converters.js | 36 ++++---- tests/conversion/view-to-model-converters.js | 25 +++--- tests/conversion/viewconversiondispatcher.js | 34 +++++--- 4 files changed, 95 insertions(+), 83 deletions(-) diff --git a/tests/conversion/buildviewconverter.js b/tests/conversion/buildviewconverter.js index 56ef47dea..4ef814d96 100644 --- a/tests/conversion/buildviewconverter.js +++ b/tests/conversion/buildviewconverter.js @@ -23,7 +23,6 @@ import ViewConversionDispatcher from '../../src/conversion/viewconversiondispatc import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import { convertToModelFragment, convertText } from '../../src/conversion/view-to-model-converters'; -import { stringify } from '../../src/dev-utils/model'; function modelAttributesToString( item ) { let result = ''; @@ -63,13 +62,13 @@ const textAttributes = [ 'linkHref', 'linkTitle', 'bold', 'italic', 'style' ]; const pAttributes = [ 'class', 'important', 'theme', 'decorated', 'size' ]; describe( 'View converter builder', () => { - let dispatcher, model, schema, additionalData; + let dispatcher, model, schema, context; beforeEach( () => { model = new Model(); - // `additionalData` parameter for `.convert` calls. - additionalData = { context: [ '$root' ] }; + // `context` parameter for `.convert` calls. + context = [ '$root' ]; schema = model.schema; @@ -111,7 +110,7 @@ describe( 'View converter builder', () => { it( 'should convert from view element to model element', () => { buildViewConverter().for( dispatcher ).fromElement( 'p' ).toElement( 'paragraph' ); - const conversionResult = dispatcher.convert( new ViewContainerElement( 'p', null, new ViewText( 'foo' ) ), additionalData ); + const conversionResult = dispatcher.convert( new ViewContainerElement( 'p', null, new ViewText( 'foo' ) ), context ); expect( modelToString( conversionResult ) ).to.equal( 'foo' ); } ); @@ -121,7 +120,7 @@ describe( 'View converter builder', () => { .fromElement( 'img' ) .toElement( viewElement => new ModelElement( 'image', { src: viewElement.getAttribute( 'src' ) } ) ); - const conversionResult = dispatcher.convert( new ViewContainerElement( 'img', { src: 'foo.jpg' } ), additionalData ); + const conversionResult = dispatcher.convert( new ViewContainerElement( 'img', { src: 'foo.jpg' } ), context ); expect( modelToString( conversionResult ) ).to.equal( '' ); } ); @@ -130,7 +129,7 @@ describe( 'View converter builder', () => { buildViewConverter().for( dispatcher ).fromElement( 'strong' ).toAttribute( 'bold', true ); const conversionResult = dispatcher.convert( - new ViewAttributeElement( 'strong', null, new ViewText( 'foo' ) ), additionalData + new ViewAttributeElement( 'strong', null, new ViewText( 'foo' ) ), context ); // Have to check root because result is a ModelText. @@ -143,7 +142,7 @@ describe( 'View converter builder', () => { .toAttribute( viewElement => ( { key: 'linkHref', value: viewElement.getAttribute( 'href' ) } ) ); const conversionResult = dispatcher.convert( - new ViewAttributeElement( 'a', { href: 'foo.html' }, new ViewText( 'foo' ) ), additionalData + new ViewAttributeElement( 'a', { href: 'foo.html' }, new ViewText( 'foo' ) ), context ); // Have to check root because result is a ModelText. @@ -158,7 +157,7 @@ describe( 'View converter builder', () => { .toAttribute( viewElement => ( { key: 'class', value: viewElement.getAttribute( 'class' ) } ) ); const conversionResult = dispatcher.convert( - new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), additionalData + new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), context ); expect( modelToString( conversionResult ) ).to.equal( 'foo' ); @@ -180,7 +179,7 @@ describe( 'View converter builder', () => { new ViewContainerElement( 'p', { 'data-type': 'foo' }, new ViewText( 'xyz' ) ) ] ); - const conversionResult = dispatcher.convert( viewStructure, additionalData ); + const conversionResult = dispatcher.convert( viewStructure, context ); expect( modelToString( conversionResult ) ).to.equal( 'foo' + @@ -206,7 +205,7 @@ describe( 'View converter builder', () => { new ViewContainerElement( 'span', { style: 'font-weight:bold; font-size:20px' }, new ViewText( 'ddd' ) ) ] ); - const conversionResult = dispatcher.convert( viewElement, additionalData ); + const conversionResult = dispatcher.convert( viewElement, context ); expect( modelToString( conversionResult ) ).to.equal( '<$text bold="true">aaabbbcccddd' ); } ); @@ -223,12 +222,13 @@ describe( 'View converter builder', () => { new ViewText( 'r' ) ] ); - const conversionResult = dispatcher.convert( viewElement, additionalData ); + const conversionResult = dispatcher.convert( viewElement, context ); const markerSearch = conversionResult.markers.get( 'search' ); expect( conversionResult.markers.size ).to.equal( 1 ); - expect( stringify( conversionResult, markerSearch ) ).to.equal( 'Fo[o ba]r' ); + expect( markerSearch.start.path ).to.deep.equal( [ 0, 2 ] ); + expect( markerSearch.end.path ).to.deep.equal( [ 0, 6 ] ); } ); it( 'should convert from element to marker using creator function', () => { @@ -245,12 +245,13 @@ describe( 'View converter builder', () => { new ViewText( 'r' ) ] ); - const conversionResult = dispatcher.convert( viewElement, additionalData ); + const conversionResult = dispatcher.convert( viewElement, context ); const markerSearch = conversionResult.markers.get( 'search' ); expect( conversionResult.markers.size ).to.equal( 1 ); - expect( stringify( conversionResult, markerSearch ) ).to.equal( 'Fo[o ba]r' ); + expect( markerSearch.start.path ).to.deep.equal( [ 0, 2 ] ); + expect( markerSearch.end.path ).to.deep.equal( [ 0, 6 ] ); } ); it( 'should convert from multiple view entities to marker', () => { @@ -271,16 +272,16 @@ describe( 'View converter builder', () => { new ViewText( 'r' ) ] ); - const conversionResult = dispatcher.convert( viewElement, additionalData ); + const conversionResult = dispatcher.convert( viewElement ); const marker1 = conversionResult.markers.get( 'marker1' ); const marker2 = conversionResult.markers.get( 'marker2' ); const marker3 = conversionResult.markers.get( 'marker3' ); expect( conversionResult.markers.size ).to.equal( 3 ); - expect( stringify( conversionResult, marker1 ) ).to.equal( 'Fo[]o bar' ); - expect( stringify( conversionResult, marker2 ) ).to.equal( 'Foo b[]ar' ); - expect( stringify( conversionResult, marker3 ) ).to.equal( 'Foo ba[]r' ); + expect( marker1.start.path ).to.deep.equal( marker1.end.path ).to.deep.equal( [ 0, 2 ] ); + expect( marker2.start.path ).to.deep.equal( marker2.end.path ).to.deep.equal( [ 0, 5 ] ); + expect( marker3.start.path ).to.deep.equal( marker3.end.path ).to.deep.equal( [ 0, 6 ] ); } ); it( 'should do nothing when there is no element matching to marker pattern', () => { @@ -288,7 +289,7 @@ describe( 'View converter builder', () => { const element = new ViewAttributeElement( 'span' ); - const result = dispatcher.convert( element, additionalData ); + const result = dispatcher.convert( element ); expect( result ).to.be.instanceof( ModelDocumentFragment ); expect( result.childCount ).to.equal( 0 ); @@ -300,7 +301,7 @@ describe( 'View converter builder', () => { const element = new ViewAttributeElement( 'marker', { class: 'search' } ); expect( () => { - dispatcher.convert( element, additionalData ); + dispatcher.convert( element, context ); } ).to.throw( CKEditorError, /^build-view-converter-invalid-marker/ ); } ); @@ -312,7 +313,7 @@ describe( 'View converter builder', () => { const element = new ViewAttributeElement( 'marker', { 'data-name': 'search' } ); expect( () => { - dispatcher.convert( element, additionalData ); + dispatcher.convert( element, context ); } ).to.throw( CKEditorError, /^build-view-converter-invalid-marker/ ); } ); @@ -324,7 +325,7 @@ describe( 'View converter builder', () => { const element = new ViewAttributeElement( 'marker', { 'data-name': 'search' } ); expect( () => { - dispatcher.convert( element, additionalData ); + dispatcher.convert( element, context ); } ).to.throw( CKEditorError, /^build-view-converter-invalid-marker/ ); } ); @@ -341,7 +342,7 @@ describe( 'View converter builder', () => { // Not quite megatron. result = dispatcher.convert( - new ViewContainerElement( 'span', { class: 'megatron' }, new ViewText( 'foo' ) ), additionalData + new ViewContainerElement( 'span', { class: 'megatron' }, new ViewText( 'foo' ) ), context ); expect( modelToString( result ) ).to.equal( 'foo' ); @@ -349,7 +350,7 @@ describe( 'View converter builder', () => { // Almost a megatron. Missing a head. result = dispatcher.convert( new ViewContainerElement( 'span', { class: 'megatron', body: 'megatron', legs: 'megatron' }, new ViewText( 'foo' ) ), - additionalData + context ); expect( modelToString( result ) ).to.equal( 'foo' ); @@ -361,7 +362,7 @@ describe( 'View converter builder', () => { { class: 'megatron', body: 'megatron', legs: 'megatron', head: 'megatron' }, new ViewText( 'foo' ) ), - additionalData + context ); expect( modelToString( result ) ).to.equal( 'foo' ); @@ -373,7 +374,7 @@ describe( 'View converter builder', () => { { class: 'megatron', body: 'megatron', legs: 'megatron', head: 'megatron' }, new ViewText( 'foo' ) ), - additionalData + context ); expect( modelToString( result ) ).to.equal( 'foo' ); @@ -393,7 +394,7 @@ describe( 'View converter builder', () => { new ViewText( 'foo' ) ); - const conversionResult = dispatcher.convert( viewElement, additionalData ); + const conversionResult = dispatcher.convert( viewElement, context ); expect( modelToString( conversionResult ) ).to.equal( 'foo' ); } ); @@ -403,7 +404,7 @@ describe( 'View converter builder', () => { const viewElement = new ViewAttributeElement( 'strong', null, new ViewText( 'foo' ) ); - const conversionResult = dispatcher.convert( viewElement, additionalData ); + const conversionResult = dispatcher.convert( viewElement, context ); expect( conversionResult.is( 'documentFragment' ) ).to.be.true; } ); @@ -415,7 +416,7 @@ describe( 'View converter builder', () => { buildViewConverter().for( dispatcher ).fromElement( 'p' ).toElement( 'paragraph' ); const conversionResult = dispatcher.convert( - new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), additionalData + new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), context ); // Element converter was fired first even though attribute converter was added first. @@ -431,7 +432,7 @@ describe( 'View converter builder', () => { let result; result = dispatcher.convert( - new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), additionalData + new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), context ); expect( modelToString( result ) ).to.equal( 'foo' ); @@ -441,7 +442,7 @@ describe( 'View converter builder', () => { .toElement( 'customP' ); result = dispatcher.convert( - new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), additionalData + new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), context ); expect( modelToString( result ) ).to.equal( 'foo' ); @@ -462,7 +463,7 @@ describe( 'View converter builder', () => { .toAttribute( 'size', 'small' ); const viewElement = new ViewContainerElement( 'p', { class: 'decorated small' }, new ViewText( 'foo' ) ); - const conversionResult = dispatcher.convert( viewElement, additionalData ); + const conversionResult = dispatcher.convert( viewElement, context ); // P element and it's children got converted by the converter (1) and the converter (1) got fired // because P name was not consumed in converter (2). Converter (3) could consume class="small" because @@ -485,7 +486,7 @@ describe( 'View converter builder', () => { new ViewContainerElement( 'abcd', null, new ViewText( 'foo' ) ) ] ); - const conversionResult = dispatcher.convert( viewStructure, additionalData ); + const conversionResult = dispatcher.convert( viewStructure, context ); expect( modelToString( conversionResult ) ).to.equal( '
foo
' ); } ); @@ -509,7 +510,7 @@ describe( 'View converter builder', () => { ) ); - const conversionResult = dispatcher.convert( viewElement, additionalData ); + const conversionResult = dispatcher.convert( viewElement, context ); expect( modelToString( conversionResult ) ).to.equal( 'foo' ); } ); @@ -536,7 +537,7 @@ describe( 'View converter builder', () => { // ) // ); - // const conversionResult = dispatcher.convert( viewElement, additionalData ); + // const conversionResult = dispatcher.convert( viewElement, context ); // expect( modelToString( conversionResult ) ).to.equal( 'foo' ); // } ); @@ -551,7 +552,7 @@ describe( 'View converter builder', () => { ) ); - const conversionResult = dispatcher.convert( viewElement, additionalData ); + const conversionResult = dispatcher.convert( viewElement, context ); expect( modelToString( conversionResult ) ).to.equal( 'foo' ); } ); @@ -565,11 +566,11 @@ describe( 'View converter builder', () => { } ); const viewElement = new ViewContainerElement( 'p' ); - let conversionResult = dispatcher.convert( viewElement, additionalData ); + let conversionResult = dispatcher.convert( viewElement, context ); expect( modelToString( conversionResult ) ).to.equal( '' ); viewElement.setAttribute( 'stop', true ); - conversionResult = dispatcher.convert( viewElement, additionalData ); + conversionResult = dispatcher.convert( viewElement, context ); expect( conversionResult ).to.be.instanceof( ModelDocumentFragment ); expect( conversionResult.childCount ).to.equal( 0 ); @@ -587,11 +588,11 @@ describe( 'View converter builder', () => { } ); const viewElement = new ViewContainerElement( 'p', { 'data-type': 'foo' } ); - let conversionResult = dispatcher.convert( viewElement, additionalData ); + let conversionResult = dispatcher.convert( viewElement, context ); expect( modelToString( conversionResult ) ).to.equal( '' ); viewElement.setAttribute( 'data-type', 'stop' ); - conversionResult = dispatcher.convert( viewElement, additionalData ); + conversionResult = dispatcher.convert( viewElement, context ); expect( modelToString( conversionResult ) ).to.equal( '' ); } ); } ); diff --git a/tests/conversion/definition-based-converters.js b/tests/conversion/definition-based-converters.js index ea7770850..12a55b5d2 100644 --- a/tests/conversion/definition-based-converters.js +++ b/tests/conversion/definition-based-converters.js @@ -95,14 +95,14 @@ function viewToString( item ) { } describe( 'definition-based-converters', () => { - let model, dispatcher, modelDoc, modelRoot, viewRoot, controller, additionalData, schema; + let model, dispatcher, modelDoc, modelRoot, viewRoot, controller, context, schema; beforeEach( () => { model = new Model(); } ); function setupViewToModelTests() { - additionalData = { context: [ '$root' ] }; + context = [ '$root' ]; schema = model.schema; dispatcher = new ViewConversionDispatcher( model, { schema } ); } @@ -229,7 +229,7 @@ describe( 'definition-based-converters', () => { viewToModelAttribute( 'foo', { model: 'bar', view: 'strong' }, [ dispatcher ] ); const conversionResult = dispatcher.convert( - new ViewAttributeElement( 'strong', null, new ViewText( 'foo' ) ), additionalData + new ViewAttributeElement( 'strong', null, new ViewText( 'foo' ) ), context ); expect( modelToString( conversionResult ) ).to.equal( '<$text foo="bar">foo' ); @@ -239,7 +239,7 @@ describe( 'definition-based-converters', () => { viewToModelAttribute( 'foo', { model: 'bar', view: { name: 'strong' } }, [ dispatcher ] ); const conversionResult = dispatcher.convert( - new ViewAttributeElement( 'strong', null, new ViewText( 'foo' ) ), additionalData + new ViewAttributeElement( 'strong', null, new ViewText( 'foo' ) ), context ); expect( modelToString( conversionResult ) ).to.equal( '<$text foo="bar">foo' ); @@ -249,7 +249,7 @@ describe( 'definition-based-converters', () => { viewToModelAttribute( 'foo', { model: 'bar', view: { name: 'span', class: 'foo' } }, [ dispatcher ] ); const conversionResult = dispatcher.convert( - new ViewAttributeElement( 'span', { class: 'foo' }, new ViewText( 'foo' ) ), additionalData + new ViewAttributeElement( 'span', { class: 'foo' }, new ViewText( 'foo' ) ), context ); expect( modelToString( conversionResult ) ).to.equal( '<$text foo="bar">foo' ); @@ -262,7 +262,7 @@ describe( 'definition-based-converters', () => { }, [ dispatcher ] ); const conversionResult = dispatcher.convert( - new ViewAttributeElement( 'span', { class: 'foo bar' }, new ViewText( 'foo' ) ), additionalData + new ViewAttributeElement( 'span', { class: 'foo bar' }, new ViewText( 'foo' ) ), context ); expect( modelToString( conversionResult ) ).to.equal( '<$text foo="bar">foo' ); @@ -275,7 +275,7 @@ describe( 'definition-based-converters', () => { }, [ dispatcher ] ); const conversionResult = dispatcher.convert( - new ViewAttributeElement( 'span', { style: 'font-weight:bold' }, new ViewText( 'foo' ) ), additionalData + new ViewAttributeElement( 'span', { style: 'font-weight:bold' }, new ViewText( 'foo' ) ), context ); expect( modelToString( conversionResult ) ).to.equal( '<$text foo="bar">foo' ); @@ -288,7 +288,7 @@ describe( 'definition-based-converters', () => { }, [ dispatcher ] ); const conversionResult = dispatcher.convert( - new ViewAttributeElement( 'span', { 'data-foo': 'bar' }, new ViewText( 'foo' ) ), additionalData + new ViewAttributeElement( 'span', { 'data-foo': 'bar' }, new ViewText( 'foo' ) ), context ); expect( modelToString( conversionResult ) ).to.equal( '<$text foo="bar">foo' ); @@ -305,7 +305,7 @@ describe( 'definition-based-converters', () => { }, [ dispatcher ] ); const conversionResult = dispatcher.convert( - new ViewAttributeElement( 'span', { 'data-foo': 'bar' }, new ViewText( 'foo' ) ), additionalData + new ViewAttributeElement( 'span', { 'data-foo': 'bar' }, new ViewText( 'foo' ) ), context ); expect( modelToString( conversionResult ) ).to.equal( '<$text foo="bar">foo' ); @@ -316,7 +316,7 @@ describe( 'definition-based-converters', () => { viewToModelAttribute( 'foo', { model: 'bar', view: { name: 'strong', priority: 'high' } }, [ dispatcher ] ); const conversionResult = dispatcher.convert( - new ViewAttributeElement( 'strong', null, new ViewText( 'foo' ) ), additionalData + new ViewAttributeElement( 'strong', null, new ViewText( 'foo' ) ), context ); expect( modelToString( conversionResult ) ).to.equal( '<$text foo="bar">foo' ); @@ -396,7 +396,7 @@ describe( 'definition-based-converters', () => { viewToModelElement( { model: 'bar', view: 'strong' }, [ dispatcher ] ); const conversionResult = dispatcher.convert( - new ViewElement( 'strong', null, new ViewText( 'foo' ) ), additionalData + new ViewElement( 'strong', null, new ViewText( 'foo' ) ), context ); expect( modelToString( conversionResult ) ).to.equal( 'foo' ); @@ -406,7 +406,7 @@ describe( 'definition-based-converters', () => { viewToModelElement( { model: 'bar', view: { name: 'strong' } }, [ dispatcher ] ); const conversionResult = dispatcher.convert( - new ViewElement( 'strong', null, new ViewText( 'foo' ) ), additionalData + new ViewElement( 'strong', null, new ViewText( 'foo' ) ), context ); expect( modelToString( conversionResult ) ).to.equal( 'foo' ); @@ -416,7 +416,7 @@ describe( 'definition-based-converters', () => { viewToModelElement( { model: 'bar', view: { name: 'span', class: 'foo' } }, [ dispatcher ] ); const conversionResult = dispatcher.convert( - new ViewElement( 'span', { class: 'foo' }, new ViewText( 'foo' ) ), additionalData + new ViewElement( 'span', { class: 'foo' }, new ViewText( 'foo' ) ), context ); expect( modelToString( conversionResult ) ).to.equal( 'foo' ); @@ -426,7 +426,7 @@ describe( 'definition-based-converters', () => { viewToModelElement( { model: 'bar', view: { name: 'span', class: [ 'foo', 'bar' ] } }, [ dispatcher ] ); const conversionResult = dispatcher.convert( - new ViewElement( 'span', { class: 'foo bar' }, new ViewText( 'foo' ) ), additionalData + new ViewElement( 'span', { class: 'foo bar' }, new ViewText( 'foo' ) ), context ); expect( modelToString( conversionResult ) ).to.equal( 'foo' ); @@ -436,7 +436,7 @@ describe( 'definition-based-converters', () => { viewToModelElement( { model: 'bar', view: { name: 'span', style: { 'font-weight': 'bold' } } }, [ dispatcher ] ); const conversionResult = dispatcher.convert( - new ViewElement( 'span', { style: 'font-weight:bold' }, new ViewText( 'foo' ) ), additionalData + new ViewElement( 'span', { style: 'font-weight:bold' }, new ViewText( 'foo' ) ), context ); expect( modelToString( conversionResult ) ).to.equal( 'foo' ); @@ -446,7 +446,7 @@ describe( 'definition-based-converters', () => { viewToModelElement( { model: 'bar', view: { name: 'span', attribute: { 'data-foo': 'bar' } } }, [ dispatcher ] ); const conversionResult = dispatcher.convert( - new ViewElement( 'span', { 'data-foo': 'bar' }, new ViewText( 'foo' ) ), additionalData + new ViewElement( 'span', { 'data-foo': 'bar' }, new ViewText( 'foo' ) ), context ); expect( modelToString( conversionResult ) ).to.equal( 'foo' ); @@ -463,7 +463,7 @@ describe( 'definition-based-converters', () => { }, [ dispatcher ] ); const conversionResult = dispatcher.convert( - new ViewElement( 'span', { 'data-foo': 'bar' }, new ViewText( 'foo' ) ), additionalData + new ViewElement( 'span', { 'data-foo': 'bar' }, new ViewText( 'foo' ) ), context ); expect( modelToString( conversionResult ) ).to.equal( 'foo' ); @@ -474,7 +474,7 @@ describe( 'definition-based-converters', () => { viewToModelElement( { model: 'bar', view: { name: 'strong', priority: 'high' } }, [ dispatcher ] ); const conversionResult = dispatcher.convert( - new ViewElement( 'strong', null, new ViewText( 'foo' ) ), additionalData + new ViewElement( 'strong', null, new ViewText( 'foo' ) ), context ); expect( modelToString( conversionResult ) ).to.equal( 'foo' ); diff --git a/tests/conversion/view-to-model-converters.js b/tests/conversion/view-to-model-converters.js index c660c2f06..b06d9e7f1 100644 --- a/tests/conversion/view-to-model-converters.js +++ b/tests/conversion/view-to-model-converters.js @@ -12,11 +12,12 @@ import Model from '../../src/model/model'; import ModelDocumentFragment from '../../src/model/documentfragment'; import ModelElement from '../../src/model/element'; import ModelText from '../../src/model/text'; +import ModelRange from '../../src/model/range'; import { convertToModelFragment, convertText } from '../../src/conversion/view-to-model-converters'; describe( 'view-to-model-converters', () => { - let dispatcher, schema, additionalData, model; + let dispatcher, schema, context, model; beforeEach( () => { model = new Model(); @@ -25,7 +26,7 @@ describe( 'view-to-model-converters', () => { schema.register( 'paragraph', { inheritAllFrom: '$block' } ); schema.extend( '$text', { allowIn: '$root' } ); - additionalData = { context: [ '$root' ] }; + context = [ '$root' ]; dispatcher = new ViewConversionDispatcher( model, { schema } ); } ); @@ -36,7 +37,7 @@ describe( 'view-to-model-converters', () => { dispatcher.on( 'text', convertText() ); - const conversionResult = dispatcher.convert( viewText, additionalData ); + const conversionResult = dispatcher.convert( viewText, context ); expect( conversionResult ).to.be.instanceof( ModelDocumentFragment ); expect( conversionResult.getChild( 0 ) ).to.be.instanceof( ModelText ); @@ -49,13 +50,15 @@ describe( 'view-to-model-converters', () => { // Default converter for elements. Returns just converted children. Added with lowest priority. dispatcher.on( 'text', convertText(), { priority: 'lowest' } ); // Added with normal priority. Should make the above converter not fire. - dispatcher.on( 'text', ( evt, data, consumable ) => { + dispatcher.on( 'text', ( evt, data, consumable, conversionApi ) => { if ( consumable.consume( data.input ) ) { - data.output = new ModelText( data.input.data.replace( /fuck/gi, '****' ) ); + const text = conversionApi.writer.createText( data.input.data.replace( /fuck/gi, '****' ) ); + conversionApi.writer.insert( text, data.position ); + data.output = ModelRange.createFromPositionAndShift( data.position, text.offsetSize ); } } ); - const conversionResult = dispatcher.convert( viewText, additionalData ); + const conversionResult = dispatcher.convert( viewText, context ); expect( conversionResult ).to.be.instanceof( ModelDocumentFragment ); expect( conversionResult.getChild( 0 ) ).to.be.instanceof( ModelText ); @@ -72,12 +75,12 @@ describe( 'view-to-model-converters', () => { const viewText = new ViewText( 'foobar' ); dispatcher.on( 'text', convertText() ); - let conversionResult = dispatcher.convert( viewText, additionalData ); + let conversionResult = dispatcher.convert( viewText, context ); expect( conversionResult ).to.be.instanceof( ModelDocumentFragment ); expect( conversionResult.childCount ).to.equal( 0 ); - conversionResult = dispatcher.convert( viewText, { context: [ '$block' ] } ); + conversionResult = dispatcher.convert( viewText, [ '$block' ] ); expect( conversionResult ).to.be.instanceof( ModelDocumentFragment ); expect( conversionResult.childCount ).to.equal( 1 ); @@ -90,7 +93,7 @@ describe( 'view-to-model-converters', () => { dispatcher.on( 'text', convertText() ); - const conversionResult = dispatcher.convert( viewText, additionalData ); + const conversionResult = dispatcher.convert( viewText, context ); expect( conversionResult ).to.be.instanceof( ModelDocumentFragment ); expect( conversionResult.getChild( 0 ) ).to.be.instanceof( ModelText ); @@ -111,7 +114,7 @@ describe( 'view-to-model-converters', () => { dispatcher.on( 'element', convertToModelFragment() ); dispatcher.on( 'documentFragment', convertToModelFragment() ); - const conversionResult = dispatcher.convert( viewFragment, additionalData ); + const conversionResult = dispatcher.convert( viewFragment, context ); expect( conversionResult ).to.be.instanceof( ModelDocumentFragment ); expect( conversionResult.maxOffset ).to.equal( 6 ); @@ -136,7 +139,7 @@ describe( 'view-to-model-converters', () => { } } ); - const conversionResult = dispatcher.convert( viewP, additionalData ); + const conversionResult = dispatcher.convert( viewP, context ); expect( conversionResult ).to.be.instanceof( ModelDocumentFragment ); expect( conversionResult.getChild( 0 ) ).to.be.instanceof( ModelElement ); diff --git a/tests/conversion/viewconversiondispatcher.js b/tests/conversion/viewconversiondispatcher.js index fa8f1280f..69b57a83b 100644 --- a/tests/conversion/viewconversiondispatcher.js +++ b/tests/conversion/viewconversiondispatcher.js @@ -12,6 +12,8 @@ import Model from '../../src/model/model'; import ModelText from '../../src/model/text'; import ModelElement from '../../src/model/element'; import ModelDocumentFragment from '../../src/model/documentfragment'; +import ModelPosition from '../../src/model/position'; +import ModelRange from '../../src/model/range'; import { stringify } from '../../src/dev-utils/model'; import log from '@ckeditor/ckeditor5-utils/src/log'; @@ -86,7 +88,7 @@ describe( 'ViewConversionDispatcher', () => { // Check correctness of passed parameters. expect( evt.name ).to.equal( 'text' ); expect( data.input ).to.equal( viewText ); - expect( data.foo ).to.equal( 'bar' ); + expect( data.position ).to.instanceof( ModelPosition ); // Check whether consumable has appropriate value to consume. expect( consumable.consume( data.input ) ).to.be.true; @@ -94,18 +96,21 @@ describe( 'ViewConversionDispatcher', () => { // Check whether conversionApi of `dispatcher` has been passed. expect( conversionApi ).to.equal( dispatcher.conversionApi ); + const text = conversionApi.writer.createText( data.input.data ); + conversionApi.writer.insert( text, data.position ); + // Set conversion result to `output` property of `data`. // Later we will check if it was returned by `convert` method. - data.output = new ModelText( data.foo ); + data.output = ModelRange.createOn( text ); } ); // Use `additionalData` parameter to check if it was passed to the event. - const conversionResult = dispatcher.convert( viewText, { foo: 'bar' } ); + const conversionResult = dispatcher.convert( viewText ); // Check conversion result. // Result should be wrapped in document fragment. expect( conversionResult ).to.be.instanceof( ModelDocumentFragment ); - expect( conversionResult.getChild( 0 ).data ).to.equal( 'bar' ); + expect( conversionResult.getChild( 0 ).data ).to.equal( 'foobar' ); expect( spy.calledOnce ).to.be.true; } ); @@ -120,7 +125,7 @@ describe( 'ViewConversionDispatcher', () => { // Check correctness of passed parameters. expect( evt.name ).to.equal( 'element:p' ); expect( data.input ).to.equal( viewElement ); - expect( data.foo ).to.equal( 'bar' ); + expect( data.position ).to.instanceof( ModelPosition ); // Check whether consumable has appropriate value to consume. expect( consumable.consume( data.input, { name: true } ) ).to.be.true; @@ -129,13 +134,16 @@ describe( 'ViewConversionDispatcher', () => { // Check whether conversionApi of `dispatcher` has been passed. expect( conversionApi ).to.equal( dispatcher.conversionApi ); + const paragraph = conversionApi.writer.createElement( 'paragraph' ); + conversionApi.writer.insert( paragraph, data.position ); + // Set conversion result to `output` property of `data`. // Later we will check if it was returned by `convert` method. - data.output = new ModelElement( 'paragraph' ); + data.output = ModelRange.createOn( paragraph ); } ); // Use `additionalData` parameter to check if it was passed to the event. - const conversionResult = dispatcher.convert( viewElement, { foo: 'bar' } ); + const conversionResult = dispatcher.convert( viewElement ); // Check conversion result. // Result should be wrapped in document fragment. @@ -155,7 +163,7 @@ describe( 'ViewConversionDispatcher', () => { // Check correctness of passed parameters. expect( evt.name ).to.equal( 'documentFragment' ); expect( data.input ).to.equal( viewFragment ); - expect( data.foo ).to.equal( 'bar' ); + expect( data.position ).to.instanceof( ModelPosition ); // Check whether consumable has appropriate value to consume. expect( consumable.consume( data.input ) ).to.be.true; @@ -169,7 +177,7 @@ describe( 'ViewConversionDispatcher', () => { } ); // Use `additionalData` parameter to check if it was passed to the event. - const conversionResult = dispatcher.convert( viewFragment, { foo: 'bar' } ); + const conversionResult = dispatcher.convert( viewFragment ); // Check conversion result. expect( conversionResult ).to.be.instanceof( ModelDocumentFragment ); @@ -263,11 +271,11 @@ describe( 'ViewConversionDispatcher', () => { dispatcher.on( 'documentFragment', ( evt, data, consumable, conversionApi ) => { spy(); - expect( conversionApi.convertItem( viewP, consumableMock, data ) ).to.equal( modelP ); - expect( conversionApi.convertItem( viewText, consumableMock, data ) ).to.equal( modelText ); + expect( conversionApi.convertItem( viewP, consumableMock, data.position ) ).to.equal( modelP ); + expect( conversionApi.convertItem( viewText, consumableMock, data.position ) ).to.equal( modelText ); } ); - dispatcher.convert( new ViewDocumentFragment(), { foo: 'bar' } ); + dispatcher.convert( new ViewDocumentFragment() ); expect( spy.calledOnce ).to.be.true; expect( spyP.calledOnce ).to.be.true; @@ -349,7 +357,7 @@ describe( 'ViewConversionDispatcher', () => { expect( result.getChild( 1 ) ).to.equal( modelText ); } ); - dispatcher.convert( new ViewDocumentFragment( [ viewArray, viewP, viewDiv, viewText, viewNull ] ), { foo: 'bar' } ); + dispatcher.convert( new ViewDocumentFragment( [ viewArray, viewP, viewDiv, viewText, viewNull ] ) ); expect( spy.calledOnce ).to.be.true; expect( spyNull.calledOnce ).to.be.true; From 81763eb42ad11a037984e601f18a8feca3ba0471 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Mon, 29 Jan 2018 13:10:08 +0100 Subject: [PATCH 10/42] Fixed failing documentFragment default converter. --- tests/conversion/view-to-model-converters.js | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/tests/conversion/view-to-model-converters.js b/tests/conversion/view-to-model-converters.js index b06d9e7f1..6fd3e9191 100644 --- a/tests/conversion/view-to-model-converters.js +++ b/tests/conversion/view-to-model-converters.js @@ -13,6 +13,7 @@ import ModelDocumentFragment from '../../src/model/documentfragment'; import ModelElement from '../../src/model/element'; import ModelText from '../../src/model/text'; import ModelRange from '../../src/model/range'; +import ModelPosition from '../../src/model/position'; import { convertToModelFragment, convertText } from '../../src/conversion/view-to-model-converters'; @@ -31,13 +32,13 @@ describe( 'view-to-model-converters', () => { dispatcher = new ViewConversionDispatcher( model, { schema } ); } ); - describe( 'convertText', () => { + describe( 'convertText()', () => { it( 'should return converter converting ViewText to ModelText', () => { const viewText = new ViewText( 'foobar' ); dispatcher.on( 'text', convertText() ); - const conversionResult = dispatcher.convert( viewText, context ); + const conversionResult = dispatcher.convert( viewText ); expect( conversionResult ).to.be.instanceof( ModelDocumentFragment ); expect( conversionResult.getChild( 0 ) ).to.be.instanceof( ModelText ); @@ -101,7 +102,7 @@ describe( 'view-to-model-converters', () => { } ); } ); - describe( 'convertToModelFragment', () => { + describe( 'convertToModelFragment()', () => { it( 'should return converter converting whole ViewDocumentFragment to ModelDocumentFragment', () => { const viewFragment = new ViewDocumentFragment( [ new ViewContainerElement( 'p', null, new ViewText( 'foo' ) ), @@ -131,11 +132,12 @@ describe( 'view-to-model-converters', () => { // Added with normal priority. Should make the above converter not fire. dispatcher.on( 'element:p', ( evt, data, consumable, conversionApi ) => { if ( consumable.consume( data.input, { name: true } ) ) { - data.output = new ModelElement( 'paragraph' ); + const paragraph = conversionApi.writer.createElement( 'paragraph' ); + + conversionApi.writer.insert( paragraph, data.position ); + conversionApi.convertChildren( data.input, consumable, ModelPosition.createAt( paragraph ) ); - data.context.push( data.output ); - data.output.appendChildren( conversionApi.convertChildren( data.input, consumable, data ) ); - data.context.pop(); + data.output = ModelRange.createOn( paragraph ); } } ); From 0ead24cd1bc868235121aa747ecabdeaa29e867a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Mon, 29 Jan 2018 21:37:02 +0100 Subject: [PATCH 11/42] Fixed typo. --- src/conversion/buildviewconverter.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conversion/buildviewconverter.js b/src/conversion/buildviewconverter.js index 88742bb78..0a6411c96 100644 --- a/src/conversion/buildviewconverter.js +++ b/src/conversion/buildviewconverter.js @@ -321,7 +321,7 @@ class ViewConverterBuilder { // Now we need to check where the output should end. - // If we had to split parent to insert our element the we want to continue conversion + // If we had to split parent to insert our element then we want to continue conversion // inside split parent. // // before: [] From 2980bdaad130440483581a594f746603344b5ade Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Mon, 29 Jan 2018 21:37:49 +0100 Subject: [PATCH 12/42] Minor ViewConversionDispatcher refactoring. --- src/conversion/viewconversiondispatcher.js | 109 +++++++++++++-------- 1 file changed, 69 insertions(+), 40 deletions(-) diff --git a/src/conversion/viewconversiondispatcher.js b/src/conversion/viewconversiondispatcher.js index eaf652f32..1106b032f 100644 --- a/src/conversion/viewconversiondispatcher.js +++ b/src/conversion/viewconversiondispatcher.js @@ -19,8 +19,8 @@ import log from '@ckeditor/ckeditor5-utils/src/log'; /** * `ViewConversionDispatcher` is a central point of {@link module:engine/view/view view} conversion, which is a process of * converting given {@link module:engine/view/documentfragment~DocumentFragment view document fragment} or - * {@link module:engine/view/element~Element} - * into another structure. In default application, {@link module:engine/view/view view} is converted to {@link module:engine/model/model}. + * {@link module:engine/view/element~Element} into another structure. + * In default application, {@link module:engine/view/view view} is converted to {@link module:engine/model/model}. * * During conversion process, for all {@link module:engine/view/node~Node view nodes} from the converted view document fragment, * `ViewConversionDispatcher` fires corresponding events. Special callbacks called "converters" should listen to @@ -114,6 +114,15 @@ export default class ViewConversionDispatcher { */ this._model = model; + /** + * Store of split elements that was created as a result of conversion using {@link #_splitToAllowedParent}. + * We need to remember these elements because at the end of conversion we want to remove all empty elements. + * + * @protected + * @type {Set} + */ + this._splitElements = new Set(); + /** * Interface passed by dispatcher to the events callbacks. * @@ -121,8 +130,8 @@ export default class ViewConversionDispatcher { */ this.conversionApi = Object.assign( {}, conversionApi ); - // `convertItem` and `convertChildren` are bound to this `ViewConversionDispatcher` instance and - // set on `conversionApi`. This way only a part of `ViewConversionDispatcher` API is exposed. + // `convertItem`, `convertChildren` and `splitToAllowedParent` are bound to this `ViewConversionDispatcher` + // instance and set on `conversionApi`. This way only a part of `ViewConversionDispatcher` API is exposed. this.conversionApi.convertItem = this._convertItem.bind( this ); this.conversionApi.convertChildren = this._convertChildren.bind( this ); this.conversionApi.splitToAllowedParent = this._splitToAllowedParent.bind( this ); @@ -155,12 +164,8 @@ export default class ViewConversionDispatcher { // Store writer in current conversion as a conversion API. this.conversionApi.writer = writer; - // Create set for split elements. We need to remember this elements, because at the end of conversion - // we want to remove all empty elements that was created as a result of the split. - this.conversionApi.splitElements = new Set(); - // Additional date available between conversions. - // Needed when one converter needs to leave some data for oder converters. + // Needed when one converter needs to leave some data for the oder converters. this.conversionApi.data = {}; // Do the conversion. @@ -171,10 +176,8 @@ export default class ViewConversionDispatcher { // When there is a conversion result. if ( range ) { - // Remove each empty element that was created as a result of split. - for ( const item of this.conversionApi.splitElements ) { - removeEmptySplitResult( item, this.conversionApi ); - } + // Remove all empty elements that was created as a result of split. + this._removeEmptySplitElements(); // Move all items that was converted to context tree to document fragment. for ( const item of Array.from( position.parent.getChildren() ) ) { @@ -185,9 +188,11 @@ export default class ViewConversionDispatcher { documentFragment.markers = extractMarkersFromModelFragment( documentFragment, writer ); } + // Clear split elements. + this._splitElements.clear(); + // Clear conversion API. this.conversionApi.writer = null; - this.conversionApi.splitElements = null; this.conversionApi.data = null; // Return fragment as conversion result. @@ -247,30 +252,35 @@ export default class ViewConversionDispatcher { } /** - * TODO - * * @private + * @see module:engine/conversion/viewconversiondispatcher~ViewConversionApi#splitToAllowedParent */ - _splitToAllowedParent( element, position, conversionApi ) { + _splitToAllowedParent( element, position ) { function checkLimit( node ) { return node.hasAttribute( 'isContextTree' ); } - const allowedParent = conversionApi.schema.findAllowedParent( element, position, checkLimit ); + // Try to find allowed parent. + const allowedParent = this.conversionApi.schema.findAllowedParent( element, position, checkLimit ); + // When there is no parent that allows to insert element then return `null`. if ( !allowedParent ) { - return; + return null; } + // When current position parent allows to insert element then return this position. if ( allowedParent === position.parent ) { return { position }; } - const data = conversionApi.writer.split( position, allowedParent ); + // Split element to allowed parent. + const data = this.conversionApi.writer.split( position, allowedParent ); + // Remember all elements that are created as a result of split. + // This is important because at the end of conversion we want to remove all empty split elements. for ( const pos of data.range.getPositions() ) { if ( !pos.isEqual( data.position ) ) { - conversionApi.splitElements.add( pos.parent ); + this._splitElements.add( pos.parent ); } } @@ -280,6 +290,30 @@ export default class ViewConversionDispatcher { }; } + /** + * Checks if {@link #_splitElements} contains empty elements and remove them. + * We need to do it smart because there could be elements that are not empty because contains + * other empty split elements and after removing its children they become available to remove. + * We need to continue iterating over split elements as long as any element will be removed. + * + * @private + */ + _removeEmptySplitElements() { + let removed = false; + + for ( const element of this._splitElements ) { + if ( element.isEmpty ) { + this.conversionApi.writer.remove( element ); + this._splitElements.delete( element ); + removed = true; + } + } + + if ( removed ) { + this._removeEmptySplitElements(); + } + } + /** * Fired before the first conversion event, at the beginning of view to model conversion process. * @@ -304,7 +338,6 @@ export default class ViewConversionDispatcher { * @param {module:engine/view/element~Element} data.input Converted element. * @param {*} data.output The current state of conversion result. Every change to converted element should * be reflected by setting or modifying this property. - * @param {module:engine/model/schema~SchemaPath} data.context The conversion context. * @param {module:engine/conversion/viewconsumable~ViewConsumable} consumable Values to consume. * @param {Object} conversionApi Conversion interface to be used by callback, passed in `ViewConversionDispatcher` constructor. * Besides of properties passed in constructor, it also has `convertItem` and `convertChildren` methods which are references @@ -394,19 +427,6 @@ function contextToPosition( contextDefinition, writer ) { return position; } -function removeEmptySplitResult( item, conversionApi ) { - if ( item.isEmpty ) { - const parent = item.parent; - - conversionApi.writer.remove( item ); - conversionApi.splitElements.delete( item ); - - if ( conversionApi.splitElements.has( parent ) ) { - removeEmptySplitResult( parent, conversionApi ); - } - } -} - /** * Conversion interface that is registered for given {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher} * and is passed as one of parameters when {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher dispatcher} @@ -423,8 +443,7 @@ function removeEmptySplitResult( item, conversionApi ) { * * Every fired event is passed (as first parameter) an object with `output` property. Every event may set and/or * modify that property. When all callbacks are done, the final value of `output` property is returned by this method. - * The `output` must be either {@link module:engine/model/node~Node model node} or - * {@link module:engine/model/documentfragment~DocumentFragment model document fragment} or `null` (as set by default). + * The `output` must be {@link module:engine/model/range~Range model range} or `null` (as set by default). * * @method #convertItem * @fires module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:element @@ -436,7 +455,7 @@ function removeEmptySplitResult( item, conversionApi ) { * @param {module:engine/model/position~Position} position Position of conversion. * @param {Object} [additionalData] Additional data to be passed in `data` argument when firing `ViewConversionDispatcher` * events. See also {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:element element event}. - * @returns {module:engine/model/node~Node|module:engine/model/documentfragment~DocumentFragment|null} The result of item conversion, + * @returns {module:engine/model/range~Range|null} Model range containing result of item conversion, * created and modified by callbacks attached to fired event, or `null` if the conversion result was incorrect. */ @@ -453,8 +472,18 @@ function removeEmptySplitResult( item, conversionApi ) { * @param {module:engine/model/position~Position} position Position of conversion. * @param {Object} [additionalData] Additional data to be passed in `data` argument when firing `ViewConversionDispatcher` * events. See also {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:element element event}. - * @returns {module:engine/model/documentfragment~DocumentFragment} Model document fragment containing results of conversion - * of all children of given item. + * @returns {module:engine/model/range~Range} Model range containing results of conversion of all children of given item. + * When none of children was converted + */ + +/** + * Find allowed parent for element that we are going to insert starting from given position. + * If current parent does not allow to insert element but one of the ancestors does then split nodes to allowed parent. + * + * @method #splitToAllowedParent + * @param {module:engine/model/position~Position} position Position on which element is going to be inserted. + * @param {module:engine/model/element~Node} element Element to insert. + * @returns TODO */ /** From 0b72cb5d52421c95cfaa678f404da27e04b0eb3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Mon, 29 Jan 2018 21:39:10 +0100 Subject: [PATCH 13/42] Fixed failing and added missing ViewConversionDispatcher tests. --- tests/conversion/viewconversiondispatcher.js | 302 +++++++++++++++++-- 1 file changed, 271 insertions(+), 31 deletions(-) diff --git a/tests/conversion/viewconversiondispatcher.js b/tests/conversion/viewconversiondispatcher.js index 69b57a83b..28fa57d57 100644 --- a/tests/conversion/viewconversiondispatcher.js +++ b/tests/conversion/viewconversiondispatcher.js @@ -10,12 +10,14 @@ import ViewText from '../../src/view/text'; import Model from '../../src/model/model'; import ModelText from '../../src/model/text'; +import ModelTextProxy from '../../src/model/textproxy'; import ModelElement from '../../src/model/element'; import ModelDocumentFragment from '../../src/model/documentfragment'; import ModelPosition from '../../src/model/position'; import ModelRange from '../../src/model/range'; -import { stringify } from '../../src/dev-utils/model'; +import ModelWriter from '../../src/model/writer'; +import first from '@ckeditor/ckeditor5-utils/src/first'; import log from '@ckeditor/ckeditor5-utils/src/log'; // Stored in case it is silenced and has to be restored. @@ -24,7 +26,7 @@ const logWarn = log.warn; describe( 'ViewConversionDispatcher', () => { let model; - afterEach( () => { + beforeEach( () => { model = new Model(); log.warn = logWarn; } ); @@ -34,14 +36,14 @@ describe( 'ViewConversionDispatcher', () => { const apiObj = {}; const dispatcher = new ViewConversionDispatcher( model, { apiObj } ); - expect( dispatcher.model ).to.equal( model ); expect( dispatcher.conversionApi.apiObj ).to.equal( apiObj ); expect( dispatcher.conversionApi ).to.have.property( 'convertItem' ).that.is.instanceof( Function ); expect( dispatcher.conversionApi ).to.have.property( 'convertChildren' ).that.is.instanceof( Function ); + expect( dispatcher.conversionApi ).to.have.property( 'splitToAllowedParent' ).that.is.instanceof( Function ); } ); } ); - describe( 'convert', () => { + describe( 'convert()', () => { let dispatcher; beforeEach( () => { @@ -104,7 +106,6 @@ describe( 'ViewConversionDispatcher', () => { data.output = ModelRange.createOn( text ); } ); - // Use `additionalData` parameter to check if it was passed to the event. const conversionResult = dispatcher.convert( viewText ); // Check conversion result. @@ -171,9 +172,12 @@ describe( 'ViewConversionDispatcher', () => { // Check whether conversionApi of `dispatcher` has been passed. expect( conversionApi ).to.equal( dispatcher.conversionApi ); + const text = conversionApi.writer.createText( 'foo' ); + conversionApi.writer.insert( text, data.position ); + // Set conversion result to `output` property of `data`. // Later we will check if it was returned by `convert` method. - data.output = new ModelDocumentFragment( [ new ModelText( 'foo' ) ] ); + data.output = ModelRange.createOn( text ); } ); // Use `additionalData` parameter to check if it was passed to the event. @@ -185,11 +189,134 @@ describe( 'ViewConversionDispatcher', () => { expect( spy.calledOnce ).to.be.true; } ); + it( 'should add contextual properties to conversion api', () => { + const viewElement = new ViewContainerElement( 'p', null, new ViewText( 'foobar' ) ); + + // To be sure that both converters was called. + const spy = sinon.spy(); + + // To check that the same batch is used across conversion. + let batch; + + // Contextual properties of ConversionApi should be undefined/empty before conversion. + expect( dispatcher.conversionApi.writer ).to.not.ok; + expect( dispatcher.conversionApi.data ).to.not.ok; + expect( dispatcher._splitElements.size ).to.equal( 0 ); + + dispatcher.on( 'element', ( evt, data, consumable, conversionApi ) => { + // Check conversion api params. + expect( conversionApi.writer ).to.instanceof( ModelWriter ); + expect( dispatcher._splitElements ).to.instanceof( Set ); + expect( dispatcher._splitElements.size ).to.equal( 0 ); + expect( conversionApi.data ).to.deep.equal( {} ); + + // Remember writer batch to check in next converter that is exactly the same batch. + batch = conversionApi.writer.batch; + + // Add some data to conversion API to verify them in next converter. + // Set some custom data to conversion api data object. + conversionApi.data.foo = 'bar'; + + // Do the conversion. + const paragraph = conversionApi.writer.createElement( 'paragraph' ); + conversionApi.writer.insert( paragraph, data.position ); + + // Add empty element and mark as a split result to check in next converter. + const splitElement = conversionApi.writer.createElement( 'paragraph' ); + dispatcher._splitElements.add( splitElement ); + conversionApi.writer.insert( splitElement, ModelPosition.createAfter( paragraph ) ); + + // Convert children - this wil call second converter. + conversionApi.convertChildren( data.input, consumable, ModelPosition.createAt( paragraph ) ); + + data.output = ModelRange.createOn( paragraph ); + + spy(); + } ); + + dispatcher.on( 'text', ( evt, data, consumable, conversionApi ) => { + // The same batch is used in converters during one conversion. + expect( conversionApi.writer.batch ).to.equal( batch ); + + // Data set by previous converter are remembered. + expect( conversionApi.data ).to.deep.equal( { foo: 'bar' } ); + + // Split element is remembered as well. + expect( dispatcher._splitElements.size ).to.equal( 1 ); + + spy(); + } ); + + dispatcher.convert( viewElement ); + + // To be sure that both converters was called. + sinon.assert.calledTwice( spy ); + + // Contextual properties of ConversionApi should be cleared after conversion. + expect( dispatcher.conversionApi.writer ).to.not.ok; + expect( dispatcher.conversionApi.data ).to.not.ok; + expect( dispatcher._splitElements.size ).to.equal( 0 ); + } ); + + it( 'should remove empty elements that was created as a result of split', () => { + const viewElement = new ViewContainerElement( 'p' ); + + // To be sure that converter was called. + const spy = sinon.spy(); + + dispatcher.on( 'element', ( evt, data, consumable, conversionApi ) => { + // First let's convert target element. + const paragraph = conversionApi.writer.createElement( 'paragraph' ); + conversionApi.writer.insert( paragraph, data.position ); + + // Then add some elements and mark as split. + + // Create and insert empty split element before target element. + const emptySplit = conversionApi.writer.createElement( 'paragraph' ); + conversionApi.writer.insert( emptySplit, ModelPosition.createAfter( paragraph ) ); + + // Create and insert not empty split after target element. + const notEmptySplit = conversionApi.writer.createElement( 'paragraph' ); + conversionApi.writer.appendText( 'foo', notEmptySplit ); + conversionApi.writer.insert( notEmptySplit, ModelPosition.createAfter( emptySplit ) ); + + // Create and insert split with other split inside (both should be removed) + const outerSplit = conversionApi.writer.createElement( 'paragraph' ); + const innerSplit = conversionApi.writer.createElement( 'paragraph' ); + conversionApi.writer.append( innerSplit, outerSplit ); + conversionApi.writer.insert( outerSplit, ModelPosition.createBefore( paragraph ) ); + + dispatcher._splitElements.add( emptySplit ); + dispatcher._splitElements.add( notEmptySplit ); + dispatcher._splitElements.add( outerSplit ); + dispatcher._splitElements.add( innerSplit ); + + data.output = ModelRange.createOn( paragraph ); + + // We have the following result: + //

[

]

foo

+ // Everything out of selected range is a result of the split. + + spy(); + } ); + + const result = dispatcher.convert( viewElement ); + + // Empty split elements should be removed and we should have the following result: + // [

]

foo

+ expect( result.childCount ).to.equal( 2 ); + expect( result.getChild( 0 ).name ).to.equal( 'paragraph' ); + expect( result.getChild( 0 ).childCount ).to.equal( 0 ); + expect( result.getChild( 1 ).name ).to.equal( 'paragraph' ); + expect( result.getChild( 1 ).childCount ).to.equal( 1 ); + expect( result.getChild( 1 ).getChild( 0 ).data ).to.equal( 'foo' ); + } ); + it( 'should extract temporary markers elements from converter element and create static markers list', () => { const viewFragment = new ViewDocumentFragment(); - dispatcher.on( 'documentFragment', ( evt, data ) => { - data.output = new ModelDocumentFragment( [ + dispatcher.on( 'documentFragment', ( evt, data, consumable, conversionApi ) => { + const fragment = new ModelDocumentFragment( [ new ModelText( 'fo' ), new ModelElement( '$marker', { 'data-name': 'marker1' } ), new ModelText( 'o' ), @@ -198,18 +325,27 @@ describe( 'ViewConversionDispatcher', () => { new ModelElement( '$marker', { 'data-name': 'marker1' } ), new ModelText( 'ar' ), ] ); + + conversionApi.writer.insert( fragment, data.position ); + + data.output = ModelRange.createIn( data.position.parent ); } ); const conversionResult = dispatcher.convert( viewFragment ); expect( conversionResult.markers.size ).to.equal( 2 ); - expect( stringify( conversionResult, conversionResult.markers.get( 'marker1' ) ) ).to.deep.equal( 'fo[ob]ar' ); - expect( stringify( conversionResult, conversionResult.markers.get( 'marker2' ) ) ).to.deep.equal( 'foo[]bar' ); + + const marker1 = conversionResult.markers.get( 'marker1' ); + const marker2 = conversionResult.markers.get( 'marker2' ); + + expect( marker1.start.path ).to.deep.equal( [ 2 ] ); + expect( marker1.end.path ).to.deep.equal( [ 4 ] ); + expect( marker2.start.path ).to.deep.equal( marker2.end.path ).to.deep.equal( [ 3 ] ); } ); } ); describe( 'conversionApi', () => { - let spy, spyP, spyText, viewP, viewText, modelP, modelText, consumableMock, dispatcher, + let spy, spyP, spyText, viewP, viewText, modelP, modelText, consumableMock, rootMock, dispatcher, spyNull, spyArray, viewDiv, viewNull, viewArray; beforeEach( () => { @@ -222,26 +358,27 @@ describe( 'ViewConversionDispatcher', () => { modelP = new ModelElement( 'paragraph' ); modelText = new ModelText( 'foobar' ); + // Put nodes to documentFragment, this will mock root element and makes possible to create range on them. + rootMock = new ModelDocumentFragment( [ modelP, modelText ] ); + consumableMock = {}; - dispatcher = new ViewConversionDispatcher( model ); + dispatcher = new ViewConversionDispatcher( model, { schema: model.schema } ); dispatcher.on( 'element:p', ( evt, data, consumable ) => { spyP(); - expect( data.foo ).to.equal( 'bar' ); expect( consumable ).to.equal( consumableMock ); - data.output = modelP; + data.output = ModelRange.createOn( modelP ); } ); dispatcher.on( 'text', ( evt, data, consumable ) => { spyText(); - expect( data.foo ).to.equal( 'bar' ); expect( consumable ).to.equal( consumableMock ); - data.output = modelText; + data.output = ModelRange.createOn( modelText ); } ); spyNull = sinon.spy(); @@ -264,15 +401,25 @@ describe( 'ViewConversionDispatcher', () => { } ); } ); - describe( 'convertItem', () => { + describe( 'convertItem()', () => { it( 'should pass consumable and additional data to proper converter and return data.output', () => { silenceWarnings(); dispatcher.on( 'documentFragment', ( evt, data, consumable, conversionApi ) => { spy(); - expect( conversionApi.convertItem( viewP, consumableMock, data.position ) ).to.equal( modelP ); - expect( conversionApi.convertItem( viewText, consumableMock, data.position ) ).to.equal( modelText ); + const result1 = conversionApi.convertItem( viewP, consumableMock, data.position ); + expect( result1 ).instanceof( ModelRange ); + expect( result1.start.path ).to.deep.equal( [ 0 ] ); + expect( result1.end.path ).to.deep.equal( [ 1 ] ); + expect( first( result1.getItems() ) ).to.equal( modelP ); + + const result2 = conversionApi.convertItem( viewText, consumableMock, data.position ); + expect( result2 ).instanceof( ModelRange ); + expect( result2.start.path ).to.deep.equal( [ 1 ] ); + expect( result2.end.path ).to.deep.equal( [ 7 ] ); + expect( first( result2.getItems() ) ).to.instanceof( ModelTextProxy ); + expect( first( result2.getItems() ).data ).to.equal( 'foobar' ); } ); dispatcher.convert( new ViewDocumentFragment() ); @@ -320,7 +467,7 @@ describe( 'ViewConversionDispatcher', () => { } ); } ); - describe( 'convertChildren', () => { + describe( 'convertChildren()', () => { it( 'should fire conversion for all children of passed element and return conversion results ' + 'wrapped in document fragment', () => { silenceWarnings(); @@ -328,15 +475,18 @@ describe( 'ViewConversionDispatcher', () => { dispatcher.on( 'documentFragment', ( evt, data, consumable, conversionApi ) => { spy(); - const result = conversionApi.convertChildren( data.input, consumableMock, data ); + const result = conversionApi.convertChildren( data.input, consumableMock, ModelPosition.createAt( rootMock ) ); - expect( result ).to.be.instanceof( ModelDocumentFragment ); - expect( result.childCount ).to.equal( 2 ); - expect( result.getChild( 0 ) ).to.equal( modelP ); - expect( result.getChild( 1 ) ).to.equal( modelText ); + expect( result ).to.be.instanceof( ModelRange ); + expect( result.start.path ).to.deep.equal( [ 0 ] ); + expect( result.end.path ).to.deep.equal( [ 7 ] ); + expect( Array.from( result.getItems() ) ).to.length( 2 ); + expect( Array.from( result.getItems() )[ 0 ] ).to.equal( modelP ); + expect( Array.from( result.getItems() )[ 1 ] ).to.instanceof( ModelTextProxy ); + expect( Array.from( result.getItems() )[ 1 ].data ).to.equal( 'foobar' ); } ); - dispatcher.convert( new ViewDocumentFragment( [ viewP, viewText ] ), { foo: 'bar' } ); + dispatcher.convert( new ViewDocumentFragment( [ viewP, viewText ] ) ); expect( spy.calledOnce ).to.be.true; expect( spyP.calledOnce ).to.be.true; @@ -349,12 +499,15 @@ describe( 'ViewConversionDispatcher', () => { dispatcher.on( 'documentFragment', ( evt, data, consumable, conversionApi ) => { spy(); - const result = conversionApi.convertChildren( data.input, consumableMock, data ); + const result = conversionApi.convertChildren( data.input, consumableMock, ModelPosition.createAt( rootMock ) ); - expect( result ).to.be.instanceof( ModelDocumentFragment ); - expect( result.childCount ).to.equal( 2 ); - expect( result.getChild( 0 ) ).to.equal( modelP ); - expect( result.getChild( 1 ) ).to.equal( modelText ); + expect( result ).to.be.instanceof( ModelRange ); + expect( result.start.path ).to.deep.equal( [ 0 ] ); + expect( result.end.path ).to.deep.equal( [ 7 ] ); + expect( Array.from( result.getItems() ) ).to.length( 2 ); + expect( Array.from( result.getItems() )[ 0 ] ).to.equal( modelP ); + expect( Array.from( result.getItems() )[ 1 ] ).to.instanceof( ModelTextProxy ); + expect( Array.from( result.getItems() )[ 1 ].data ).to.equal( 'foobar' ); } ); dispatcher.convert( new ViewDocumentFragment( [ viewArray, viewP, viewDiv, viewText, viewNull ] ) ); @@ -367,6 +520,93 @@ describe( 'ViewConversionDispatcher', () => { log.warn.restore(); } ); } ); + + describe( 'splitToAllowedParent()', () => { + beforeEach( () => { + model.schema.register( 'paragraph', { + allowIn: '$root' + } ); + } ); + + it( 'should return current position if element is allowed on this position', () => { + const spy = sinon.spy(); + + model.schema.register( 'span', { + allowIn: 'paragraph' + } ); + + dispatcher.on( 'documentFragment', ( evt, data, consumable, conversionApi ) => { + const paragraph = conversionApi.writer.createElement( 'paragraph' ); + const span = conversionApi.writer.createElement( 'span' ); + const position = ModelPosition.createAt( paragraph ); + + const result = conversionApi.splitToAllowedParent( span, position ); + + expect( result ).to.deep.equal( { position } ); + spy(); + } ); + + dispatcher.convert( new ViewDocumentFragment() ); + sinon.assert.calledOnce( spy ); + } ); + + it( 'should split position to allowed ancestor if element is allowed in one of ancestors', () => { + const spy = sinon.spy(); + + model.schema.register( 'section', { + allowIn: '$root' + } ); + model.schema.register( 'span', { + allowIn: 'paragraph' + } ); + model.schema.extend( 'paragraph', { + allowIn: 'section' + } ); + + dispatcher.on( 'documentFragment', ( evt, data, consumable, conversionApi ) => { + const section = conversionApi.writer.createElement( 'section' ); + const paragraph = conversionApi.writer.createElement( 'paragraph' ); + const span = conversionApi.writer.createElement( 'span' ); + conversionApi.writer.insert( paragraph, section ); + conversionApi.writer.insert( span, paragraph ); + + const position = ModelPosition.createAt( span ); + + const paragraph2 = conversionApi.writer.createElement( 'paragraph' ); + const result = conversionApi.splitToAllowedParent( paragraph2, position ); + + expect( result ).to.deep.equal( { + position: ModelPosition.createAfter( paragraph ), + endElement: paragraph.parent.getChild( 1 ).getChild( 0 ) + } ); + + spy(); + } ); + + dispatcher.convert( new ViewDocumentFragment() ); + sinon.assert.calledOnce( spy ); + } ); + + it( 'should return null if element is not allowed in position and any of ancestors', () => { + const spy = sinon.spy(); + + model.schema.register( 'span' ); + + dispatcher.on( 'documentFragment', ( evt, data, consumable, conversionApi ) => { + const paragraph = conversionApi.writer.createElement( 'paragraph' ); + const span = conversionApi.writer.createElement( 'span' ); + const position = ModelPosition.createAt( paragraph ); + + const result = conversionApi.splitToAllowedParent( span, position ); + + expect( result ).to.null; + spy(); + } ); + + dispatcher.convert( new ViewDocumentFragment() ); + sinon.assert.calledOnce( spy ); + } ); + } ); } ); // Silences warnings that pop up in tests. Use when the test checks a specific functionality and we are not interested in those logs. From 4a7983359e2e1cd76c70b45b9f069875475eac3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Mon, 29 Jan 2018 23:09:20 +0100 Subject: [PATCH 14/42] Add temporary condition for issue with multiple matchers. --- src/conversion/buildviewconverter.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/conversion/buildviewconverter.js b/src/conversion/buildviewconverter.js index 0a6411c96..143307a04 100644 --- a/src/conversion/buildviewconverter.js +++ b/src/conversion/buildviewconverter.js @@ -503,6 +503,12 @@ class ViewConverterBuilder { continue; } + // Tmp fix because multiple matchers are not properly matched and consumed. + // See https://github.com/ckeditor/ckeditor5-engine/issues/1257. + if ( data.output ) { + continue; + } + writer.insert( modelElement, data.position ); data.output = Range.createOn( modelElement ); From 6d865dc9c2b1428258b3869b4bba1e7d9a8674cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Mon, 29 Jan 2018 23:10:13 +0100 Subject: [PATCH 15/42] Uncomment test that didn't work with previous conversion. --- tests/conversion/buildviewconverter.js | 49 ++++++++++++-------------- 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/tests/conversion/buildviewconverter.js b/tests/conversion/buildviewconverter.js index 4ef814d96..b6f5eb1da 100644 --- a/tests/conversion/buildviewconverter.js +++ b/tests/conversion/buildviewconverter.js @@ -515,32 +515,29 @@ describe( 'View converter builder', () => { expect( modelToString( conversionResult ) ).to.equal( 'foo' ); } ); - // TMP We can't make this test work for now. - // See https://github.com/ckeditor/ckeditor5-engine/issues/1213#issuecomment-354454906 - // - // it( 'should filter out structure that is wrong with schema - attributes', () => { - // buildViewConverter().for( dispatcher ).fromElement( 'p' ).toElement( 'paragraph' ); - // buildViewConverter().for( dispatcher ).fromElement( 'strong' ).toAttribute( 'bold', true ); - - // // Disallow bold in paragraph>$text. - // schema.addAttributeCheck( ( ctx, attributeName ) => { - // if ( ctx.endsWith( 'paragraph $text' ) && attributeName == 'bold' ) { - // return false; - // } - // } ); - - // dispatcher.on( 'element', convertToModelFragment(), { priority: 'lowest' } ); - - // const viewElement = new ViewContainerElement( 'p', null, - // new ViewAttributeElement( 'strong', null, - // new ViewText( 'foo' ) - // ) - // ); - - // const conversionResult = dispatcher.convert( viewElement, context ); - - // expect( modelToString( conversionResult ) ).to.equal( 'foo' ); - // } ); + it( 'should filter out structure that is wrong with schema - attributes', () => { + buildViewConverter().for( dispatcher ).fromElement( 'p' ).toElement( 'paragraph' ); + buildViewConverter().for( dispatcher ).fromElement( 'strong' ).toAttribute( 'bold', true ); + + // Disallow bold in paragraph>$text. + schema.addAttributeCheck( ( ctx, attributeName ) => { + if ( ctx.endsWith( 'paragraph $text' ) && attributeName == 'bold' ) { + return false; + } + } ); + + dispatcher.on( 'element', convertToModelFragment(), { priority: 'lowest' } ); + + const viewElement = new ViewContainerElement( 'p', null, + new ViewAttributeElement( 'strong', null, + new ViewText( 'foo' ) + ) + ); + + const conversionResult = dispatcher.convert( viewElement, context ); + + expect( modelToString( conversionResult ) ).to.equal( 'foo' ); + } ); it( 'should not set attribute when it is not allowed', () => { buildViewConverter().for( dispatcher ).fromElement( 'p' ).toElement( 'paragraph' ); From 3c187b056857ebf7a702a83ebe54321f3f479f58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 30 Jan 2018 00:17:57 +0100 Subject: [PATCH 16/42] Simplified toElement conversion and increased CC of ViewConverterBuilder. --- src/conversion/buildviewconverter.js | 10 +------ tests/conversion/buildviewconverter.js | 36 ++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/conversion/buildviewconverter.js b/src/conversion/buildviewconverter.js index 143307a04..98ccd618b 100644 --- a/src/conversion/buildviewconverter.js +++ b/src/conversion/buildviewconverter.js @@ -335,16 +335,8 @@ class ViewConverterBuilder { // // after: [] // before: [] - } else if ( childrenOutput.end.parent !== childrenOutput.end.root ) { - output.end = Position.createAfter( childrenOutput.end.parent ); - - // Finally when we are sure that element and its parent was not split we need to put selection - // after element. - // - // after: [] - // before: [] } else { - output.end = Position.createAfter( modelElement ); + output.end = Position.createAfter( childrenOutput.end.parent ); } data.output = output; diff --git a/tests/conversion/buildviewconverter.js b/tests/conversion/buildviewconverter.js index b6f5eb1da..dd2248916 100644 --- a/tests/conversion/buildviewconverter.js +++ b/tests/conversion/buildviewconverter.js @@ -125,6 +125,42 @@ describe( 'View converter builder', () => { expect( modelToString( conversionResult ) ).to.equal( '' ); } ); + it( 'should convert to model element when element children are not allowed in parent (empty split elements should be removed)', () => { + schema.register( 'section', { + inheritAllFrom: '$block' + } ); + + buildViewConverter().for( dispatcher ) + .fromElement( 'p' ) + .toElement( 'paragraph' ); + + buildViewConverter().for( dispatcher ) + .fromElement( 'section' ) + .toElement( 'section' ); + + const input = new ViewContainerElement( 'p', null, [ + new ViewText( 'foo' ), + new ViewContainerElement( 'section', null, [ + new ViewContainerElement( 'p', null, [ + new ViewText( 'abc' ), + new ViewContainerElement( 'section' ), + new ViewText( 'cde' ), + ] ) + ] ), + new ViewText( 'bar' ), + ] ); + + const conversionResult = dispatcher.convert( input ); + + expect( modelToString( conversionResult ) ).to.equal( + 'foo' + + 'abc' + + '
' + + 'cde' + + 'bar' + ); + } ); + it( 'should convert from view element to model attribute', () => { buildViewConverter().for( dispatcher ).fromElement( 'strong' ).toAttribute( 'bold', true ); From 52250473ce9b3c0cfd9baba5e662195937975ad1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 30 Jan 2018 01:21:50 +0100 Subject: [PATCH 17/42] Increased cc of ViewConversionDispatcher. --- tests/conversion/viewconversiondispatcher.js | 45 ++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/tests/conversion/viewconversiondispatcher.js b/tests/conversion/viewconversiondispatcher.js index 28fa57d57..67c9a316c 100644 --- a/tests/conversion/viewconversiondispatcher.js +++ b/tests/conversion/viewconversiondispatcher.js @@ -342,6 +342,51 @@ describe( 'ViewConversionDispatcher', () => { expect( marker1.end.path ).to.deep.equal( [ 4 ] ); expect( marker2.start.path ).to.deep.equal( marker2.end.path ).to.deep.equal( [ 3 ] ); } ); + + it( 'should convert according to given context', () => { + dispatcher = new ViewConversionDispatcher( model, { schema: model.schema } ); + + const spy = sinon.spy(); + const viewElement = new ViewContainerElement( 'third' ); + let checkChildResult; + + model.schema.register( 'first', { + allowIn: '$root' + } ); + model.schema.register( 'second', { + allowIn: 'first' + } ); + model.schema.register( 'third', { + allowIn: 'second', + disallowIn: 'first' + } ); + + dispatcher.on( 'element:third', ( evt, data, consumable, conversionApi ) => { + spy(); + checkChildResult = conversionApi.schema.checkChild( data.position, 'third' ); + } ); + + // Default context $root. + dispatcher.convert( viewElement ); + sinon.assert.calledOnce( spy ); + expect( checkChildResult ).to.false; + + // SchemaDefinition as context. + dispatcher.convert( viewElement, [ 'first' ] ); + sinon.assert.calledTwice( spy ); + expect( checkChildResult ).to.false; + + // Position as context. + const fragment = new ModelDocumentFragment( [ + new ModelElement( 'first', { foo: 'bar' }, [ + new ModelElement( 'second', null ) + ] ) + ] ); + + dispatcher.convert( viewElement, new ModelPosition( fragment, [ 0, 0, 0 ] ) ); + sinon.assert.calledThrice( spy ); + expect( checkChildResult ).to.true; + } ); } ); describe( 'conversionApi', () => { From 0b2ea2bdcdca4be40fa42dc65e736848d22981bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 30 Jan 2018 11:07:16 +0100 Subject: [PATCH 18/42] Changed Schema#findAllowedParent() limitChecker to limitElement. --- src/model/schema.js | 7 +++---- tests/model/schema.js | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/model/schema.js b/src/model/schema.js index cb8f679b3..a68dc3b96 100644 --- a/src/model/schema.js +++ b/src/model/schema.js @@ -681,11 +681,10 @@ export default class Schema { * * @params {module:engine/model/node~Node} node Node for which allowed parent should be found. * @params {module:engine/model/position~Position} position Position from searching will start. - * @params {Function} [limitChecker] When function is defined and returns true - * then stops searching and returns null as a search result. Function gets current parent as a parameter. + * @params {module:engine/model/element~Element} [limitElement] Custom limit element. * @returns {module:engine/model/element~Element|null} element Allowed parent or null if nothing was found. */ - findAllowedParent( node, position, limitChecker ) { + findAllowedParent( node, position, limitElement ) { let parent = position.parent; while ( parent ) { @@ -697,7 +696,7 @@ export default class Schema { return null; } - if ( typeof limitChecker == 'function' && limitChecker( parent ) ) { + if ( parent === limitElement ) { return null; } diff --git a/tests/model/schema.js b/tests/model/schema.js index 55123fe00..ef07b33d6 100644 --- a/tests/model/schema.js +++ b/tests/model/schema.js @@ -1189,7 +1189,7 @@ describe( 'Schema', () => { expect( parent ).to.null; } ); - it( 'should use custom limit checker nad return null if returns true', () => { + it( 'should use custom limit element nad return null if is reached', () => { // $root is allowed ancestor for blockQuote. const node = new Element( 'blockQuote' ); @@ -1197,7 +1197,7 @@ describe( 'Schema', () => { node, Position.createAt( r1bQp ), // However lest stop searching when blockQuote is reached. - element => element.name == 'blockQuote' + r1bQ ); expect( parent ).to.null; From 91adebc865c3b7ecbcc4cc7289e5dc8716dc182c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 30 Jan 2018 11:08:24 +0100 Subject: [PATCH 19/42] Change the way of storing context position in ViewConversionDispatcher. --- src/conversion/viewconversiondispatcher.js | 32 ++++++++++++++-------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/src/conversion/viewconversiondispatcher.js b/src/conversion/viewconversiondispatcher.js index 1106b032f..e80c52939 100644 --- a/src/conversion/viewconversiondispatcher.js +++ b/src/conversion/viewconversiondispatcher.js @@ -123,6 +123,18 @@ export default class ViewConversionDispatcher { */ this._splitElements = new Set(); + /** + * Position inside top element of context tree. + * + * At the beginning of each conversion fragment of model tree is created according to given context. + * {@link module:engine/view/item~Item View items} are converted to the top element of created fragment. + * This makes possible to precisely check converted items by {@link module:engine/model/schema~Schema}. + * + * @private + * @type {module:engine/model/position~Position|null} + */ + this._contextPosition = null; + /** * Interface passed by dispatcher to the events callbacks. * @@ -156,10 +168,9 @@ export default class ViewConversionDispatcher { const consumable = ViewConsumable.createFrom( viewItem ); - // Create model tree according to given context. Elements will be converted to the top element of this tree. - // Thanks to this schema will be able check items precisely. - // Top element of context tree is marked by a `isContextTree` attribute. - const position = contextToPosition( context, writer ); + // Create context tree and store position in the top element. + // Items will be converted according to this position. + this._contextPosition = contextToPosition( context, writer ); // Store writer in current conversion as a conversion API. this.conversionApi.writer = writer; @@ -169,7 +180,7 @@ export default class ViewConversionDispatcher { this.conversionApi.data = {}; // Do the conversion. - const range = this._convertItem( viewItem, consumable, position ); + const range = this._convertItem( viewItem, consumable, this._contextPosition ); // Conversion result is always a document fragment so let's create this fragment. const documentFragment = writer.createDocumentFragment(); @@ -180,7 +191,7 @@ export default class ViewConversionDispatcher { this._removeEmptySplitElements(); // Move all items that was converted to context tree to document fragment. - for ( const item of Array.from( position.parent.getChildren() ) ) { + for ( const item of Array.from( this._contextPosition.parent.getChildren() ) ) { writer.append( item, documentFragment ); } @@ -188,6 +199,9 @@ export default class ViewConversionDispatcher { documentFragment.markers = extractMarkersFromModelFragment( documentFragment, writer ); } + // Clear context position. + this._contextPosition = null; + // Clear split elements. this._splitElements.clear(); @@ -256,12 +270,8 @@ export default class ViewConversionDispatcher { * @see module:engine/conversion/viewconversiondispatcher~ViewConversionApi#splitToAllowedParent */ _splitToAllowedParent( element, position ) { - function checkLimit( node ) { - return node.hasAttribute( 'isContextTree' ); - } - // Try to find allowed parent. - const allowedParent = this.conversionApi.schema.findAllowedParent( element, position, checkLimit ); + const allowedParent = this.conversionApi.schema.findAllowedParent( element, position, this._contextPosition.parent ); // When there is no parent that allows to insert element then return `null`. if ( !allowedParent ) { From b2d4c6b377dcad00916a7dbdcf11dc6008a09548 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 30 Jan 2018 11:52:31 +0100 Subject: [PATCH 20/42] Improved docs. --- src/conversion/buildviewconverter.js | 4 ++-- src/conversion/viewconversiondispatcher.js | 15 +++++++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/conversion/buildviewconverter.js b/src/conversion/buildviewconverter.js index 98ccd618b..1884c9597 100644 --- a/src/conversion/buildviewconverter.js +++ b/src/conversion/buildviewconverter.js @@ -326,8 +326,8 @@ class ViewConverterBuilder { // // before: [] // after: [] - if ( splitResult.endElement ) { - output.end = Position.createAt( splitResult.endElement ); + if ( splitResult.cursorParent ) { + output.end = Position.createAt( splitResult.cursorParent ); // When element is converted on target position (without splitting) we need to move range // after this element but we need to take into consideration that children could split our diff --git a/src/conversion/viewconversiondispatcher.js b/src/conversion/viewconversiondispatcher.js index e80c52939..5ce105402 100644 --- a/src/conversion/viewconversiondispatcher.js +++ b/src/conversion/viewconversiondispatcher.js @@ -175,8 +175,7 @@ export default class ViewConversionDispatcher { // Store writer in current conversion as a conversion API. this.conversionApi.writer = writer; - // Additional date available between conversions. - // Needed when one converter needs to leave some data for the oder converters. + // Custom data stored by converter for conversion process. this.conversionApi.data = {}; // Do the conversion. @@ -296,7 +295,7 @@ export default class ViewConversionDispatcher { return { position: data.position, - endElement: data.range.end.parent + cursorParent: data.range.end.parent }; } @@ -413,6 +412,7 @@ function extractMarkersFromModelFragment( modelItem, writer ) { return markers; } +// Creates model fragment according to given context and returns position in top element. function contextToPosition( contextDefinition, writer ) { let position; @@ -493,7 +493,7 @@ function contextToPosition( contextDefinition, writer ) { * @method #splitToAllowedParent * @param {module:engine/model/position~Position} position Position on which element is going to be inserted. * @param {module:engine/model/element~Node} element Element to insert. - * @returns TODO + * @returns {SplitToAllowedParentResult} Split result. */ /** @@ -501,3 +501,10 @@ function contextToPosition( contextDefinition, writer ) { * * @param {Object} #data */ + +/** + * @typedef {Object} SplitToAllowedParentResult + * @property {module:engine/model/position~Position} position between split elements. + * @property {module:engine/model/element~Element} [cursorParent] Element inside which cursor should be placed to + * continue conversion. When element is not defined it means that there was no split. + */ From 32d1dafc0ef26d66669bf3eca2f8286361acee38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 30 Jan 2018 17:57:18 +0100 Subject: [PATCH 21/42] Changed structure of data object passed to conversion callbacks. --- src/conversion/viewconversiondispatcher.js | 157 ++++++----- tests/conversion/viewconversiondispatcher.js | 260 +++++++++---------- 2 files changed, 203 insertions(+), 214 deletions(-) diff --git a/src/conversion/viewconversiondispatcher.js b/src/conversion/viewconversiondispatcher.js index 5ce105402..aed6e9d6f 100644 --- a/src/conversion/viewconversiondispatcher.js +++ b/src/conversion/viewconversiondispatcher.js @@ -26,25 +26,22 @@ import log from '@ckeditor/ckeditor5-utils/src/log'; * `ViewConversionDispatcher` fires corresponding events. Special callbacks called "converters" should listen to * `ViewConversionDispatcher` for those events. * - * Each callback, as a first argument, is passed a special object `data` that has `input` and `output` properties. - * `input` property contains {@link module:engine/view/node~Node view node} or + * Each callback, as a first argument, is passed a special object `data` that has `viewItem`, `cursorPosition` and + * `modelRange` properties. `viewItem` property contains {@link module:engine/view/node~Node view node} or * {@link module:engine/view/documentfragment~DocumentFragment view document fragment} - * that is converted at the moment and might be handled by the callback. `output` property should be used to save the result - * of conversion. Keep in mind that the `data` parameter is customizable and may contain other values - see - * {@link ~ViewConversionDispatcher#convert}. It is also shared by reference by all callbacks - * listening to given event. **Note**: in view to model conversion - `data` contains `context` property that is an array - * of {@link module:engine/model/element~Element model elements}. These are model elements that will be the parent of currently - * converted view item. `context` property is used in examples below. - * - * The second parameter passed to a callback is an instance of {@link module:engine/conversion/viewconsumable~ViewConsumable}. It stores - * information about what parts of processed view item are still waiting to be handled. After a piece of view item - * was converted, appropriate consumable value should be {@link module:engine/conversion/viewconsumable~ViewConsumable#consume consumed}. + * that is converted at the moment and might be handled by the callback. `modelRange` property should be used to save the result + * of conversion and is always a {@link module:engine/model/range~Range} when conversion result is correct. + * `cursorPosition` property is a {@link module:engine/model/position~Position position} on which conversion result will be inserted + * and is a context according to {@link module:engine/model/schema~Schema schema} will be checked before the conversion. + * See also {@link ~ViewConversionDispatcher#convert}. It is also shared by reference by all callbacks listening to given event. * * The third parameter passed to a callback is an instance of {@link ~ViewConversionDispatcher} * which provides additional tools for converters. * * Examples of providing callbacks for `ViewConversionDispatcher`: * + * TODO - update samples. + * * // Converter for paragraphs (

). * viewDispatcher.on( 'element:p', ( evt, data, consumable, conversionApi ) => { * const paragraph = new ModelElement( 'paragraph' ); @@ -166,20 +163,22 @@ export default class ViewConversionDispatcher { return this._model.change( writer => { this.fire( 'viewCleanup', viewItem ); - const consumable = ViewConsumable.createFrom( viewItem ); - // Create context tree and store position in the top element. // Items will be converted according to this position. this._contextPosition = contextToPosition( context, writer ); - // Store writer in current conversion as a conversion API. + // Store writer in conversion as a conversion API + // to be sure that conversion process will use the same batch. this.conversionApi.writer = writer; + // Create consumable values list for conversion process. + this.conversionApi.consumable = ViewConsumable.createFrom( viewItem ); + // Custom data stored by converter for conversion process. this.conversionApi.data = {}; // Do the conversion. - const range = this._convertItem( viewItem, consumable, this._contextPosition ); + const range = this._convertItem( viewItem, this._contextPosition ); // Conversion result is always a document fragment so let's create this fragment. const documentFragment = writer.createDocumentFragment(); @@ -217,19 +216,19 @@ export default class ViewConversionDispatcher { * @private * @see module:engine/conversion/viewconversiondispatcher~ViewConversionApi#convertItem */ - _convertItem( input, consumable, position ) { - const data = Object.assign( { input, position, output: null } ); + _convertItem( viewItem, cursorPosition ) { + const data = Object.assign( { viewItem, cursorPosition, modelRange: null } ); - if ( input.is( 'element' ) ) { - this.fire( 'element:' + input.name, data, consumable, this.conversionApi ); - } else if ( input.is( 'text' ) ) { - this.fire( 'text', data, consumable, this.conversionApi ); + if ( viewItem.is( 'element' ) ) { + this.fire( 'element:' + viewItem.name, data, this.conversionApi ); + } else if ( viewItem.is( 'text' ) ) { + this.fire( 'text', data, this.conversionApi ); } else { - this.fire( 'documentFragment', data, consumable, this.conversionApi ); + this.fire( 'documentFragment', data, this.conversionApi ); } // Handle incorrect `data.output`. - if ( data.output && !( data.output instanceof ModelRange ) ) { + if ( data.modelRange && !( data.modelRange instanceof ModelRange ) ) { /** * Incorrect conversion result was dropped. * @@ -238,39 +237,42 @@ export default class ViewConversionDispatcher { * * @error view-conversion-dispatcher-incorrect-result */ - log.warn( 'view-conversion-dispatcher-incorrect-result: Incorrect conversion result was dropped.', [ input, data.output ] ); + log.warn( + 'view-conversion-dispatcher-incorrect-result: Incorrect conversion result was dropped.', + [ viewItem, data.modelRange ] + ); return null; } - return data.output; + return data.modelRange; } /** * @private * @see module:engine/conversion/viewconversiondispatcher~ViewConversionApi#convertChildren */ - _convertChildren( input, consumable, position ) { - const output = new ModelRange( position ); + _convertChildren( viewItem, cursorPosition ) { + const childrenRange = new ModelRange( cursorPosition ); - for ( const viewChild of Array.from( input.getChildren() ) ) { - const range = this._convertItem( viewChild, consumable, output.end ); + for ( const viewChild of Array.from( viewItem.getChildren() ) ) { + const itemRange = this._convertItem( viewChild, childrenRange.end ); - if ( range instanceof ModelRange ) { - output.end = range.end; + if ( itemRange instanceof ModelRange ) { + childrenRange.end = itemRange.end; } } - return output; + return childrenRange; } /** * @private * @see module:engine/conversion/viewconversiondispatcher~ViewConversionApi#splitToAllowedParent */ - _splitToAllowedParent( element, position ) { + _splitToAllowedParent( element, cursorPosition ) { // Try to find allowed parent. - const allowedParent = this.conversionApi.schema.findAllowedParent( element, position, this._contextPosition.parent ); + const allowedParent = this.conversionApi.schema.findAllowedParent( element, cursorPosition, this._contextPosition.parent ); // When there is no parent that allows to insert element then return `null`. if ( !allowedParent ) { @@ -278,24 +280,24 @@ export default class ViewConversionDispatcher { } // When current position parent allows to insert element then return this position. - if ( allowedParent === position.parent ) { - return { position }; + if ( allowedParent === cursorPosition.parent ) { + return { position: cursorPosition }; } // Split element to allowed parent. - const data = this.conversionApi.writer.split( position, allowedParent ); + const splitResult = this.conversionApi.writer.split( cursorPosition, allowedParent ); // Remember all elements that are created as a result of split. // This is important because at the end of conversion we want to remove all empty split elements. - for ( const pos of data.range.getPositions() ) { - if ( !pos.isEqual( data.position ) ) { - this._splitElements.add( pos.parent ); + for ( const position of splitResult.range.getPositions() ) { + if ( !position.isEqual( splitResult.position ) ) { + this._splitElements.add( position.parent ); } } return { - position: data.position, - cursorParent: data.range.end.parent + position: splitResult.position, + cursorParent: splitResult.range.end.parent }; } @@ -339,20 +341,16 @@ export default class ViewConversionDispatcher { * all elements conversion or to conversion of specific elements. * * @event element - * @param {Object} data Object containing conversion input and a placeholder for conversion output and possibly other - * values (see {@link #convert}). - * Keep in mind that this object is shared by reference between all callbacks that will be called. - * This means that callbacks can add their own values if needed, - * and those values will be available in other callbacks. - * @param {module:engine/view/element~Element} data.input Converted element. - * @param {*} data.output The current state of conversion result. Every change to converted element should - * be reflected by setting or modifying this property. - * @param {module:engine/conversion/viewconsumable~ViewConsumable} consumable Values to consume. - * @param {Object} conversionApi Conversion interface to be used by callback, passed in `ViewConversionDispatcher` constructor. - * Besides of properties passed in constructor, it also has `convertItem` and `convertChildren` methods which are references - * to {@link #_convertItem} and - * {@link ~ViewConversionDispatcher#_convertChildren}. Those methods are needed to convert - * the whole view-tree they were exposed in `conversionApi` for callbacks. + * @param {Object} data Object containing viewItem to convert, cursorPosition as a conversion position and placeholder + * for modelRange that is a conversion result. Keep in mind that this object is shared by reference between all + * callbacks that will be called. This means that callbacks can override values if needed, and those values will + * be available in other callbacks. + * @param {module:engine/view/item~Item} data.viewItem Converted item. + * @param {module:engine/model/position~Position} data.cursorPosition Target position for current item. + * @param {module:engine/model/range~Range} data.modelRange The current state of conversion result. Every change to + * converted element should be reflected by setting or modifying this property. + * @param {ViewConversionApi} conversionApi Conversion interface to be used by callback, passed in + * `ViewConversionDispatcher` constructor. */ /** @@ -442,8 +440,7 @@ function contextToPosition( contextDefinition, writer ) { * and is passed as one of parameters when {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher dispatcher} * fires it's events. * - * `ViewConversionApi` object is built by {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher} constructor. - * The exact list of properties of this object is determined by the object passed to the constructor. + * TODO more explanation. * * @interface ViewConversionApi */ @@ -451,20 +448,16 @@ function contextToPosition( contextDefinition, writer ) { /** * Starts conversion of given item by firing an appropriate event. * - * Every fired event is passed (as first parameter) an object with `output` property. Every event may set and/or - * modify that property. When all callbacks are done, the final value of `output` property is returned by this method. - * The `output` must be {@link module:engine/model/range~Range model range} or `null` (as set by default). + * Every fired event is passed (as first parameter) an object with `modelRange` property. Every event may set and/or + * modify that property. When all callbacks are done, the final value of `modelRange` property is returned by this method. + * The `modelRange` must be {@link module:engine/model/range~Range model range} or `null` (as set by default). * * @method #convertItem * @fires module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:element * @fires module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:text * @fires module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:documentFragment - * @param {module:engine/view/documentfragment~DocumentFragment|module:engine/view/element~Element|module:engine/view/text~Text} - * input Item to convert. - * @param {module:engine/conversion/viewconsumable~ViewConsumable} consumable Values to consume. - * @param {module:engine/model/position~Position} position Position of conversion. - * @param {Object} [additionalData] Additional data to be passed in `data` argument when firing `ViewConversionDispatcher` - * events. See also {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:element element event}. + * @param {module:engine/view/item~Item} viewItem Item to convert. + * @param {module:engine/model/position~Position} cursorPosition Position of conversion. * @returns {module:engine/model/range~Range|null} Model range containing result of item conversion, * created and modified by callbacks attached to fired event, or `null` if the conversion result was incorrect. */ @@ -476,14 +469,10 @@ function contextToPosition( contextDefinition, writer ) { * @fires module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:element * @fires module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:text * @fires module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:documentFragment - * @param {module:engine/view/documentfragment~DocumentFragment|module:engine/view/element~Element} - * input Item which children will be converted. - * @param {module:engine/conversion/viewconsumable~ViewConsumable} consumable Values to consume. - * @param {module:engine/model/position~Position} position Position of conversion. - * @param {Object} [additionalData] Additional data to be passed in `data` argument when firing `ViewConversionDispatcher` - * events. See also {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:element element event}. + * @param {module:engine/view/item~Item} viewItem Item to convert. + * @param {module:engine/model/position~Position} cursorPosition Position of conversion. * @returns {module:engine/model/range~Range} Model range containing results of conversion of all children of given item. - * When none of children was converted + * When no children was converted then range is collapsed. */ /** @@ -493,18 +482,22 @@ function contextToPosition( contextDefinition, writer ) { * @method #splitToAllowedParent * @param {module:engine/model/position~Position} position Position on which element is going to be inserted. * @param {module:engine/model/element~Node} element Element to insert. - * @returns {SplitToAllowedParentResult} Split result. + * @returns {Object} Split result. + * @returns {module:engine/model/position~Position} position between split elements. + * @returns {module:engine/model/element~Element} [cursorParent] Element inside which cursor should be placed to + * continue conversion. When element is not defined it means that there was no split. */ /** - * Custom data stored by converter for conversion process. + * Instance of {@link module:engine/conversion/viewconsumable~ViewConsumable}. It stores + * information about what parts of processed view item are still waiting to be handled. After a piece of view item + * was converted, appropriate consumable value should be {@link module:engine/conversion/viewconsumable~ViewConsumable#consume consumed}. * - * @param {Object} #data + * @param {Object} #consumable */ /** - * @typedef {Object} SplitToAllowedParentResult - * @property {module:engine/model/position~Position} position between split elements. - * @property {module:engine/model/element~Element} [cursorParent] Element inside which cursor should be placed to - * continue conversion. When element is not defined it means that there was no split. + * Custom data stored by converter for conversion process. + * + * @param {Object} #data */ diff --git a/tests/conversion/viewconversiondispatcher.js b/tests/conversion/viewconversiondispatcher.js index 67c9a316c..c91ac399b 100644 --- a/tests/conversion/viewconversiondispatcher.js +++ b/tests/conversion/viewconversiondispatcher.js @@ -41,6 +41,12 @@ describe( 'ViewConversionDispatcher', () => { expect( dispatcher.conversionApi ).to.have.property( 'convertChildren' ).that.is.instanceof( Function ); expect( dispatcher.conversionApi ).to.have.property( 'splitToAllowedParent' ).that.is.instanceof( Function ); } ); + + it( 'should have properties', () => { + const dispatcher = new ViewConversionDispatcher( model ); + + expect( dispatcher._splitElements ).to.instanceof( Set ); + } ); } ); describe( 'convert()', () => { @@ -50,6 +56,65 @@ describe( 'ViewConversionDispatcher', () => { dispatcher = new ViewConversionDispatcher( model ); } ); + it( 'should create api for current conversion process', () => { + const viewElement = new ViewContainerElement( 'p', null, new ViewText( 'foobar' ) ); + + // To be sure that both converters was called. + const spy = sinon.spy(); + + // To check that the same writer instance. + let writer; + + // Conversion process properties should be undefined/empty before conversion. + expect( dispatcher.conversionApi.writer ).to.not.ok; + expect( dispatcher.conversionApi.data ).to.not.ok; + expect( dispatcher._splitElements.size ).to.equal( 0 ); + + dispatcher.on( 'element', ( evt, data, conversionApi ) => { + // Check conversion api params. + expect( conversionApi.writer ).to.instanceof( ModelWriter ); + expect( conversionApi.data ).to.deep.equal( {} ); + expect( dispatcher._splitElements.size ).to.equal( 0 ); + + // Remember writer to check in next converter that is exactly the same instance (the same undo step). + writer = conversionApi.writer; + + // Add some data to conversion storage to verify them in next converter. + conversionApi.data.foo = 'bar'; + + // Add empty element and mark as a split result to check in next converter. + dispatcher._splitElements.add( conversionApi.writer.createElement( 'paragraph' ) ); + + // Convert children - this will call second converter. + conversionApi.convertChildren( data.viewItem, data.cursorPosition ); + + spy(); + } ); + + dispatcher.on( 'text', ( evt, data, conversionApi ) => { + // The same writer is used in converters during one conversion. + expect( conversionApi.writer ).to.equal( writer ); + + // Data set by previous converter are remembered. + expect( conversionApi.data ).to.deep.equal( { foo: 'bar' } ); + + // Split element is remembered as well. + expect( dispatcher._splitElements.size ).to.equal( 1 ); + + spy(); + } ); + + dispatcher.convert( viewElement ); + + // To be sure that both converters was called. + sinon.assert.calledTwice( spy ); + + // Conversion process properties should be cleared after conversion. + expect( dispatcher.conversionApi.writer ).to.not.ok; + expect( dispatcher.conversionApi.data ).to.not.ok; + expect( dispatcher._splitElements.size ).to.equal( 0 ); + } ); + it( 'should fire viewCleanup event on converted view part', () => { silenceWarnings(); @@ -83,27 +148,27 @@ describe( 'ViewConversionDispatcher', () => { const spy = sinon.spy(); const viewText = new ViewText( 'foobar' ); - dispatcher.on( 'text', ( evt, data, consumable, conversionApi ) => { + dispatcher.on( 'text', ( evt, data, conversionApi ) => { // Check if this method has been fired. spy(); // Check correctness of passed parameters. expect( evt.name ).to.equal( 'text' ); - expect( data.input ).to.equal( viewText ); - expect( data.position ).to.instanceof( ModelPosition ); + expect( data.viewItem ).to.equal( viewText ); + expect( data.cursorPosition ).to.instanceof( ModelPosition ); // Check whether consumable has appropriate value to consume. - expect( consumable.consume( data.input ) ).to.be.true; + expect( conversionApi.consumable.consume( data.viewItem ) ).to.be.true; // Check whether conversionApi of `dispatcher` has been passed. expect( conversionApi ).to.equal( dispatcher.conversionApi ); - const text = conversionApi.writer.createText( data.input.data ); - conversionApi.writer.insert( text, data.position ); + const text = conversionApi.writer.createText( data.viewItem.data ); + conversionApi.writer.insert( text, data.cursorPosition ); - // Set conversion result to `output` property of `data`. + // Set conversion result to `modelRange` property of `data`. // Later we will check if it was returned by `convert` method. - data.output = ModelRange.createOn( text ); + data.modelRange = ModelRange.createOn( text ); } ); const conversionResult = dispatcher.convert( viewText ); @@ -119,28 +184,28 @@ describe( 'ViewConversionDispatcher', () => { const spy = sinon.spy(); const viewElement = new ViewContainerElement( 'p', { attrKey: 'attrValue' } ); - dispatcher.on( 'element', ( evt, data, consumable, conversionApi ) => { + dispatcher.on( 'element', ( evt, data, conversionApi ) => { // Check if this method has been fired. spy(); // Check correctness of passed parameters. expect( evt.name ).to.equal( 'element:p' ); - expect( data.input ).to.equal( viewElement ); - expect( data.position ).to.instanceof( ModelPosition ); + expect( data.viewItem ).to.equal( viewElement ); + expect( data.cursorPosition ).to.instanceof( ModelPosition ); // Check whether consumable has appropriate value to consume. - expect( consumable.consume( data.input, { name: true } ) ).to.be.true; - expect( consumable.consume( data.input, { attribute: 'attrKey' } ) ).to.be.true; + expect( conversionApi.consumable.consume( data.viewItem, { name: true } ) ).to.be.true; + expect( conversionApi.consumable.consume( data.viewItem, { attribute: 'attrKey' } ) ).to.be.true; // Check whether conversionApi of `dispatcher` has been passed. expect( conversionApi ).to.equal( dispatcher.conversionApi ); const paragraph = conversionApi.writer.createElement( 'paragraph' ); - conversionApi.writer.insert( paragraph, data.position ); + conversionApi.writer.insert( paragraph, data.cursorPosition ); - // Set conversion result to `output` property of `data`. + // Set conversion result to `modelRange` property of `data`. // Later we will check if it was returned by `convert` method. - data.output = ModelRange.createOn( paragraph ); + data.modelRange = ModelRange.createOn( paragraph ); } ); // Use `additionalData` parameter to check if it was passed to the event. @@ -157,27 +222,27 @@ describe( 'ViewConversionDispatcher', () => { const spy = sinon.spy(); const viewFragment = new ViewDocumentFragment(); - dispatcher.on( 'documentFragment', ( evt, data, consumable, conversionApi ) => { + dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => { // Check if this method has been fired. spy(); // Check correctness of passed parameters. expect( evt.name ).to.equal( 'documentFragment' ); - expect( data.input ).to.equal( viewFragment ); - expect( data.position ).to.instanceof( ModelPosition ); + expect( data.viewItem ).to.equal( viewFragment ); + expect( data.cursorPosition ).to.instanceof( ModelPosition ); // Check whether consumable has appropriate value to consume. - expect( consumable.consume( data.input ) ).to.be.true; + expect( conversionApi.consumable.consume( data.viewItem ) ).to.be.true; // Check whether conversionApi of `dispatcher` has been passed. expect( conversionApi ).to.equal( dispatcher.conversionApi ); const text = conversionApi.writer.createText( 'foo' ); - conversionApi.writer.insert( text, data.position ); + conversionApi.writer.insert( text, data.cursorPosition ); - // Set conversion result to `output` property of `data`. + // Set conversion result to `modelRange` property of `data`. // Later we will check if it was returned by `convert` method. - data.output = ModelRange.createOn( text ); + data.modelRange = ModelRange.createOn( text ); } ); // Use `additionalData` parameter to check if it was passed to the event. @@ -189,85 +254,16 @@ describe( 'ViewConversionDispatcher', () => { expect( spy.calledOnce ).to.be.true; } ); - it( 'should add contextual properties to conversion api', () => { - const viewElement = new ViewContainerElement( 'p', null, new ViewText( 'foobar' ) ); - - // To be sure that both converters was called. - const spy = sinon.spy(); - - // To check that the same batch is used across conversion. - let batch; - - // Contextual properties of ConversionApi should be undefined/empty before conversion. - expect( dispatcher.conversionApi.writer ).to.not.ok; - expect( dispatcher.conversionApi.data ).to.not.ok; - expect( dispatcher._splitElements.size ).to.equal( 0 ); - - dispatcher.on( 'element', ( evt, data, consumable, conversionApi ) => { - // Check conversion api params. - expect( conversionApi.writer ).to.instanceof( ModelWriter ); - expect( dispatcher._splitElements ).to.instanceof( Set ); - expect( dispatcher._splitElements.size ).to.equal( 0 ); - expect( conversionApi.data ).to.deep.equal( {} ); - - // Remember writer batch to check in next converter that is exactly the same batch. - batch = conversionApi.writer.batch; - - // Add some data to conversion API to verify them in next converter. - // Set some custom data to conversion api data object. - conversionApi.data.foo = 'bar'; - - // Do the conversion. - const paragraph = conversionApi.writer.createElement( 'paragraph' ); - conversionApi.writer.insert( paragraph, data.position ); - - // Add empty element and mark as a split result to check in next converter. - const splitElement = conversionApi.writer.createElement( 'paragraph' ); - dispatcher._splitElements.add( splitElement ); - conversionApi.writer.insert( splitElement, ModelPosition.createAfter( paragraph ) ); - - // Convert children - this wil call second converter. - conversionApi.convertChildren( data.input, consumable, ModelPosition.createAt( paragraph ) ); - - data.output = ModelRange.createOn( paragraph ); - - spy(); - } ); - - dispatcher.on( 'text', ( evt, data, consumable, conversionApi ) => { - // The same batch is used in converters during one conversion. - expect( conversionApi.writer.batch ).to.equal( batch ); - - // Data set by previous converter are remembered. - expect( conversionApi.data ).to.deep.equal( { foo: 'bar' } ); - - // Split element is remembered as well. - expect( dispatcher._splitElements.size ).to.equal( 1 ); - - spy(); - } ); - - dispatcher.convert( viewElement ); - - // To be sure that both converters was called. - sinon.assert.calledTwice( spy ); - - // Contextual properties of ConversionApi should be cleared after conversion. - expect( dispatcher.conversionApi.writer ).to.not.ok; - expect( dispatcher.conversionApi.data ).to.not.ok; - expect( dispatcher._splitElements.size ).to.equal( 0 ); - } ); - it( 'should remove empty elements that was created as a result of split', () => { const viewElement = new ViewContainerElement( 'p' ); // To be sure that converter was called. const spy = sinon.spy(); - dispatcher.on( 'element', ( evt, data, consumable, conversionApi ) => { + dispatcher.on( 'element', ( evt, data, conversionApi ) => { // First let's convert target element. const paragraph = conversionApi.writer.createElement( 'paragraph' ); - conversionApi.writer.insert( paragraph, data.position ); + conversionApi.writer.insert( paragraph, data.cursorPosition ); // Then add some elements and mark as split. @@ -291,7 +287,8 @@ describe( 'ViewConversionDispatcher', () => { dispatcher._splitElements.add( outerSplit ); dispatcher._splitElements.add( innerSplit ); - data.output = ModelRange.createOn( paragraph ); + data.modelRange = ModelRange.createOn( paragraph ); + data.cursorPosition = data.modelRange.end; // We have the following result: //

[

]

foo

@@ -315,7 +312,8 @@ describe( 'ViewConversionDispatcher', () => { it( 'should extract temporary markers elements from converter element and create static markers list', () => { const viewFragment = new ViewDocumentFragment(); - dispatcher.on( 'documentFragment', ( evt, data, consumable, conversionApi ) => { + dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => { + // Create model fragment. const fragment = new ModelDocumentFragment( [ new ModelText( 'fo' ), new ModelElement( '$marker', { 'data-name': 'marker1' } ), @@ -326,9 +324,11 @@ describe( 'ViewConversionDispatcher', () => { new ModelText( 'ar' ), ] ); - conversionApi.writer.insert( fragment, data.position ); + // Insert to conversion tree. + conversionApi.writer.insert( fragment, data.cursorPosition ); - data.output = ModelRange.createIn( data.position.parent ); + // Create range on this fragment as a conversion result. + data.modelRange = ModelRange.createIn( data.cursorPosition.parent ); } ); const conversionResult = dispatcher.convert( viewFragment ); @@ -361,9 +361,9 @@ describe( 'ViewConversionDispatcher', () => { disallowIn: 'first' } ); - dispatcher.on( 'element:third', ( evt, data, consumable, conversionApi ) => { + dispatcher.on( 'element:third', ( evt, data, conversionApi ) => { spy(); - checkChildResult = conversionApi.schema.checkChild( data.position, 'third' ); + checkChildResult = conversionApi.schema.checkChild( data.cursorPosition, 'third' ); } ); // Default context $root. @@ -390,7 +390,7 @@ describe( 'ViewConversionDispatcher', () => { } ); describe( 'conversionApi', () => { - let spy, spyP, spyText, viewP, viewText, modelP, modelText, consumableMock, rootMock, dispatcher, + let spy, spyP, spyText, viewP, viewText, modelP, modelText, rootMock, dispatcher, spyNull, spyArray, viewDiv, viewNull, viewArray; beforeEach( () => { @@ -406,60 +406,56 @@ describe( 'ViewConversionDispatcher', () => { // Put nodes to documentFragment, this will mock root element and makes possible to create range on them. rootMock = new ModelDocumentFragment( [ modelP, modelText ] ); - consumableMock = {}; - dispatcher = new ViewConversionDispatcher( model, { schema: model.schema } ); - dispatcher.on( 'element:p', ( evt, data, consumable ) => { + dispatcher.on( 'element:p', ( evt, data ) => { spyP(); - expect( consumable ).to.equal( consumableMock ); - - data.output = ModelRange.createOn( modelP ); + data.modelRange = ModelRange.createOn( modelP ); + data.cursorPosition = data.modelRange.end; } ); - dispatcher.on( 'text', ( evt, data, consumable ) => { + dispatcher.on( 'text', ( evt, data ) => { spyText(); - expect( consumable ).to.equal( consumableMock ); - - data.output = ModelRange.createOn( modelText ); + data.modelRange = ModelRange.createOn( modelText ); + data.cursorPosition = data.modelRange.end; } ); spyNull = sinon.spy(); spyArray = sinon.spy(); viewDiv = new ViewContainerElement( 'div' ); // Will not be recognized and not converted. - viewNull = new ViewContainerElement( 'null' ); // Will return `null` in `data.output` upon conversion. - viewArray = new ViewContainerElement( 'array' ); // Will return an array in `data.output` upon conversion. + viewNull = new ViewContainerElement( 'null' ); // Will return `null` in `data.modelRange` upon conversion. + viewArray = new ViewContainerElement( 'array' ); // Will return an array in `data.modelRange` upon conversion. dispatcher.on( 'element:null', ( evt, data ) => { spyNull(); - data.output = null; + data.modelRange = null; } ); dispatcher.on( 'element:array', ( evt, data ) => { spyArray(); - data.output = [ new ModelText( 'foo' ) ]; + data.modelRange = [ new ModelText( 'foo' ) ]; } ); } ); describe( 'convertItem()', () => { - it( 'should pass consumable and additional data to proper converter and return data.output', () => { + it( 'should call proper converter and return correct data', () => { silenceWarnings(); - dispatcher.on( 'documentFragment', ( evt, data, consumable, conversionApi ) => { + dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => { spy(); - const result1 = conversionApi.convertItem( viewP, consumableMock, data.position ); + const result1 = conversionApi.convertItem( viewP, data.cursorPosition ); expect( result1 ).instanceof( ModelRange ); expect( result1.start.path ).to.deep.equal( [ 0 ] ); expect( result1.end.path ).to.deep.equal( [ 1 ] ); expect( first( result1.getItems() ) ).to.equal( modelP ); - const result2 = conversionApi.convertItem( viewText, consumableMock, data.position ); + const result2 = conversionApi.convertItem( viewText, data.cursorPosition ); expect( result2 ).instanceof( ModelRange ); expect( result2.start.path ).to.deep.equal( [ 1 ] ); expect( result2.end.path ).to.deep.equal( [ 7 ] ); @@ -477,11 +473,11 @@ describe( 'ViewConversionDispatcher', () => { it( 'should do nothing if element was not converted', () => { sinon.stub( log, 'warn' ); - dispatcher.on( 'documentFragment', ( evt, data, consumable, conversionApi ) => { + dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => { spy(); - expect( conversionApi.convertItem( viewDiv ) ).to.equal( null ); - expect( conversionApi.convertItem( viewNull ) ).to.equal( null ); + expect( conversionApi.convertItem( viewDiv, data.cursorPosition ) ).to.equal( null ); + expect( conversionApi.convertItem( viewNull, data.cursorPosition ) ).to.equal( null ); } ); dispatcher.convert( new ViewDocumentFragment() ); @@ -496,10 +492,10 @@ describe( 'ViewConversionDispatcher', () => { it( 'should return null if element was incorrectly converted and log a warning', () => { sinon.stub( log, 'warn' ); - dispatcher.on( 'documentFragment', ( evt, data, consumable, conversionApi ) => { + dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => { spy(); - expect( conversionApi.convertItem( viewArray ) ).to.equal( null ); + expect( conversionApi.convertItem( viewArray, data.cursorPosition ) ).to.equal( null ); } ); dispatcher.convert( new ViewDocumentFragment() ); @@ -517,10 +513,10 @@ describe( 'ViewConversionDispatcher', () => { 'wrapped in document fragment', () => { silenceWarnings(); - dispatcher.on( 'documentFragment', ( evt, data, consumable, conversionApi ) => { + dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => { spy(); - const result = conversionApi.convertChildren( data.input, consumableMock, ModelPosition.createAt( rootMock ) ); + const result = conversionApi.convertChildren( data.viewItem, ModelPosition.createAt( rootMock ) ); expect( result ).to.be.instanceof( ModelRange ); expect( result.start.path ).to.deep.equal( [ 0 ] ); @@ -541,10 +537,10 @@ describe( 'ViewConversionDispatcher', () => { it( 'should filter out incorrectly converted elements and log warnings', () => { sinon.stub( log, 'warn' ); - dispatcher.on( 'documentFragment', ( evt, data, consumable, conversionApi ) => { + dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => { spy(); - const result = conversionApi.convertChildren( data.input, consumableMock, ModelPosition.createAt( rootMock ) ); + const result = conversionApi.convertChildren( data.viewItem, ModelPosition.createAt( rootMock ) ); expect( result ).to.be.instanceof( ModelRange ); expect( result.start.path ).to.deep.equal( [ 0 ] ); @@ -580,7 +576,7 @@ describe( 'ViewConversionDispatcher', () => { allowIn: 'paragraph' } ); - dispatcher.on( 'documentFragment', ( evt, data, consumable, conversionApi ) => { + dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => { const paragraph = conversionApi.writer.createElement( 'paragraph' ); const span = conversionApi.writer.createElement( 'span' ); const position = ModelPosition.createAt( paragraph ); @@ -608,7 +604,7 @@ describe( 'ViewConversionDispatcher', () => { allowIn: 'section' } ); - dispatcher.on( 'documentFragment', ( evt, data, consumable, conversionApi ) => { + dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => { const section = conversionApi.writer.createElement( 'section' ); const paragraph = conversionApi.writer.createElement( 'paragraph' ); const span = conversionApi.writer.createElement( 'span' ); @@ -622,7 +618,7 @@ describe( 'ViewConversionDispatcher', () => { expect( result ).to.deep.equal( { position: ModelPosition.createAfter( paragraph ), - endElement: paragraph.parent.getChild( 1 ).getChild( 0 ) + cursorParent: paragraph.parent.getChild( 1 ).getChild( 0 ) } ); spy(); @@ -637,7 +633,7 @@ describe( 'ViewConversionDispatcher', () => { model.schema.register( 'span' ); - dispatcher.on( 'documentFragment', ( evt, data, consumable, conversionApi ) => { + dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => { const paragraph = conversionApi.writer.createElement( 'paragraph' ); const span = conversionApi.writer.createElement( 'span' ); const position = ModelPosition.createAt( paragraph ); From 34b59c505fc864fc0dc9c39588899fd92b22e2e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 30 Jan 2018 23:05:51 +0100 Subject: [PATCH 22/42] Changed format of data returned by conversionApi#convertItem and conversionApi#convertChildren. --- src/conversion/viewconversiondispatcher.js | 42 +++---- tests/conversion/viewconversiondispatcher.js | 114 ++++++------------- 2 files changed, 55 insertions(+), 101 deletions(-) diff --git a/src/conversion/viewconversiondispatcher.js b/src/conversion/viewconversiondispatcher.js index aed6e9d6f..f605f0ce7 100644 --- a/src/conversion/viewconversiondispatcher.js +++ b/src/conversion/viewconversiondispatcher.js @@ -12,9 +12,9 @@ import ModelRange from '../model/range'; import ModelPosition from '../model/position'; import { SchemaContext } from '../model/schema'; +import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import EmitterMixin from '@ckeditor/ckeditor5-utils/src/emittermixin'; import mix from '@ckeditor/ckeditor5-utils/src/mix'; -import log from '@ckeditor/ckeditor5-utils/src/log'; /** * `ViewConversionDispatcher` is a central point of {@link module:engine/view/view view} conversion, which is a process of @@ -178,13 +178,13 @@ export default class ViewConversionDispatcher { this.conversionApi.data = {}; // Do the conversion. - const range = this._convertItem( viewItem, this._contextPosition ); + const { modelRange } = this._convertItem( viewItem, this._contextPosition ); // Conversion result is always a document fragment so let's create this fragment. const documentFragment = writer.createDocumentFragment(); // When there is a conversion result. - if ( range ) { + if ( modelRange ) { // Remove all empty elements that was created as a result of split. this._removeEmptySplitElements(); @@ -227,25 +227,19 @@ export default class ViewConversionDispatcher { this.fire( 'documentFragment', data, this.conversionApi ); } - // Handle incorrect `data.output`. + // Handle incorrect conversion result. if ( data.modelRange && !( data.modelRange instanceof ModelRange ) ) { /** * Incorrect conversion result was dropped. * - * Item may be converted to either {@link module:engine/model/node~Node model node} or - * {@link module:engine/model/documentfragment~DocumentFragment model document fragment}. + * {@link module:engine/model/range~Range Model range} should be a conversion result. * * @error view-conversion-dispatcher-incorrect-result */ - log.warn( - 'view-conversion-dispatcher-incorrect-result: Incorrect conversion result was dropped.', - [ viewItem, data.modelRange ] - ); - - return null; + throw new CKEditorError( 'view-conversion-dispatcher-incorrect-result: Incorrect conversion result was dropped.' ); } - return data.modelRange; + return { modelRange: data.modelRange, cursorPosition: data.cursorPosition }; } /** @@ -253,17 +247,19 @@ export default class ViewConversionDispatcher { * @see module:engine/conversion/viewconversiondispatcher~ViewConversionApi#convertChildren */ _convertChildren( viewItem, cursorPosition ) { - const childrenRange = new ModelRange( cursorPosition ); + const modelRange = new ModelRange( cursorPosition ); + let nextCursorPosition = cursorPosition; for ( const viewChild of Array.from( viewItem.getChildren() ) ) { - const itemRange = this._convertItem( viewChild, childrenRange.end ); + const result = this._convertItem( viewChild, nextCursorPosition ); - if ( itemRange instanceof ModelRange ) { - childrenRange.end = itemRange.end; + if ( result.modelRange instanceof ModelRange ) { + modelRange.end = result.modelRange.end; + nextCursorPosition = result.cursorPosition; } } - return childrenRange; + return { modelRange, cursorPosition: nextCursorPosition }; } /** @@ -440,7 +436,7 @@ function contextToPosition( contextDefinition, writer ) { * and is passed as one of parameters when {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher dispatcher} * fires it's events. * - * TODO more explanation. + * TODO better explanation. * * @interface ViewConversionApi */ @@ -458,8 +454,10 @@ function contextToPosition( contextDefinition, writer ) { * @fires module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:documentFragment * @param {module:engine/view/item~Item} viewItem Item to convert. * @param {module:engine/model/position~Position} cursorPosition Position of conversion. - * @returns {module:engine/model/range~Range|null} Model range containing result of item conversion, + * @returns {Object} result Conversion result. + * @returns {module:engine/model/range~Range|null} result.modelRange Model range containing result of item conversion, * created and modified by callbacks attached to fired event, or `null` if the conversion result was incorrect. + * @returns {module:engine/model/position~Position} result.cursorPosition Position where conversion should be continued. */ /** @@ -471,8 +469,10 @@ function contextToPosition( contextDefinition, writer ) { * @fires module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:documentFragment * @param {module:engine/view/item~Item} viewItem Item to convert. * @param {module:engine/model/position~Position} cursorPosition Position of conversion. - * @returns {module:engine/model/range~Range} Model range containing results of conversion of all children of given item. + * @returns {Object} result Conversion result. + * @returns {module:engine/model/range~Range} result.modelRange Model range containing results of conversion of all children of given item. * When no children was converted then range is collapsed. + * @returns {module:engine/model/position~Position} result.cursorPosition Position where conversion should be continued. */ /** diff --git a/tests/conversion/viewconversiondispatcher.js b/tests/conversion/viewconversiondispatcher.js index c91ac399b..62c633675 100644 --- a/tests/conversion/viewconversiondispatcher.js +++ b/tests/conversion/viewconversiondispatcher.js @@ -18,17 +18,13 @@ import ModelRange from '../../src/model/range'; import ModelWriter from '../../src/model/writer'; import first from '@ckeditor/ckeditor5-utils/src/first'; -import log from '@ckeditor/ckeditor5-utils/src/log'; - -// Stored in case it is silenced and has to be restored. -const logWarn = log.warn; +import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; describe( 'ViewConversionDispatcher', () => { let model; beforeEach( () => { model = new Model(); - log.warn = logWarn; } ); describe( 'constructor()', () => { @@ -116,8 +112,6 @@ describe( 'ViewConversionDispatcher', () => { } ); it( 'should fire viewCleanup event on converted view part', () => { - silenceWarnings(); - sinon.spy( dispatcher, 'fire' ); const viewP = new ViewContainerElement( 'p' ); @@ -127,8 +121,6 @@ describe( 'ViewConversionDispatcher', () => { } ); it( 'should fire proper events', () => { - silenceWarnings(); - const viewText = new ViewText( 'foobar' ); const viewElement = new ViewContainerElement( 'p', null, viewText ); const viewFragment = new ViewDocumentFragment( viewElement ); @@ -444,23 +436,25 @@ describe( 'ViewConversionDispatcher', () => { describe( 'convertItem()', () => { it( 'should call proper converter and return correct data', () => { - silenceWarnings(); - dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => { spy(); - const result1 = conversionApi.convertItem( viewP, data.cursorPosition ); - expect( result1 ).instanceof( ModelRange ); - expect( result1.start.path ).to.deep.equal( [ 0 ] ); - expect( result1.end.path ).to.deep.equal( [ 1 ] ); - expect( first( result1.getItems() ) ).to.equal( modelP ); - - const result2 = conversionApi.convertItem( viewText, data.cursorPosition ); - expect( result2 ).instanceof( ModelRange ); - expect( result2.start.path ).to.deep.equal( [ 1 ] ); - expect( result2.end.path ).to.deep.equal( [ 7 ] ); - expect( first( result2.getItems() ) ).to.instanceof( ModelTextProxy ); - expect( first( result2.getItems() ).data ).to.equal( 'foobar' ); + const pResult = conversionApi.convertItem( viewP, data.cursorPosition ); + expect( pResult.modelRange ).instanceof( ModelRange ); + expect( pResult.modelRange.start.path ).to.deep.equal( [ 0 ] ); + expect( pResult.modelRange.end.path ).to.deep.equal( [ 1 ] ); + expect( first( pResult.modelRange.getItems() ) ).to.equal( modelP ); + expect( pResult.cursorPosition ).to.instanceof( ModelPosition ); + expect( pResult.cursorPosition.path ).to.deep.equal( [ 1 ] ); + + const textResult = conversionApi.convertItem( viewText, data.cursorPosition ); + expect( textResult.modelRange ).instanceof( ModelRange ); + expect( textResult.modelRange.start.path ).to.deep.equal( [ 1 ] ); + expect( textResult.modelRange.end.path ).to.deep.equal( [ 7 ] ); + expect( first( textResult.modelRange.getItems() ) ).to.instanceof( ModelTextProxy ); + expect( first( textResult.modelRange.getItems() ).data ).to.equal( 'foobar' ); + expect( textResult.cursorPosition ).to.instanceof( ModelPosition ); + expect( textResult.cursorPosition.path ).to.deep.equal( [ 7 ] ); } ); dispatcher.convert( new ViewDocumentFragment() ); @@ -471,60 +465,53 @@ describe( 'ViewConversionDispatcher', () => { } ); it( 'should do nothing if element was not converted', () => { - sinon.stub( log, 'warn' ); - dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => { spy(); - expect( conversionApi.convertItem( viewDiv, data.cursorPosition ) ).to.equal( null ); - expect( conversionApi.convertItem( viewNull, data.cursorPosition ) ).to.equal( null ); + expect( conversionApi.convertItem( viewDiv, data.cursorPosition ).modelRange ).to.equal( null ); + expect( conversionApi.convertItem( viewNull, data.cursorPosition ).modelRange ).to.equal( null ); } ); dispatcher.convert( new ViewDocumentFragment() ); expect( spy.calledOnce ).to.be.true; expect( spyNull.calledOnce ).to.be.true; - expect( log.warn.called ).to.be.false; - - log.warn.restore(); } ); - it( 'should return null if element was incorrectly converted and log a warning', () => { - sinon.stub( log, 'warn' ); - + it( 'should throw an error if element was incorrectly converted', () => { dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => { spy(); - expect( conversionApi.convertItem( viewArray, data.cursorPosition ) ).to.equal( null ); + conversionApi.convertItem( viewArray, data.cursorPosition ); } ); - dispatcher.convert( new ViewDocumentFragment() ); + expect( () => { + dispatcher.convert( new ViewDocumentFragment() ); + } ).to.throw( CKEditorError, /^view-conversion-dispatcher-incorrect-result/ ); expect( spy.calledOnce ).to.be.true; expect( spyArray.calledOnce ).to.be.true; - expect( log.warn.calledOnce ).to.be.true; - - log.warn.restore(); } ); } ); describe( 'convertChildren()', () => { it( 'should fire conversion for all children of passed element and return conversion results ' + 'wrapped in document fragment', () => { - silenceWarnings(); - dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => { spy(); const result = conversionApi.convertChildren( data.viewItem, ModelPosition.createAt( rootMock ) ); - expect( result ).to.be.instanceof( ModelRange ); - expect( result.start.path ).to.deep.equal( [ 0 ] ); - expect( result.end.path ).to.deep.equal( [ 7 ] ); - expect( Array.from( result.getItems() ) ).to.length( 2 ); - expect( Array.from( result.getItems() )[ 0 ] ).to.equal( modelP ); - expect( Array.from( result.getItems() )[ 1 ] ).to.instanceof( ModelTextProxy ); - expect( Array.from( result.getItems() )[ 1 ].data ).to.equal( 'foobar' ); + expect( result.modelRange ).to.be.instanceof( ModelRange ); + expect( result.modelRange.start.path ).to.deep.equal( [ 0 ] ); + expect( result.modelRange.end.path ).to.deep.equal( [ 7 ] ); + expect( Array.from( result.modelRange.getItems() ) ).to.length( 2 ); + expect( Array.from( result.modelRange.getItems() )[ 0 ] ).to.equal( modelP ); + expect( Array.from( result.modelRange.getItems() )[ 1 ] ).to.instanceof( ModelTextProxy ); + expect( Array.from( result.modelRange.getItems() )[ 1 ].data ).to.equal( 'foobar' ); + + expect( result.cursorPosition ).instanceof( ModelPosition ); + expect( result.cursorPosition.path ).to.deep.equal( [ 7 ] ); } ); dispatcher.convert( new ViewDocumentFragment( [ viewP, viewText ] ) ); @@ -533,33 +520,6 @@ describe( 'ViewConversionDispatcher', () => { expect( spyP.calledOnce ).to.be.true; expect( spyText.calledOnce ).to.be.true; } ); - - it( 'should filter out incorrectly converted elements and log warnings', () => { - sinon.stub( log, 'warn' ); - - dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => { - spy(); - - const result = conversionApi.convertChildren( data.viewItem, ModelPosition.createAt( rootMock ) ); - - expect( result ).to.be.instanceof( ModelRange ); - expect( result.start.path ).to.deep.equal( [ 0 ] ); - expect( result.end.path ).to.deep.equal( [ 7 ] ); - expect( Array.from( result.getItems() ) ).to.length( 2 ); - expect( Array.from( result.getItems() )[ 0 ] ).to.equal( modelP ); - expect( Array.from( result.getItems() )[ 1 ] ).to.instanceof( ModelTextProxy ); - expect( Array.from( result.getItems() )[ 1 ].data ).to.equal( 'foobar' ); - } ); - - dispatcher.convert( new ViewDocumentFragment( [ viewArray, viewP, viewDiv, viewText, viewNull ] ) ); - - expect( spy.calledOnce ).to.be.true; - expect( spyNull.calledOnce ).to.be.true; - expect( spyArray.calledOnce ).to.be.true; - expect( log.warn.calledOnce ).to.be.true; - - log.warn.restore(); - } ); } ); describe( 'splitToAllowedParent()', () => { @@ -649,10 +609,4 @@ describe( 'ViewConversionDispatcher', () => { } ); } ); } ); - - // Silences warnings that pop up in tests. Use when the test checks a specific functionality and we are not interested in those logs. - // No need to restore `log.warn` - it is done in `afterEach()`. - function silenceWarnings() { - log.warn = () => {}; - } } ); From 2c3e9c8c0db07dc903686440a99c6d62adc4e642 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 30 Jan 2018 23:07:07 +0100 Subject: [PATCH 23/42] Aligned ViewConversionBuilder to latest conversion API. --- src/conversion/buildviewconverter.js | 87 +++++++++++++------------- tests/conversion/buildviewconverter.js | 2 +- 2 files changed, 45 insertions(+), 44 deletions(-) diff --git a/src/conversion/buildviewconverter.js b/src/conversion/buildviewconverter.js index 1884c9597..fa212ea25 100644 --- a/src/conversion/buildviewconverter.js +++ b/src/conversion/buildviewconverter.js @@ -270,12 +270,12 @@ class ViewConverterBuilder { */ toElement( element ) { function eventCallbackGen( from ) { - return ( evt, data, consumable, conversionApi ) => { + return ( evt, data, conversionApi ) => { const writer = conversionApi.writer; // There is one callback for all patterns in the matcher. // This will be usually just one pattern but we support matchers with many patterns too. - const matchAll = from.matcher.matchAll( data.input ); + const matchAll = from.matcher.matchAll( data.viewItem ); // If there is no match, this callback should not do anything. if ( !matchAll ) { @@ -285,7 +285,7 @@ class ViewConverterBuilder { // Now, for every match between matcher and actual element, we will try to consume the match. for ( const match of matchAll ) { // Create model element basing on creator function or element name. - const modelElement = element instanceof Function ? element( data.input, writer ) : writer.createElement( element ); + const modelElement = element instanceof Function ? element( data.viewItem, writer ) : writer.createElement( element ); // Do not convert if element building function returned falsy value. if ( !modelElement ) { @@ -293,14 +293,14 @@ class ViewConverterBuilder { } // When element was already consumed then skip it. - if ( !consumable.test( data.input, from.consume || match.match ) ) { + if ( !conversionApi.consumable.test( data.viewItem, from.consume || match.match ) ) { continue; } // Find allowed parent for element that we are going to insert. // If current parent does not allow to insert element but one of the ancestors does // then split nodes to allowed parent. - const splitResult = conversionApi.splitToAllowedParent( modelElement, data.position, conversionApi ); + const splitResult = conversionApi.splitToAllowedParent( modelElement, data.cursorPosition ); // When there is no split result it means that we can't insert element to model tree, so let's skip it. if ( !splitResult ) { @@ -311,36 +311,35 @@ class ViewConverterBuilder { conversionApi.writer.insert( modelElement, splitResult.position ); // Convert children and insert to element. - const childrenOutput = conversionApi.convertChildren( data.input, consumable, Position.createAt( modelElement ) ); + const childrenResult = conversionApi.convertChildren( data.viewItem, Position.createAt( modelElement ) ); // Consume appropriate value from consumable values list. - consumable.consume( data.input, from.consume || match.match ); - - // Prepare conversion output range, we know that range will start before inserted element. - const output = new Range( Position.createBefore( modelElement ) ); - - // Now we need to check where the output should end. + conversionApi.consumable.consume( data.viewItem, from.consume || match.match ); + + // Set conversion result range. + data.modelRange = new Range( + // Range should start before inserted element + Position.createBefore( modelElement ), + // Should end after but we need to take into consideration that children could split our + // element, so we need to move range after parent of the last converted child. + // before: [] + // after: [] + Position.createAfter( childrenResult.cursorPosition.parent ) + ); - // If we had to split parent to insert our element then we want to continue conversion - // inside split parent. + // Now we need to check where the cursorPosition should be. + // If we had to split parent to insert our element then we want to continue conversion inside split parent. // // before: [] - // after: [] + // after: [] if ( splitResult.cursorParent ) { - output.end = Position.createAt( splitResult.cursorParent ); + data.cursorPosition = Position.createAt( splitResult.cursorParent ); - // When element is converted on target position (without splitting) we need to move range - // after this element but we need to take into consideration that children could split our - // element, so we need to move range after parent of the last converted child. - // - // after: [] - // before: [] + // Otherwise just continue after inserted element. } else { - output.end = Position.createAfter( childrenOutput.end.parent ); + data.cursorPosition = data.modelRange.end; } - data.output = output; - // Prevent multiple conversion if there are other correct matches. break; } @@ -369,10 +368,10 @@ class ViewConverterBuilder { */ toAttribute( keyOrCreator, value ) { function eventCallbackGen( from ) { - return ( evt, data, consumable, conversionApi ) => { + return ( evt, data, conversionApi ) => { // There is one callback for all patterns in the matcher. // This will be usually just one pattern but we support matchers with many patterns too. - const matchAll = from.matcher.matchAll( data.input ); + const matchAll = from.matcher.matchAll( data.viewItem ); // If there is no match, this callback should not do anything. if ( !matchAll ) { @@ -382,21 +381,22 @@ class ViewConverterBuilder { // Now, for every match between matcher and actual element, we will try to consume the match. for ( const match of matchAll ) { // Try to consume appropriate values from consumable values list. - if ( !consumable.consume( data.input, from.consume || match.match ) ) { + if ( !conversionApi.consumable.consume( data.viewItem, from.consume || match.match ) ) { continue; } - // Since we are converting to attribute we need an output on which we will set the attribute. - // If the output is not created yet, we will create it. - if ( !data.output ) { - data.output = conversionApi.convertChildren( data.input, consumable, data.position ); + // Since we are converting to attribute we need an range on which we will set the attribute. + // If the range is not created yet, we will create it. + if ( !data.modelRange ) { + // Convert children and set conversion result as a current data. + data = Object.assign( data, conversionApi.convertChildren( data.viewItem, data.cursorPosition ) ); } // Use attribute creator function, if provided. let attribute; if ( keyOrCreator instanceof Function ) { - attribute = keyOrCreator( data.input ); + attribute = keyOrCreator( data.viewItem ); if ( !attribute ) { return; @@ -404,12 +404,12 @@ class ViewConverterBuilder { } else { attribute = { key: keyOrCreator, - value: value ? value : data.input.getAttribute( from.attributeKey ) + value: value ? value : data.viewItem.getAttribute( from.attributeKey ) }; } // Set attribute on each item in range according to Schema. - for ( const node of Array.from( data.output.getItems() ) ) { + for ( const node of Array.from( data.modelRange.getItems() ) ) { if ( conversionApi.schema.checkAttribute( node, attribute.key ) ) { conversionApi.writer.setAttribute( attribute.key, attribute.value, node ); } @@ -459,12 +459,12 @@ class ViewConverterBuilder { */ toMarker( creator ) { function eventCallbackGen( from ) { - return ( evt, data, consumable, conversionApi ) => { + return ( evt, data, conversionApi ) => { const writer = conversionApi.writer; // There is one callback for all patterns in the matcher. // This will be usually just one pattern but we support matchers with many patterns too. - const matchAll = from.matcher.matchAll( data.input ); + const matchAll = from.matcher.matchAll( data.viewItem ); // If there is no match, this callback should not do anything. if ( !matchAll ) { @@ -475,10 +475,10 @@ class ViewConverterBuilder { // When creator is provided then create model element basing on creator function. if ( creator instanceof Function ) { - modelElement = creator( data.input ); + modelElement = creator( data.viewItem ); // When there is no creator then create model element basing on data from view element. } else { - modelElement = writer.createElement( '$marker', { 'data-name': data.input.getAttribute( 'data-name' ) } ); + modelElement = writer.createElement( '$marker', { 'data-name': data.viewItem.getAttribute( 'data-name' ) } ); } // Check if model element is correct (has proper name and property). @@ -491,18 +491,19 @@ class ViewConverterBuilder { // Now, for every match between matcher and actual element, we will try to consume the match. for ( const match of matchAll ) { // Try to consume appropriate values from consumable values list. - if ( !consumable.consume( data.input, from.consume || match.match ) ) { + if ( !conversionApi.consumable.consume( data.viewItem, from.consume || match.match ) ) { continue; } // Tmp fix because multiple matchers are not properly matched and consumed. // See https://github.com/ckeditor/ckeditor5-engine/issues/1257. - if ( data.output ) { + if ( data.modelRange ) { continue; } - writer.insert( modelElement, data.position ); - data.output = Range.createOn( modelElement ); + writer.insert( modelElement, data.cursorPosition ); + data.modelRange = Range.createOn( modelElement ); + data.cursorPosition = data.modelRange.end; // Prevent multiple conversion if there are other correct matches. break; diff --git a/tests/conversion/buildviewconverter.js b/tests/conversion/buildviewconverter.js index dd2248916..82531bfbb 100644 --- a/tests/conversion/buildviewconverter.js +++ b/tests/conversion/buildviewconverter.js @@ -110,7 +110,7 @@ describe( 'View converter builder', () => { it( 'should convert from view element to model element', () => { buildViewConverter().for( dispatcher ).fromElement( 'p' ).toElement( 'paragraph' ); - const conversionResult = dispatcher.convert( new ViewContainerElement( 'p', null, new ViewText( 'foo' ) ), context ); + const conversionResult = dispatcher.convert( new ViewContainerElement( 'p', null, new ViewText( 'foo' ) ) ); expect( modelToString( conversionResult ) ).to.equal( 'foo' ); } ); From 7346eb79f89247d343b3ee060ae9f9c6a38669fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 30 Jan 2018 23:08:16 +0100 Subject: [PATCH 24/42] Aligned conversion tests to latest conversion API. --- src/conversion/view-to-model-converters.js | 20 ++++++++++-------- tests/conversion/view-to-model-converters.js | 22 +++++++++++--------- 2 files changed, 23 insertions(+), 19 deletions(-) diff --git a/src/conversion/view-to-model-converters.js b/src/conversion/view-to-model-converters.js index 8a002434b..bba9ed875 100644 --- a/src/conversion/view-to-model-converters.js +++ b/src/conversion/view-to-model-converters.js @@ -28,10 +28,11 @@ import Range from '../model/range'; * {@link module:engine/model/documentfragment~DocumentFragment model fragment} with children of converted view item. */ export function convertToModelFragment() { - return ( evt, data, consumable, conversionApi ) => { + return ( evt, data, conversionApi ) => { // Second argument in `consumable.consume` is discarded for ViewDocumentFragment but is needed for ViewElement. - if ( !data.output && consumable.consume( data.input, { name: true } ) ) { - data.output = conversionApi.convertChildren( data.input, consumable, data.position ); + if ( !data.modelRange && conversionApi.consumable.consume( data.viewItem, { name: true } ) ) { + data = Object.assign( data, conversionApi.convertChildren( data.viewItem, data.cursorPosition ) ); + data.cursorPosition = data.modelRange.end; } }; } @@ -42,14 +43,15 @@ export function convertToModelFragment() { * @returns {Function} {@link module:engine/view/text~Text View text} converter. */ export function convertText() { - return ( evt, data, consumable, conversionApi ) => { - if ( conversionApi.schema.checkChild( data.position, '$text' ) ) { - if ( consumable.consume( data.input ) ) { - const text = conversionApi.writer.createText( data.input.data ); + return ( evt, data, conversionApi ) => { + if ( conversionApi.schema.checkChild( data.cursorPosition, '$text' ) ) { + if ( conversionApi.consumable.consume( data.viewItem ) ) { + const text = conversionApi.writer.createText( data.viewItem.data ); - conversionApi.writer.insert( text, data.position ); + conversionApi.writer.insert( text, data.cursorPosition ); - data.output = Range.createFromPositionAndShift( data.position, text.offsetSize ); + data.modelRange = Range.createFromPositionAndShift( data.cursorPosition, text.offsetSize ); + data.cursorPosition = data.modelRange.end; } } }; diff --git a/tests/conversion/view-to-model-converters.js b/tests/conversion/view-to-model-converters.js index 6fd3e9191..a0a960443 100644 --- a/tests/conversion/view-to-model-converters.js +++ b/tests/conversion/view-to-model-converters.js @@ -51,11 +51,12 @@ describe( 'view-to-model-converters', () => { // Default converter for elements. Returns just converted children. Added with lowest priority. dispatcher.on( 'text', convertText(), { priority: 'lowest' } ); // Added with normal priority. Should make the above converter not fire. - dispatcher.on( 'text', ( evt, data, consumable, conversionApi ) => { - if ( consumable.consume( data.input ) ) { - const text = conversionApi.writer.createText( data.input.data.replace( /fuck/gi, '****' ) ); - conversionApi.writer.insert( text, data.position ); - data.output = ModelRange.createFromPositionAndShift( data.position, text.offsetSize ); + dispatcher.on( 'text', ( evt, data, conversionApi ) => { + if ( conversionApi.consumable.consume( data.viewItem ) ) { + const text = conversionApi.writer.createText( data.viewItem.data.replace( /fuck/gi, '****' ) ); + conversionApi.writer.insert( text, data.cursorPosition ); + data.modelRange = ModelRange.createFromPositionAndShift( data.cursorPosition, text.offsetSize ); + data.cursorPosition = data.modelRange.end; } } ); @@ -130,14 +131,15 @@ describe( 'view-to-model-converters', () => { // Default converter for elements. Returns just converted children. Added with lowest priority. dispatcher.on( 'element', convertToModelFragment(), { priority: 'lowest' } ); // Added with normal priority. Should make the above converter not fire. - dispatcher.on( 'element:p', ( evt, data, consumable, conversionApi ) => { - if ( consumable.consume( data.input, { name: true } ) ) { + dispatcher.on( 'element:p', ( evt, data, conversionApi ) => { + if ( conversionApi.consumable.consume( data.viewItem, { name: true } ) ) { const paragraph = conversionApi.writer.createElement( 'paragraph' ); - conversionApi.writer.insert( paragraph, data.position ); - conversionApi.convertChildren( data.input, consumable, ModelPosition.createAt( paragraph ) ); + conversionApi.writer.insert( paragraph, data.cursorPosition ); + conversionApi.convertChildren( data.viewItem, ModelPosition.createAt( paragraph ) ); - data.output = ModelRange.createOn( paragraph ); + data.modelRange = ModelRange.createOn( paragraph ); + data.cursorPosition = data.modelRange.end; } } ); From 1c9c0103e37a1f8791d86a72e5502e6070605368 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 30 Jan 2018 23:08:44 +0100 Subject: [PATCH 25/42] Aligned model dev-utils to latest conversion API. --- src/dev-utils/model.js | 48 +++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/src/dev-utils/model.js b/src/dev-utils/model.js index 9c2a3ff60..8e5eb6f04 100644 --- a/src/dev-utils/model.js +++ b/src/dev-utils/model.js @@ -57,7 +57,7 @@ export function getData( model, options = {} ) { throw new TypeError( 'Model needs to be an instance of module:engine/model/model~Model.' ); } - const withoutSelection = !!options.withoutSelection; + const withoutSelection = options.withoutSelection; const rootName = options.rootName || 'main'; const root = model.document.getRoot( rootName ); @@ -320,44 +320,47 @@ export function parse( data, schema, options = {} ) { // -- Converters view -> model ----------------------------------------------------- function convertToModelFragment() { - return ( evt, data, consumable, conversionApi ) => { - data.output = conversionApi.convertChildren( data.input, consumable, data.position ); + return ( evt, data, conversionApi ) => { + const childrenResult = conversionApi.convertChildren( data.viewItem, data.cursorPosition ); - conversionApi.mapper.bindElements( data.position.parent, data.input ); + conversionApi.mapper.bindElements( data.cursorPosition.parent, data.viewItem ); + + data = Object.assign( data, childrenResult ); evt.stop(); }; } function convertToModelElement() { - return ( evt, data, consumable, conversionApi ) => { - const elementName = data.input.name; + return ( evt, data, conversionApi ) => { + const elementName = data.viewItem.name; - if ( !conversionApi.schema.checkChild( data.position, elementName ) ) { - throw new Error( `Element '${ elementName }' was not allowed in given position.`, { position: data.position } ); + if ( !conversionApi.schema.checkChild( data.cursorPosition, elementName ) ) { + throw new Error( `Element '${ elementName }' was not allowed in given position.`, { position: data.cursorPosition } ); } // View attribute value is a string so we want to typecast it to the original type. // E.g. `bold="true"` - value will be parsed from string `"true"` to boolean `true`. - const attributes = convertAttributes( data.input.getAttributes(), parseAttributeValue ); - const element = conversionApi.writer.createElement( data.input.name, attributes ); + const attributes = convertAttributes( data.viewItem.getAttributes(), parseAttributeValue ); + const element = conversionApi.writer.createElement( data.viewItem.name, attributes ); - conversionApi.writer.insert( element, data.position ); + conversionApi.writer.insert( element, data.cursorPosition ); - conversionApi.mapper.bindElements( element, data.input ); + conversionApi.mapper.bindElements( element, data.viewItem ); - conversionApi.convertChildren( data.input, consumable, ModelPosition.createAt( element ) ); + conversionApi.convertChildren( data.viewItem, ModelPosition.createAt( element ) ); - data.output = ModelRange.createOn( element ); + data.modelRange = ModelRange.createOn( element ); + data.cursorPosition = data.modelRange.end; evt.stop(); }; } function convertToModelText( withAttributes = false ) { - return ( evt, data, consumable, conversionApi ) => { - if ( !conversionApi.schema.checkChild( data.position, '$text' ) ) { - throw new Error( 'Text was not allowed in given position.', { position: data.position } ); + return ( evt, data, conversionApi ) => { + if ( !conversionApi.schema.checkChild( data.cursorPosition, '$text' ) ) { + throw new Error( 'Text was not allowed in given position.', { position: data.cursorPosition } ); } let node; @@ -365,16 +368,17 @@ function convertToModelText( withAttributes = false ) { if ( withAttributes ) { // View attribute value is a string so we want to typecast it to the original type. // E.g. `bold="true"` - value will be parsed from string `"true"` to boolean `true`. - const attributes = convertAttributes( data.input.getAttributes(), parseAttributeValue ); + const attributes = convertAttributes( data.viewItem.getAttributes(), parseAttributeValue ); - node = conversionApi.writer.createText( data.input.getChild( 0 ).data, attributes ); + node = conversionApi.writer.createText( data.viewItem.getChild( 0 ).data, attributes ); } else { - node = conversionApi.writer.createText( data.input.data ); + node = conversionApi.writer.createText( data.viewItem.data ); } - conversionApi.writer.insert( node, data.position ); + conversionApi.writer.insert( node, data.cursorPosition ); - data.output = ModelRange.createFromPositionAndShift( data.position, node.offsetSize ); + data.modelRange = ModelRange.createFromPositionAndShift( data.cursorPosition, node.offsetSize ); + data.cursorPosition = data.modelRange.end; evt.stop(); }; From 1fbee48d9ca942f6457c4e6a9a7f701cc002ca3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 31 Jan 2018 00:11:38 +0100 Subject: [PATCH 26/42] Allowed SchemaContext as a context of SchemaContext. --- src/model/schema.js | 15 +++++++-------- tests/model/schema.js | 8 ++++++++ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/model/schema.js b/src/model/schema.js index a68dc3b96..0eed3bb47 100644 --- a/src/model/schema.js +++ b/src/model/schema.js @@ -193,16 +193,11 @@ export default class Schema { this.decorate( 'checkAttribute' ); this.on( 'checkAttribute', ( evt, args ) => { - if ( !( args[ 0 ] instanceof SchemaContext ) ) { - args[ 0 ] = new SchemaContext( args[ 0 ] ); - } + args[ 0 ] = new SchemaContext( args[ 0 ] ); }, { priority: 'highest' } ); this.on( 'checkChild', ( evt, args ) => { - if ( !( args[ 0 ] instanceof SchemaContext ) ) { - args[ 0 ] = new SchemaContext( args[ 0 ] ); - } - + args[ 0 ] = new SchemaContext( args[ 0 ] ); args[ 1 ] = this.getDefinition( args[ 1 ] ); }, { priority: 'highest' } ); } @@ -1072,9 +1067,13 @@ export class SchemaContext { /** * Creates an instance of the context. * - * @param {module:engine/model/schema~SchemaContextDefinition} context + * @param {module:engine/model/schema~SchemaContextDefinition|module:engine/model/schema~SchemaContext} context */ constructor( context ) { + if ( context instanceof SchemaContext ) { + return context; + } + if ( Array.isArray( context ) ) { if ( context[ 0 ] && typeof context[ 0 ] != 'string' && context[ 0 ].is( 'documentFragment' ) ) { context.shift(); diff --git a/tests/model/schema.js b/tests/model/schema.js index ef07b33d6..e17de2a64 100644 --- a/tests/model/schema.js +++ b/tests/model/schema.js @@ -2455,6 +2455,14 @@ describe( 'SchemaContext', () => { expect( Array.from( ctx.getItem( 2 ).getAttributeKeys() ).sort() ).to.deep.equal( [ 'align' ] ); } ); + it( 'creates context based on a SchemaContext instance', () => { + const previousCtx = new SchemaContext( [ 'a', 'b', 'c' ] ); + + const ctx = new SchemaContext( previousCtx ); + + expect( ctx ).to.equal( previousCtx ); + } ); + it( 'filters out DocumentFragment when it is a first item of context - array', () => { const ctx = new SchemaContext( [ new DocumentFragment(), 'paragraph' ] ); From b5e98b11e58741bd4a4e653a7b62a692820d8227 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 31 Jan 2018 00:14:34 +0100 Subject: [PATCH 27/42] Simplified SchemaContext constructor. --- src/model/schema.js | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/model/schema.js b/src/model/schema.js index 0eed3bb47..fd42775fb 100644 --- a/src/model/schema.js +++ b/src/model/schema.js @@ -1074,19 +1074,14 @@ export class SchemaContext { return context; } - if ( Array.isArray( context ) ) { - if ( context[ 0 ] && typeof context[ 0 ] != 'string' && context[ 0 ].is( 'documentFragment' ) ) { - context.shift(); - } - } - else { + if ( !Array.isArray( context ) ) { // `context` is item or position. // Position#getAncestors() doesn't accept any parameters but it works just fine here. context = context.getAncestors( { includeSelf: true } ); + } - if ( context[ 0 ].is( 'documentFragment' ) ) { - context.shift(); - } + if ( context[ 0 ] && typeof context[ 0 ] != 'string' && context[ 0 ].is( 'documentFragment' ) ) { + context.shift(); } this._items = context.map( mapContextItem ); From df000882d1de423266337d308116505d826c2d53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 31 Jan 2018 00:16:10 +0100 Subject: [PATCH 28/42] Kept SchemaContext immutable. --- src/model/schema.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/model/schema.js b/src/model/schema.js index fd42775fb..8f0a8d710 100644 --- a/src/model/schema.js +++ b/src/model/schema.js @@ -1122,7 +1122,7 @@ export class SchemaContext { * @param {module:engine/model/node~Node|String} item Item to add. */ addItem( item ) { - this._items.push( mapContextItem( item ) ); + this._items = [ ...this._items, mapContextItem( item ) ]; } /** From 46a0840d424546103617aa59ee36da4866da6bd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 31 Jan 2018 00:23:31 +0100 Subject: [PATCH 29/42] Add sample comment to loop collecting split elements. --- src/conversion/viewconversiondispatcher.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/conversion/viewconversiondispatcher.js b/src/conversion/viewconversiondispatcher.js index f605f0ce7..b00204938 100644 --- a/src/conversion/viewconversiondispatcher.js +++ b/src/conversion/viewconversiondispatcher.js @@ -285,6 +285,9 @@ export default class ViewConversionDispatcher { // Remember all elements that are created as a result of split. // This is important because at the end of conversion we want to remove all empty split elements. + // + // Loop through positions between elements in range (except split result position) and collect parents. + // [pos][pos][omit][pos][pos] for ( const position of splitResult.range.getPositions() ) { if ( !position.isEqual( splitResult.position ) ) { this._splitElements.add( position.parent ); From ed0140bc90b6ebdf4df336e9bb0bee1db115ee22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 31 Jan 2018 00:26:06 +0100 Subject: [PATCH 30/42] Renamed #_splitElements to #_removeIfEmpty. --- src/conversion/viewconversiondispatcher.js | 26 ++++++++++---------- tests/conversion/viewconversiondispatcher.js | 20 +++++++-------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/conversion/viewconversiondispatcher.js b/src/conversion/viewconversiondispatcher.js index b00204938..446f3c905 100644 --- a/src/conversion/viewconversiondispatcher.js +++ b/src/conversion/viewconversiondispatcher.js @@ -112,13 +112,13 @@ export default class ViewConversionDispatcher { this._model = model; /** - * Store of split elements that was created as a result of conversion using {@link #_splitToAllowedParent}. - * We need to remember these elements because at the end of conversion we want to remove all empty elements. + * List of elements that will be checked after conversion process and if element in the list will be empty it + * will be removed from conversion result. * * @protected * @type {Set} */ - this._splitElements = new Set(); + this._removeIfEmpty = new Set(); /** * Position inside top element of context tree. @@ -185,8 +185,8 @@ export default class ViewConversionDispatcher { // When there is a conversion result. if ( modelRange ) { - // Remove all empty elements that was created as a result of split. - this._removeEmptySplitElements(); + // Remove all empty elements that was added to #_removeIfEmpty list. + this._removeEmptyElements(); // Move all items that was converted to context tree to document fragment. for ( const item of Array.from( this._contextPosition.parent.getChildren() ) ) { @@ -201,7 +201,7 @@ export default class ViewConversionDispatcher { this._contextPosition = null; // Clear split elements. - this._splitElements.clear(); + this._removeIfEmpty.clear(); // Clear conversion API. this.conversionApi.writer = null; @@ -290,7 +290,7 @@ export default class ViewConversionDispatcher { // [pos][pos][omit][pos][pos] for ( const position of splitResult.range.getPositions() ) { if ( !position.isEqual( splitResult.position ) ) { - this._splitElements.add( position.parent ); + this._removeIfEmpty.add( position.parent ); } } @@ -301,26 +301,26 @@ export default class ViewConversionDispatcher { } /** - * Checks if {@link #_splitElements} contains empty elements and remove them. + * Checks if {@link #_removeIfEmpty} contains empty elements and remove them. * We need to do it smart because there could be elements that are not empty because contains - * other empty split elements and after removing its children they become available to remove. + * other empty elements and after removing its children they become available to remove. * We need to continue iterating over split elements as long as any element will be removed. * * @private */ - _removeEmptySplitElements() { + _removeEmptyElements() { let removed = false; - for ( const element of this._splitElements ) { + for ( const element of this._removeIfEmpty ) { if ( element.isEmpty ) { this.conversionApi.writer.remove( element ); - this._splitElements.delete( element ); + this._removeIfEmpty.delete( element ); removed = true; } } if ( removed ) { - this._removeEmptySplitElements(); + this._removeEmptyElements(); } } diff --git a/tests/conversion/viewconversiondispatcher.js b/tests/conversion/viewconversiondispatcher.js index 62c633675..ea8d53a1c 100644 --- a/tests/conversion/viewconversiondispatcher.js +++ b/tests/conversion/viewconversiondispatcher.js @@ -41,7 +41,7 @@ describe( 'ViewConversionDispatcher', () => { it( 'should have properties', () => { const dispatcher = new ViewConversionDispatcher( model ); - expect( dispatcher._splitElements ).to.instanceof( Set ); + expect( dispatcher._removeIfEmpty ).to.instanceof( Set ); } ); } ); @@ -64,13 +64,13 @@ describe( 'ViewConversionDispatcher', () => { // Conversion process properties should be undefined/empty before conversion. expect( dispatcher.conversionApi.writer ).to.not.ok; expect( dispatcher.conversionApi.data ).to.not.ok; - expect( dispatcher._splitElements.size ).to.equal( 0 ); + expect( dispatcher._removeIfEmpty.size ).to.equal( 0 ); dispatcher.on( 'element', ( evt, data, conversionApi ) => { // Check conversion api params. expect( conversionApi.writer ).to.instanceof( ModelWriter ); expect( conversionApi.data ).to.deep.equal( {} ); - expect( dispatcher._splitElements.size ).to.equal( 0 ); + expect( dispatcher._removeIfEmpty.size ).to.equal( 0 ); // Remember writer to check in next converter that is exactly the same instance (the same undo step). writer = conversionApi.writer; @@ -79,7 +79,7 @@ describe( 'ViewConversionDispatcher', () => { conversionApi.data.foo = 'bar'; // Add empty element and mark as a split result to check in next converter. - dispatcher._splitElements.add( conversionApi.writer.createElement( 'paragraph' ) ); + dispatcher._removeIfEmpty.add( conversionApi.writer.createElement( 'paragraph' ) ); // Convert children - this will call second converter. conversionApi.convertChildren( data.viewItem, data.cursorPosition ); @@ -95,7 +95,7 @@ describe( 'ViewConversionDispatcher', () => { expect( conversionApi.data ).to.deep.equal( { foo: 'bar' } ); // Split element is remembered as well. - expect( dispatcher._splitElements.size ).to.equal( 1 ); + expect( dispatcher._removeIfEmpty.size ).to.equal( 1 ); spy(); } ); @@ -108,7 +108,7 @@ describe( 'ViewConversionDispatcher', () => { // Conversion process properties should be cleared after conversion. expect( dispatcher.conversionApi.writer ).to.not.ok; expect( dispatcher.conversionApi.data ).to.not.ok; - expect( dispatcher._splitElements.size ).to.equal( 0 ); + expect( dispatcher._removeIfEmpty.size ).to.equal( 0 ); } ); it( 'should fire viewCleanup event on converted view part', () => { @@ -274,10 +274,10 @@ describe( 'ViewConversionDispatcher', () => { conversionApi.writer.append( innerSplit, outerSplit ); conversionApi.writer.insert( outerSplit, ModelPosition.createBefore( paragraph ) ); - dispatcher._splitElements.add( emptySplit ); - dispatcher._splitElements.add( notEmptySplit ); - dispatcher._splitElements.add( outerSplit ); - dispatcher._splitElements.add( innerSplit ); + dispatcher._removeIfEmpty.add( emptySplit ); + dispatcher._removeIfEmpty.add( notEmptySplit ); + dispatcher._removeIfEmpty.add( outerSplit ); + dispatcher._removeIfEmpty.add( innerSplit ); data.modelRange = ModelRange.createOn( paragraph ); data.cursorPosition = data.modelRange.end; From 7de6753b950d5c6eb708e9bd361180fa96b5bb98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 31 Jan 2018 00:43:52 +0100 Subject: [PATCH 31/42] Renamed contextToPosition to createContextTree. --- src/conversion/viewconversiondispatcher.js | 31 ++++++++++++---------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/conversion/viewconversiondispatcher.js b/src/conversion/viewconversiondispatcher.js index 446f3c905..379238eab 100644 --- a/src/conversion/viewconversiondispatcher.js +++ b/src/conversion/viewconversiondispatcher.js @@ -115,22 +115,27 @@ export default class ViewConversionDispatcher { * List of elements that will be checked after conversion process and if element in the list will be empty it * will be removed from conversion result. * + * After conversion process list is cleared. + * * @protected * @type {Set} */ this._removeIfEmpty = new Set(); /** - * Position inside top element of context tree. + * Position where conversion result will be inserted. Note that it's not exactly position in one of the + * {@link module:engine/model/document~Document#roots document roots} but it's only a similar position. + * At the beginning of conversion process fragment of model tree is created according to given context and this + * position is created in the top element of created fragment. Then {@link module:engine/view/item~Item View items} + * are converted to this position what makes possible to precisely check converted items by + * {@link module:engine/model/schema~Schema}. * - * At the beginning of each conversion fragment of model tree is created according to given context. - * {@link module:engine/view/item~Item View items} are converted to the top element of created fragment. - * This makes possible to precisely check converted items by {@link module:engine/model/schema~Schema}. + * After conversion process position is cleared. * * @private * @type {module:engine/model/position~Position|null} */ - this._contextPosition = null; + this._modelCursor = null; /** * Interface passed by dispatcher to the events callbacks. @@ -163,9 +168,9 @@ export default class ViewConversionDispatcher { return this._model.change( writer => { this.fire( 'viewCleanup', viewItem ); - // Create context tree and store position in the top element. + // Create context tree and set position in the top element. // Items will be converted according to this position. - this._contextPosition = contextToPosition( context, writer ); + this._modelCursor = createContextTree( context, writer ); // Store writer in conversion as a conversion API // to be sure that conversion process will use the same batch. @@ -178,7 +183,7 @@ export default class ViewConversionDispatcher { this.conversionApi.data = {}; // Do the conversion. - const { modelRange } = this._convertItem( viewItem, this._contextPosition ); + const { modelRange } = this._convertItem( viewItem, this._modelCursor ); // Conversion result is always a document fragment so let's create this fragment. const documentFragment = writer.createDocumentFragment(); @@ -189,7 +194,7 @@ export default class ViewConversionDispatcher { this._removeEmptyElements(); // Move all items that was converted to context tree to document fragment. - for ( const item of Array.from( this._contextPosition.parent.getChildren() ) ) { + for ( const item of Array.from( this._modelCursor.parent.getChildren() ) ) { writer.append( item, documentFragment ); } @@ -198,7 +203,7 @@ export default class ViewConversionDispatcher { } // Clear context position. - this._contextPosition = null; + this._modelCursor = null; // Clear split elements. this._removeIfEmpty.clear(); @@ -268,7 +273,7 @@ export default class ViewConversionDispatcher { */ _splitToAllowedParent( element, cursorPosition ) { // Try to find allowed parent. - const allowedParent = this.conversionApi.schema.findAllowedParent( element, cursorPosition, this._contextPosition.parent ); + const allowedParent = this.conversionApi.schema.findAllowedParent( element, cursorPosition, this._modelCursor.parent ); // When there is no parent that allows to insert element then return `null`. if ( !allowedParent ) { @@ -410,7 +415,7 @@ function extractMarkersFromModelFragment( modelItem, writer ) { } // Creates model fragment according to given context and returns position in top element. -function contextToPosition( contextDefinition, writer ) { +function createContextTree( contextDefinition, writer ) { let position; for ( const item of new SchemaContext( contextDefinition ) ) { @@ -429,8 +434,6 @@ function contextToPosition( contextDefinition, writer ) { position = ModelPosition.createAt( current ); } - position.parent.setAttribute( 'isContextTree', true ); - return position; } From 42b10ff2c1f651afc74646be792639c1fe60cec0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 31 Jan 2018 00:51:42 +0100 Subject: [PATCH 32/42] Removed limitElement from Schema#findAllowedParent(). --- src/model/schema.js | 7 +------ tests/model/schema.js | 16 +--------------- 2 files changed, 2 insertions(+), 21 deletions(-) diff --git a/src/model/schema.js b/src/model/schema.js index 8f0a8d710..6d76b973e 100644 --- a/src/model/schema.js +++ b/src/model/schema.js @@ -676,10 +676,9 @@ export default class Schema { * * @params {module:engine/model/node~Node} node Node for which allowed parent should be found. * @params {module:engine/model/position~Position} position Position from searching will start. - * @params {module:engine/model/element~Element} [limitElement] Custom limit element. * @returns {module:engine/model/element~Element|null} element Allowed parent or null if nothing was found. */ - findAllowedParent( node, position, limitElement ) { + findAllowedParent( node, position ) { let parent = position.parent; while ( parent ) { @@ -691,10 +690,6 @@ export default class Schema { return null; } - if ( parent === limitElement ) { - return null; - } - parent = parent.parent; } diff --git a/tests/model/schema.js b/tests/model/schema.js index e17de2a64..55a3c1dc6 100644 --- a/tests/model/schema.js +++ b/tests/model/schema.js @@ -1137,7 +1137,7 @@ describe( 'Schema', () => { } ); } ); - describe( 'findAllowedParent', () => { + describe( 'findAllowedParent()', () => { beforeEach( () => { schema.register( '$root' ); schema.register( 'blockQuote', { @@ -1188,20 +1188,6 @@ describe( 'Schema', () => { expect( parent ).to.null; } ); - - it( 'should use custom limit element nad return null if is reached', () => { - // $root is allowed ancestor for blockQuote. - const node = new Element( 'blockQuote' ); - - const parent = schema.findAllowedParent( - node, - Position.createAt( r1bQp ), - // However lest stop searching when blockQuote is reached. - r1bQ - ); - - expect( parent ).to.null; - } ); } ); describe( 'removeDisallowedAttributes()', () => { From 6e2ca7bb948b8a2e50439c335cba54651160a9dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 31 Jan 2018 00:52:23 +0100 Subject: [PATCH 33/42] Prevent from splitting context tree in view->model conversion. --- src/conversion/viewconversiondispatcher.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/conversion/viewconversiondispatcher.js b/src/conversion/viewconversiondispatcher.js index 379238eab..6fcb33f82 100644 --- a/src/conversion/viewconversiondispatcher.js +++ b/src/conversion/viewconversiondispatcher.js @@ -280,6 +280,11 @@ export default class ViewConversionDispatcher { return null; } + // When allowed parent is in context tree. + if ( this._modelCursor.parent.getAncestors().includes( allowedParent ) ) { + return null; + } + // When current position parent allows to insert element then return this position. if ( allowedParent === cursorPosition.parent ) { return { position: cursorPosition }; From 89f3a3d060296ba77b5e25b6ed1edd2a81dc60fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 31 Jan 2018 00:54:25 +0100 Subject: [PATCH 34/42] Improved Writer#split() docs. --- src/model/writer.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/model/writer.js b/src/model/writer.js index 80b75a994..8283d7092 100644 --- a/src/model/writer.js +++ b/src/model/writer.js @@ -581,7 +581,10 @@ export default class Writer { * * @param {module:engine/model/position~Position} position Position of split. * @param {module:engine/model/node~Node} [limitElement] Stop splitting when this element will be reached. - * @returns {~SplitResult} + * @returns {Object} result Split result. + * @returns {module:engine/model/position~Position} result.position between split elements. + * @returns {module:engine/model/range~Range} result.range Range that stars from the end of the first split element and ands + * at the beginning of the first copy element. */ split( position, limitElement ) { this._assertWriterUsedCorrectly(); @@ -1174,10 +1177,3 @@ function isSameTree( rootA, rootB ) { return false; } - -/** - * @typedef {Object} SplitResult - * @property {module:engine/model/position~Position} position between split elements. - * @property {module:engine/model/range~Range} range Range that stars from the end of the first split element and ands - * at the beginning of the first copy element. - */ From 1a88d11b17f157304f6d46cfd6a779084d2e04ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 31 Jan 2018 00:59:28 +0100 Subject: [PATCH 35/42] Renamed conversionApi.data to conversionApi.store. --- src/conversion/viewconversiondispatcher.js | 6 +++--- tests/conversion/viewconversiondispatcher.js | 11 +++++------ 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/conversion/viewconversiondispatcher.js b/src/conversion/viewconversiondispatcher.js index 6fcb33f82..d65e86dcb 100644 --- a/src/conversion/viewconversiondispatcher.js +++ b/src/conversion/viewconversiondispatcher.js @@ -180,7 +180,7 @@ export default class ViewConversionDispatcher { this.conversionApi.consumable = ViewConsumable.createFrom( viewItem ); // Custom data stored by converter for conversion process. - this.conversionApi.data = {}; + this.conversionApi.store = {}; // Do the conversion. const { modelRange } = this._convertItem( viewItem, this._modelCursor ); @@ -210,7 +210,7 @@ export default class ViewConversionDispatcher { // Clear conversion API. this.conversionApi.writer = null; - this.conversionApi.data = null; + this.conversionApi.store = null; // Return fragment as conversion result. return documentFragment; @@ -510,5 +510,5 @@ function createContextTree( contextDefinition, writer ) { /** * Custom data stored by converter for conversion process. * - * @param {Object} #data + * @param {Object} #store */ diff --git a/tests/conversion/viewconversiondispatcher.js b/tests/conversion/viewconversiondispatcher.js index ea8d53a1c..a4fa1f992 100644 --- a/tests/conversion/viewconversiondispatcher.js +++ b/tests/conversion/viewconversiondispatcher.js @@ -63,20 +63,20 @@ describe( 'ViewConversionDispatcher', () => { // Conversion process properties should be undefined/empty before conversion. expect( dispatcher.conversionApi.writer ).to.not.ok; - expect( dispatcher.conversionApi.data ).to.not.ok; + expect( dispatcher.conversionApi.store ).to.not.ok; expect( dispatcher._removeIfEmpty.size ).to.equal( 0 ); dispatcher.on( 'element', ( evt, data, conversionApi ) => { // Check conversion api params. expect( conversionApi.writer ).to.instanceof( ModelWriter ); - expect( conversionApi.data ).to.deep.equal( {} ); + expect( conversionApi.store ).to.deep.equal( {} ); expect( dispatcher._removeIfEmpty.size ).to.equal( 0 ); // Remember writer to check in next converter that is exactly the same instance (the same undo step). writer = conversionApi.writer; // Add some data to conversion storage to verify them in next converter. - conversionApi.data.foo = 'bar'; + conversionApi.store.foo = 'bar'; // Add empty element and mark as a split result to check in next converter. dispatcher._removeIfEmpty.add( conversionApi.writer.createElement( 'paragraph' ) ); @@ -92,7 +92,7 @@ describe( 'ViewConversionDispatcher', () => { expect( conversionApi.writer ).to.equal( writer ); // Data set by previous converter are remembered. - expect( conversionApi.data ).to.deep.equal( { foo: 'bar' } ); + expect( conversionApi.store ).to.deep.equal( { foo: 'bar' } ); // Split element is remembered as well. expect( dispatcher._removeIfEmpty.size ).to.equal( 1 ); @@ -107,7 +107,7 @@ describe( 'ViewConversionDispatcher', () => { // Conversion process properties should be cleared after conversion. expect( dispatcher.conversionApi.writer ).to.not.ok; - expect( dispatcher.conversionApi.data ).to.not.ok; + expect( dispatcher.conversionApi.store ).to.not.ok; expect( dispatcher._removeIfEmpty.size ).to.equal( 0 ); } ); @@ -237,7 +237,6 @@ describe( 'ViewConversionDispatcher', () => { data.modelRange = ModelRange.createOn( text ); } ); - // Use `additionalData` parameter to check if it was passed to the event. const conversionResult = dispatcher.convert( viewFragment ); // Check conversion result. From 430a6e2f31cef5eef7b43d454528dc4fc7de33a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 31 Jan 2018 11:58:22 +0100 Subject: [PATCH 36/42] Increased CC of ViewConversionDispatcher. --- src/conversion/viewconversiondispatcher.js | 12 ++++++------ tests/conversion/viewconversiondispatcher.js | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/conversion/viewconversiondispatcher.js b/src/conversion/viewconversiondispatcher.js index d65e86dcb..098a447fd 100644 --- a/src/conversion/viewconversiondispatcher.js +++ b/src/conversion/viewconversiondispatcher.js @@ -273,23 +273,23 @@ export default class ViewConversionDispatcher { */ _splitToAllowedParent( element, cursorPosition ) { // Try to find allowed parent. - const allowedParent = this.conversionApi.schema.findAllowedParent( element, cursorPosition, this._modelCursor.parent ); + const allowedParent = this.conversionApi.schema.findAllowedParent( element, cursorPosition ); // When there is no parent that allows to insert element then return `null`. if ( !allowedParent ) { return null; } - // When allowed parent is in context tree. - if ( this._modelCursor.parent.getAncestors().includes( allowedParent ) ) { - return null; - } - // When current position parent allows to insert element then return this position. if ( allowedParent === cursorPosition.parent ) { return { position: cursorPosition }; } + // When allowed parent is in context tree. + if ( this._modelCursor.parent.getAncestors().includes( allowedParent ) ) { + return null; + } + // Split element to allowed parent. const splitResult = this.conversionApi.writer.split( cursorPosition, allowedParent ); diff --git a/tests/conversion/viewconversiondispatcher.js b/tests/conversion/viewconversiondispatcher.js index a4fa1f992..1cc5ef661 100644 --- a/tests/conversion/viewconversiondispatcher.js +++ b/tests/conversion/viewconversiondispatcher.js @@ -606,6 +606,25 @@ describe( 'ViewConversionDispatcher', () => { dispatcher.convert( new ViewDocumentFragment() ); sinon.assert.calledOnce( spy ); } ); + + it( 'should return null if element is not allowed in position and any of ancestors but is allowed in context tree', () => { + const spy = sinon.spy(); + + model.schema.register( 'div', { + allowIn: '$root', + } ); + + dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => { + const code = conversionApi.writer.createElement( 'div' ); + const result = conversionApi.splitToAllowedParent( code, data.cursorPosition ); + + expect( result ).to.null; + spy(); + } ); + + dispatcher.convert( new ViewDocumentFragment(), [ '$root', 'paragraph' ] ); + sinon.assert.calledOnce( spy ); + } ); } ); } ); } ); From 677d1a73eeddeef4ea61ff55bb4920bf048aa21a Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 31 Jan 2018 12:54:11 +0100 Subject: [PATCH 37/42] Tests: Added additional test for model.Differ. --- tests/model/differ.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/model/differ.js b/tests/model/differ.js index b0b910364..870a29adc 100644 --- a/tests/model/differ.js +++ b/tests/model/differ.js @@ -533,6 +533,18 @@ describe( 'Differ', () => { ] ); } ); + it( 'on an element - only one of many attributes changes', () => { + root.getChild( 0 ).setAttribute( 'otherAttr', true ); + + const range = Range.createFromParentsAndOffsets( root, 0, root.getChild( 0 ), 0 ); + + attribute( range, attributeKey, attributeOldValue, attributeNewValue ); + + expectChanges( [ + { type: 'attribute', range, attributeKey, attributeOldValue, attributeNewValue } + ] ); + } ); + it( 'on a character', () => { const parent = root.getChild( 1 ); const range = Range.createFromParentsAndOffsets( parent, 1, parent, 2 ); From 6b155682bb6ede0dca3a67d74d5aba748c0ec73d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 31 Jan 2018 13:06:49 +0100 Subject: [PATCH 38/42] Improved ViewConversionDispatcher docs. --- src/conversion/viewconversiondispatcher.js | 52 +++++++++++----------- 1 file changed, 25 insertions(+), 27 deletions(-) diff --git a/src/conversion/viewconversiondispatcher.js b/src/conversion/viewconversiondispatcher.js index 098a447fd..1ff70bc72 100644 --- a/src/conversion/viewconversiondispatcher.js +++ b/src/conversion/viewconversiondispatcher.js @@ -40,40 +40,40 @@ import mix from '@ckeditor/ckeditor5-utils/src/mix'; * * Examples of providing callbacks for `ViewConversionDispatcher`: * - * TODO - update samples. - * * // Converter for paragraphs (

). - * viewDispatcher.on( 'element:p', ( evt, data, consumable, conversionApi ) => { - * const paragraph = new ModelElement( 'paragraph' ); + * viewDispatcher.on( 'element:p', ( evt, data, conversionApi ) => { + * // Create paragraph element. + * const paragraph = conversionApi.createElement( 'paragraph' ); + * + * // Check if paragraph is allowed on current cursor position. + * if ( conversionApi.schema.checkChild( data.cursorPosition, paragraph ) ) { + * // Try to consume value from consumable list. + * if ( !conversionApi.consumable.consume( data.viewItem, { name: true } ) ) { + * // Insert paragraph on cursor position. + * conversionApi.writer.insert( paragraph, data.cursorPosition ); + * + * // Convert paragraph children to paragraph element. + * conversionApi.convertChildren( data.viewItem, ModelPosition.createAt( paragraph ) ); * - * if ( conversionApi.schema.checkChild( data.context, paragraph ) ) { - * if ( !consumable.consume( data.input, { name: true } ) ) { - * // Before converting this paragraph's children we have to update their context by this paragraph. - * data.context.push( paragraph ); - * const children = conversionApi.convertChildren( data.input, consumable, data ); - * data.context.pop(); - * paragraph.appendChildren( children ); - * data.output = paragraph; + * // Wrap paragraph in range and set as conversion result. + * data.modelRange = ModelRange.createOn( paragraph ); + * + * // Continue conversion just after paragraph. + * data.cursorPosition = data.modelRange.end; * } * } * } ); * * // Converter for links (). - * viewDispatcher.on( 'element:a', ( evt, data, consumable, conversionApi ) => { - * if ( consumable.consume( data.input, { name: true, attributes: [ 'href' ] } ) ) { + * viewDispatcher.on( 'element:a', ( evt, data, conversionApi ) => { + * if ( conversionApi.consumable.consume( data.viewItem, { name: true, attributes: [ 'href' ] } ) ) { * // element is inline and is represented by an attribute in the model. - * // This is why we are not updating `context` property. - * data.output = conversionApi.convertChildren( data.input, consumable, data ); - * - * for ( let item of Range.createFrom( data.output ) ) { - * const schemaQuery = { - * name: item.name || '$text', - * attribute: 'link', - * inside: data.context - * }; + * // This is why we need to convert only children. + * const { modelRange } = conversionApi.convertChildren( data.viewItem, data.cursorPosition ); * - * if ( conversionApi.schema.checkAttribute( [ ...data.context, '$text' ], 'link' ) ) { - * item.setAttribute( 'link', data.input.getAttribute( 'href' ) ); + * for ( let item of modelRange.getItems() ) { + * if ( conversionApi.schema.checkAttribute( item, 'linkHref' ) ) { + * conversionApi.writer.setAttribute( 'linkHref', data.viewItem.getAttribute( 'href' ), item ); * } * } * } @@ -447,8 +447,6 @@ function createContextTree( contextDefinition, writer ) { * and is passed as one of parameters when {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher dispatcher} * fires it's events. * - * TODO better explanation. - * * @interface ViewConversionApi */ From f36e5087f7bf934da83e47205f08aec506957d21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 31 Jan 2018 13:17:53 +0100 Subject: [PATCH 39/42] Fixed invalid docs. --- src/conversion/viewconversiondispatcher.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/conversion/viewconversiondispatcher.js b/src/conversion/viewconversiondispatcher.js index 1ff70bc72..db5bd2a59 100644 --- a/src/conversion/viewconversiondispatcher.js +++ b/src/conversion/viewconversiondispatcher.js @@ -271,16 +271,16 @@ export default class ViewConversionDispatcher { * @private * @see module:engine/conversion/viewconversiondispatcher~ViewConversionApi#splitToAllowedParent */ - _splitToAllowedParent( element, cursorPosition ) { + _splitToAllowedParent( node, cursorPosition ) { // Try to find allowed parent. - const allowedParent = this.conversionApi.schema.findAllowedParent( element, cursorPosition ); + const allowedParent = this.conversionApi.schema.findAllowedParent( node, cursorPosition ); - // When there is no parent that allows to insert element then return `null`. + // When there is no parent that allows to insert node then return `null`. if ( !allowedParent ) { return null; } - // When current position parent allows to insert element then return this position. + // When current position parent allows to insert node then return this position. if ( allowedParent === cursorPosition.parent ) { return { position: cursorPosition }; } @@ -490,7 +490,7 @@ function createContextTree( contextDefinition, writer ) { * * @method #splitToAllowedParent * @param {module:engine/model/position~Position} position Position on which element is going to be inserted. - * @param {module:engine/model/element~Node} element Element to insert. + * @param {module:engine/model/node~Node} node Node to insert. * @returns {Object} Split result. * @returns {module:engine/model/position~Position} position between split elements. * @returns {module:engine/model/element~Element} [cursorParent] Element inside which cursor should be placed to From 3b55b0eb876a549aed7fb276937cf75cb2c9556d Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 31 Jan 2018 13:39:56 +0100 Subject: [PATCH 40/42] Fixed: `model.Differ` returned wrong values on `getChanges()` if an element had an attribute that didn't change. --- src/model/differ.js | 6 +++--- tests/model/differ.js | 9 +++++++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/model/differ.js b/src/model/differ.js index 921c6f8e2..c5102e5f4 100644 --- a/src/model/differ.js +++ b/src/model/differ.js @@ -786,10 +786,10 @@ export default class Differ { attributeNewValue: newValue, changeCount: this._changeCount++ } ); - - // Prevent returning two diff items for the same change. - newAttributes.delete( key ); } + + // Prevent returning two diff items for the same change. + newAttributes.delete( key ); } // Look through new attributes that weren't handled above. diff --git a/tests/model/differ.js b/tests/model/differ.js index 870a29adc..f3ba17129 100644 --- a/tests/model/differ.js +++ b/tests/model/differ.js @@ -534,10 +534,15 @@ describe( 'Differ', () => { } ); it( 'on an element - only one of many attributes changes', () => { - root.getChild( 0 ).setAttribute( 'otherAttr', true ); - const range = Range.createFromParentsAndOffsets( root, 0, root.getChild( 0 ), 0 ); + // Set an attribute on an element. It won't change afterwards. + attribute( range, 'otherAttr', null, true ); + + // "Flush" differ. + differ.getChanges(); + differ.reset(); + attribute( range, attributeKey, attributeOldValue, attributeNewValue ); expectChanges( [ From bf921edefa2ab9dd668dcec9288da801775bc828 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 31 Jan 2018 15:53:30 +0100 Subject: [PATCH 41/42] Changed SchemaContext#addItem to SchemaContext#concat. --- src/model/schema.js | 18 +++++++++++---- tests/model/schema.js | 54 +++++++++++++++++++++++++++++++++---------- 2 files changed, 56 insertions(+), 16 deletions(-) diff --git a/src/model/schema.js b/src/model/schema.js index 4076b4f92..be8131ea1 100644 --- a/src/model/schema.js +++ b/src/model/schema.js @@ -1164,12 +1164,22 @@ export class SchemaContext { } /** - * Adds new item at the to of the context. + * Returns new SchemaContext instance with additional items created from provided definition. * - * @param {module:engine/model/node~Node|String} item Item to add. + * @param {String|module:engine/model/node~Node|~SchemaContextDefinition|Array} definition + * Definition of item(s) that will be added to current context. + * @returns {~SchemaContext} New SchemaContext instance. */ - addItem( item ) { - this._items = [ ...this._items, mapContextItem( item ) ]; + concat( definition ) { + if ( !( definition instanceof SchemaContext ) && !Array.isArray( definition ) ) { + definition = [ definition ]; + } + + const ctx = new SchemaContext( definition ); + + ctx._items = [ ...this._items, ...ctx._items ]; + + return ctx; } /** diff --git a/tests/model/schema.js b/tests/model/schema.js index cf9d3e35f..bacbb0ea0 100644 --- a/tests/model/schema.js +++ b/tests/model/schema.js @@ -2755,33 +2755,63 @@ describe( 'SchemaContext', () => { } ); } ); - describe( 'addItem()', () => { - it( 'adds new item at the top of the context #text', () => { - const node = new Text( 'd' ); + describe( 'concat()', () => { + it( 'creates new SchemaContext instance with new item - #string', () => { + const ctx = new SchemaContext( [ 'a', 'b', 'c' ] ); + + const newCtx = ctx.concat( 'd' ); + + expect( newCtx ).to.instanceof( SchemaContext ); + expect( newCtx ).to.not.equal( ctx ); + expect( Array.from( newCtx.getNames() ) ).to.deep.equal( [ 'a', 'b', 'c', 'd' ] ); + expect( Array.from( ctx.getNames() ) ).to.deep.equal( [ 'a', 'b', 'c' ] ); + } ); + it( 'creates new SchemaContext instance with new item - #text', () => { + const node = new Text( 'd' ); const ctx = new SchemaContext( [ 'a', 'b', 'c' ] ); - ctx.addItem( node ); + const newCtx = ctx.concat( node ); - expect( Array.from( ctx ).map( item => item.name ) ).to.deep.equal( [ 'a', 'b', 'c', '$text' ] ); + expect( newCtx ).to.instanceof( SchemaContext ); + expect( newCtx ).to.not.equal( ctx ); + expect( Array.from( newCtx.getNames() ) ).to.deep.equal( [ 'a', 'b', 'c', '$text' ] ); + expect( Array.from( ctx.getNames() ) ).to.deep.equal( [ 'a', 'b', 'c' ] ); } ); - it( 'adds new item at the top of the context #string', () => { + it( 'creates new SchemaContext instance with new item - #node', () => { const ctx = new SchemaContext( [ 'a', 'b', 'c' ] ); + const parent = new Element( 'parent', null, new Element( 'd' ) ); - ctx.addItem( 'd' ); + const newCtx = ctx.concat( parent.getChild( 0 ) ); - expect( Array.from( ctx ).map( item => item.name ) ).to.deep.equal( [ 'a', 'b', 'c', 'd' ] ); + expect( newCtx ).to.instanceof( SchemaContext ); + expect( newCtx ).to.not.equal( ctx ); + expect( Array.from( newCtx.getNames() ) ).to.deep.equal( [ 'a', 'b', 'c', 'd' ] ); + expect( Array.from( ctx.getNames() ) ).to.deep.equal( [ 'a', 'b', 'c' ] ); } ); - it( 'adds new item at the top of the context #node', () => { - const node = new Element( 'd' ); + it( 'creates new SchemaContext instance with new item - #SchemaContext', () => { + const ctx = new SchemaContext( [ 'a', 'b', 'c' ] ); + const schemaContext = new SchemaContext( [ 'd', 'e' ] ); + + const newCtx = ctx.concat( schemaContext ); + expect( newCtx ).to.instanceof( SchemaContext ); + expect( newCtx ).to.not.equal( ctx ); + expect( Array.from( newCtx.getNames() ) ).to.deep.equal( [ 'a', 'b', 'c', 'd', 'e' ] ); + expect( Array.from( ctx.getNames() ) ).to.deep.equal( [ 'a', 'b', 'c' ] ); + } ); + + it( 'creates new SchemaContext instance with new item - #array', () => { const ctx = new SchemaContext( [ 'a', 'b', 'c' ] ); - ctx.addItem( node ); + const newCtx = ctx.concat( [ 'd', new Text( 'e' ), new Element( 'f' ) ] ); - expect( Array.from( ctx ).map( item => item.name ) ).to.deep.equal( [ 'a', 'b', 'c', 'd' ] ); + expect( newCtx ).to.instanceof( SchemaContext ); + expect( newCtx ).to.not.equal( ctx ); + expect( Array.from( newCtx.getNames() ) ).to.deep.equal( [ 'a', 'b', 'c', 'd', '$text', 'f' ] ); + expect( Array.from( ctx.getNames() ) ).to.deep.equal( [ 'a', 'b', 'c' ] ); } ); } ); From fcd6fd47de1f37f84d0e1c4e5b6ca8827a5bdc55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 31 Jan 2018 15:57:42 +0100 Subject: [PATCH 42/42] Fixed invalid docs. --- src/model/schema.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/model/schema.js b/src/model/schema.js index be8131ea1..667d14b29 100644 --- a/src/model/schema.js +++ b/src/model/schema.js @@ -1166,9 +1166,9 @@ export class SchemaContext { /** * Returns new SchemaContext instance with additional items created from provided definition. * - * @param {String|module:engine/model/node~Node|~SchemaContextDefinition|Array} definition - * Definition of item(s) that will be added to current context. - * @returns {~SchemaContext} New SchemaContext instance. + * @param {String|module:engine/model/node~Node|module:engine/model/schema~SchemaContext| + * Array} definition Definition of item(s) that will be added to current context. + * @returns {module:engine/model/schema~SchemaContext} New SchemaContext instance. */ concat( definition ) { if ( !( definition instanceof SchemaContext ) && !Array.isArray( definition ) ) {