Explorar el Código

Merge pull request #766 from ckeditor/t/743

Fixed cloning for (Root)EditableElement and EmptyElement.
Piotrek Koszuliński hace 9 años
padre
commit
c0f1c252b5

+ 2 - 1
packages/ckeditor5-engine/src/view/document.js

@@ -193,7 +193,8 @@ export default class Document {
 	createRoot( domRoot, name = 'main' ) {
 		const rootTag = typeof domRoot == 'string' ? domRoot : domRoot.tagName;
 
-		const viewRoot = new RootEditableElement( this, rootTag.toLowerCase(), name );
+		const viewRoot = new RootEditableElement( rootTag.toLowerCase(), name );
+		viewRoot.document = this;
 
 		this.roots.set( name, viewRoot );
 

+ 32 - 19
packages/ckeditor5-engine/src/view/editableelement.js

@@ -8,10 +8,12 @@
  */
 
 import ContainerElement from './containerelement';
-
+import CKEditorError from 'ckeditor5-utils/src/ckeditorerror';
 import mix from 'ckeditor5-utils/src/mix';
 import ObservableMixin from 'ckeditor5-utils/src/observablemixin';
 
+const documentSymbol = Symbol( 'document' );
+
 /**
  * Editable element which can be a {@link module:engine/view/rooteditableelement~RootEditableElement root}
  * or nested editable area in the editor.
@@ -23,17 +25,9 @@ export default class EditableElement extends ContainerElement {
 	/**
 	 * Creates an editable element.
 	 */
-	constructor( document, name, attrs, children ) {
+	constructor( name, attrs, children ) {
 		super( name, attrs, children );
 
-		/**
-		 * {@link module:engine/view/document~Document} that is an owner of this root.
-		 *
-		 * @private
-		 * @member {module:engine/view/document~Document} module:engine/view/rooteditableelement~RootEditableElement#_document
-		 */
-		this._document = document;
-
 		/**
 		 * Whether the editable is in read-write or read-only mode.
 		 *
@@ -52,6 +46,34 @@ export default class EditableElement extends ContainerElement {
 		 * @observable
 		 * @member {Boolean} module:engine/view/editableelement~EditableElement#isFocused
 		 */
+		this.set( 'isFocused', false );
+
+		/**
+		 * The {@link module:engine/view/document~Document} which is an owner of this root.
+		 * Can only by set once.
+		 * Throws {@link module:utils/ckeditorerror~CKEditorError CKEditorError} `view-editableelement-document-already-set`
+		 * when document is already set.
+		 *
+		 * @member {module:engine/view/document~Document} #document
+		 */
+	}
+
+	get document() {
+		return this.getCustomProperty( documentSymbol );
+	}
+
+	set document( document ) {
+		if ( this.getCustomProperty( documentSymbol ) ) {
+			/**
+			 * View document is already set. It can only be set once.
+			 *
+			 * @error view-editableelement-document-already-set
+			 */
+			throw new CKEditorError( 'view-editableelement-document-already-set: View document is already set.' );
+		}
+
+		this.setCustomProperty( documentSymbol, document );
+
 		this.bind( 'isFocused' ).to(
 			document,
 			'isFocused',
@@ -64,15 +86,6 @@ export default class EditableElement extends ContainerElement {
 			this.isFocused = document.isFocused && document.selection.editableElement == this;
 		}, { priority: 'low' } );
 	}
-
-	/**
-	 * {@link module:engine/view/document~Document View document} reference that owns this editable element.
-	 *
-	 * @type {module:engine/view/document~Document|null}
-	 */
-	get document() {
-		return this._document;
-	}
 }
 
 mix( EditableElement, ObservableMixin );

+ 12 - 43
packages/ckeditor5-engine/src/view/emptyelement.js

@@ -9,6 +9,7 @@
 
 import Element from './element';
 import CKEditorError from 'ckeditor5-utils/src/ckeditorerror';
+import Node from './node';
 
 /**
  * EmptyElement class. It is used to represent elements that cannot contain any child nodes.
@@ -23,12 +24,8 @@ export default class EmptyElement extends Element {
 	 * @param {String} name Node name.
 	 * @param {Object|Iterable} [attributes] Collection of attributes.
 	 */
-	constructor( name, attributes ) {
-		super( name, attributes );
-
-		if ( arguments.length > 2 ) {
-			throwCannotAdd();
-		}
+	constructor( name, attributes, children ) {
+		super( name, attributes, children );
 
 		/**
 		 * Returns `null` because filler is not needed for EmptyElements.
@@ -39,51 +36,23 @@ export default class EmptyElement extends Element {
 		this.getFillerOffset = getFillerOffset;
 	}
 
-	/**
-	 * Clones provided element. Overrides {@link module:engine/view/element~Element#clone} method, as it's forbidden to pass child
-	 * nodes to EmptyElement's constructor.
-	 *
-	 * @returns {module:engine/view/emptyelement~EmptyElement} Clone of this element.
-	 */
-	clone() {
-		const cloned = new this.constructor( this.name, this._attrs );
-
-		// Classes and styles are cloned separately - this solution is faster than adding them back to attributes and
-		// parse once again in constructor.
-		cloned._classes = new Set( this._classes );
-		cloned._styles = new Map( this._styles );
-
-		return cloned;
-	}
-
-	/**
-	 * Overrides {@link module:engine/view/element~Element#appendChildren} method.
-	 * Throws {@link module:utils/ckeditorerror~CKEditorError CKEditorError} `view-emptyelement-cannot-add` to prevent adding any child nodes
-	 * to EmptyElement.
-	 */
-	appendChildren() {
-		throwCannotAdd();
-	}
-
 	/**
 	 * Overrides {@link module:engine/view/element~Element#insertChildren} method.
 	 * Throws {@link module:utils/ckeditorerror~CKEditorError CKEditorError} `view-emptyelement-cannot-add` to prevent adding any child nodes
 	 * to EmptyElement.
 	 */
-	insertChildren() {
-		throwCannotAdd();
+	insertChildren( index, nodes ) {
+		if ( nodes && ( nodes instanceof Node || Array.from( nodes ).length > 0 ) ) {
+			/**
+			 * Cannot add children to {@link module:engine/view/emptyelement~EmptyElement}.
+			 *
+			 * @error view-emptyelement-cannot-add
+			 */
+			throw new CKEditorError( 'view-emptyelement-cannot-add: Cannot add child nodes to EmptyElement instance.' );
+		}
 	}
 }
 
-function throwCannotAdd() {
-	/**
-	 * Cannot add children to {@link module:engine/view/emptyelement~EmptyElement}.
-	 *
-	 * @error view-emptyelement-cannot-add
-	 */
-	throw new CKEditorError( 'view-emptyelement-cannot-add: Cannot add child nodes to EmptyElement instance.' );
-}
-
 // Returns `null` because block filler is not needed for EmptyElements.
 //
 // @returns {null}

+ 15 - 6
packages/ckeditor5-engine/src/view/rooteditableelement.js

@@ -9,6 +9,8 @@
 
 import EditableElement from './editableelement';
 
+const rootNameSymbol = Symbol( 'rootName' );
+
 /**
  * Class representing a single root in the data view. A root can be either {@link #isReadOnly editable or read-only}, but
  * in both cases it is called "an editable". Roots can contain other {@link module:engine/view/editableelement~EditableElement editable
@@ -23,17 +25,24 @@ export default class RootEditableElement extends EditableElement {
 	 *
 	 * @param {module:engine/view/document~Document} document {@link module:engine/view/document~Document} that is an owner of the root.
 	 * @param {String} name Node name.
-	 * @param {String} [rootName='main'] Root name inside parent {@link module:engine/view/document~Document}.
 	 */
-	constructor( document, name, rootName = 'main' ) {
-		super( document, name );
+	constructor( name ) {
+		super( name );
 
 		/**
-		 * Name of this root inside {@link module:engine/view/document~Document} that is an owner of this root.
+		 * Name of this root inside {@link module:engine/view/document~Document} that is an owner of this root. If no
+		 * other name is set, `main` name is used.
 		 *
-		 * @readonly
 		 * @member {String}
 		 */
-		this.rootName = rootName;
+		this.rootName = 'main';
+	}
+
+	get rootName() {
+		return this.getCustomProperty( rootNameSymbol );
+	}
+
+	set rootName( rootName ) {
+		this.setCustomProperty( rootNameSymbol, rootName );
 	}
 }

+ 3 - 1
packages/ckeditor5-engine/tests/view/domconverter/domconverter.js

@@ -36,7 +36,9 @@ describe( 'DomConverter', () => {
 
 		beforeEach( () => {
 			viewDocument = new ViewDocument();
-			viewEditable = new ViewEditable( viewDocument, 'div' );
+			viewEditable = new ViewEditable( 'div' );
+			viewEditable.document = viewDocument;
+
 			domEditable = document.createElement( 'div' );
 			converter.bindElements( domEditable, viewEditable );
 			domEditable.setAttribute( 'contenteditable', 'true' );

+ 48 - 5
packages/ckeditor5-engine/tests/view/editableelement.js

@@ -4,22 +4,63 @@
  */
 
 import createDocumentMock from 'ckeditor5-engine/tests/view/_utils/createdocumentmock';
+import CKEditorError from 'ckeditor5-utils/src/ckeditorerror';
 import RootEditableElement from 'ckeditor5-engine/src/view/rooteditableelement';
 import Range from 'ckeditor5-engine/src/view/range';
 
 describe( 'EditableElement', () => {
+	describe( 'document', () => {
+		let element, docMock;
+
+		beforeEach( () => {
+			element = new RootEditableElement( 'div' );
+			docMock = createDocumentMock();
+		} );
+
+		it( 'should allow to set document', () => {
+			element.document = docMock;
+
+			expect( element.document ).to.equal( docMock );
+		} );
+
+		it( 'should return undefined if document is not set', () => {
+			expect( element.document ).to.be.undefined;
+		} );
+
+		it( 'should throw if trying to set document again', () => {
+			element.document = docMock;
+			const newDoc = createDocumentMock();
+
+			expect( () => {
+				element.document = newDoc;
+			} ).to.throw( CKEditorError, 'view-editableelement-document-already-set: View document is already set.' );
+		} );
+
+		it( 'should be cloned properly', () => {
+			element.document = docMock;
+			const newElement = element.clone();
+
+			expect( newElement.document ).to.equal( docMock );
+		} );
+	} );
+
 	describe( 'isFocused', () => {
 		let docMock, viewMain, viewHeader;
 
 		beforeEach( () => {
 			docMock = createDocumentMock();
 
-			viewMain = new RootEditableElement( docMock, 'div' );
-			viewHeader = new RootEditableElement( docMock, 'h1', 'header' );
+			viewMain = new RootEditableElement( 'div' );
+			viewMain.document = docMock;
+
+			viewHeader = new RootEditableElement( 'h1' );
+			viewHeader.document = docMock;
+			viewHeader.rootName = 'header';
 		} );
 
 		it( 'should be observable', () => {
-			const root = new RootEditableElement( createDocumentMock(), 'div' );
+			const root = new RootEditableElement( 'div' );
+			root.document = createDocumentMock();
 
 			expect( root.isFocused ).to.be.false;
 
@@ -73,7 +114,8 @@ describe( 'EditableElement', () => {
 
 	describe( 'isReadOnly', () => {
 		it( 'should be observable', () => {
-			const root = new RootEditableElement( createDocumentMock(), 'div' );
+			const root = new RootEditableElement( 'div' );
+			root.document = createDocumentMock();
 
 			expect( root.isReadOnly ).to.be.false;
 
@@ -92,7 +134,8 @@ describe( 'EditableElement', () => {
 	describe( 'getDocument', ()=> {
 		it( 'should return document', () => {
 			const docMock = createDocumentMock();
-			const root = new RootEditableElement( docMock, 'div' );
+			const root = new RootEditableElement( 'div' );
+			root.document = docMock;
 
 			expect( root.document ).to.equal( docMock );
 		} );

+ 4 - 2
packages/ckeditor5-engine/tests/view/node.js

@@ -146,7 +146,8 @@ describe( 'Node', () => {
 
 		it( 'should return Document attached to the parent element', () => {
 			const docMock = createDocumentMock();
-			const parent = new RootEditableElement( docMock, 'div' );
+			const parent = new RootEditableElement( 'div' );
+			parent.document = docMock;
 			const child = new Element( 'p' );
 
 			child.parent = parent;
@@ -171,7 +172,8 @@ describe( 'Node', () => {
 		} );
 
 		it( 'should return root element', () => {
-			const parent = new RootEditableElement( createDocumentMock(), 'div' );
+			const parent = new RootEditableElement( 'div' );
+			parent.document = createDocumentMock();
 			const child = new Element( 'p' );
 
 			child.parent = parent;

+ 2 - 1
packages/ckeditor5-engine/tests/view/position.js

@@ -457,7 +457,8 @@ describe( 'Position', () => {
 		it( 'should return EditableElement when position is placed inside', () => {
 			const document = new Document();
 			const p = new Element( 'p' );
-			const editable = new EditableElement( document, 'div', null, p );
+			const editable = new EditableElement( 'div', null, p );
+			editable.document = document;
 			const position = new Position( p, 0 );
 
 			expect( position.editableElement ).to.equal( editable );

+ 16 - 2
packages/ckeditor5-engine/tests/view/rooteditableelement.js

@@ -12,7 +12,8 @@ import createDocumentMock from 'ckeditor5-engine/tests/view/_utils/createdocumen
 describe( 'RootEditableElement', () => {
 	describe( 'constructor()', () => {
 		it( 'should create an element with default root name', () => {
-			const root = new RootEditableElement( createDocumentMock(), 'div' );
+			const root = new RootEditableElement( 'div' );
+			root.document = createDocumentMock();
 
 			expect( root ).to.be.instanceof( EditableElement );
 			expect( root ).to.be.instanceof( ContainerElement );
@@ -25,7 +26,9 @@ describe( 'RootEditableElement', () => {
 		} );
 
 		it( 'should create an element with custom root name', () => {
-			const root = new RootEditableElement( createDocumentMock(), 'h1', 'header' );
+			const root = new RootEditableElement( 'h1' );
+			root.document = createDocumentMock();
+			root.rootName = 'header';
 
 			expect( root.rootName ).to.equal( 'header' );
 			expect( root.name ).to.equal( 'h1' );
@@ -34,4 +37,15 @@ describe( 'RootEditableElement', () => {
 			expect( root.isReadOnly ).to.be.false;
 		} );
 	} );
+
+	it( 'should be cloned properly', () => {
+		const root = new RootEditableElement( 'h1' );
+		root.document = createDocumentMock();
+		root.rootName = 'header';
+
+		const newRoot = root.clone();
+
+		expect( newRoot.document ).to.equal( root.document );
+		expect( newRoot.rootName ).to.equal( root.rootName );
+	} );
 } );

+ 4 - 2
packages/ckeditor5-engine/tests/view/textproxy.js

@@ -68,7 +68,8 @@ describe( 'TextProxy', () => {
 
 		it( 'should return Document attached to the parent element', () => {
 			const docMock = createDocumentMock();
-			const root = new RootEditableElement( docMock, 'div' );
+			const root = new RootEditableElement( 'div' );
+			root.document = docMock;
 
 			wrapper.parent = root;
 
@@ -84,7 +85,8 @@ describe( 'TextProxy', () => {
 
 	describe( 'getRoot', () => {
 		it( 'should return root element', () => {
-			const root = new RootEditableElement( createDocumentMock(), 'div' );
+			const root = new RootEditableElement( 'div' );
+			root.document = createDocumentMock();
 
 			wrapper.parent = root;