Przeglądaj źródła

Using writer to set attribute on view element.

Szymon Kupś 7 lat temu
rodzic
commit
9e3a2f8128

+ 5 - 2
packages/ckeditor5-engine/src/conversion/downcast-converters.js

@@ -722,16 +722,19 @@ export function changeAttribute( attributeCreator ) {
 
 		// First remove the old attribute if there was one.
 		const oldAttribute = attributeCreator( data.attributeOldValue, data, consumable, conversionApi );
+		const mapper = conversionApi.mapper;
+		const viewWriter = conversionApi.writer;
 
 		if ( data.attributeOldValue !== null && oldAttribute ) {
-			conversionApi.mapper.toViewElement( data.item ).removeAttribute( oldAttribute.key );
+			mapper.toViewElement( data.item ).removeAttribute( oldAttribute.key );
 		}
 
 		// Then, if conversion was successful, set the new attribute.
 		const newAttribute = attributeCreator( data.attributeNewValue, data, consumable, conversionApi );
 
 		if ( data.attributeNewValue !== null && newAttribute ) {
-			conversionApi.mapper.toViewElement( data.item ).setAttribute( newAttribute.key, newAttribute.value );
+			const viewElement = mapper.toViewElement( data.item );
+			viewWriter.setAttribute( newAttribute.key, newAttribute.value, viewElement );
 		}
 	};
 }

+ 1 - 1
packages/ckeditor5-engine/src/dev-utils/view.js

@@ -930,7 +930,7 @@ function _convertElement( viewElement ) {
 
 	// Move attributes.
 	for ( const attributeKey of viewElement.getAttributeKeys() ) {
-		newElement.setAttribute( attributeKey, viewElement.getAttribute( attributeKey ) );
+		newElement._setAttribute( attributeKey, viewElement.getAttribute( attributeKey ) );
 	}
 
 	return newElement;

+ 1 - 1
packages/ckeditor5-engine/src/view/domconverter.js

@@ -413,7 +413,7 @@ export default class DomConverter {
 				const attrs = domNode.attributes;
 
 				for ( let i = attrs.length - 1; i >= 0; i-- ) {
-					viewElement.setAttribute( attrs[ i ].name, attrs[ i ].value );
+					viewElement._setAttribute( attrs[ i ].name, attrs[ i ].value );
 				}
 			}
 

+ 28 - 27
packages/ckeditor5-engine/src/view/element.js

@@ -318,27 +318,6 @@ export default class Element extends Node {
 	}
 
 	/**
-	 * Adds or overwrite attribute with a specified key and value.
-	 *
-	 * @param {String} key Attribute key.
-	 * @param {String} value Attribute value.
-	 * @fires module:engine/view/node~Node#change
-	 */
-	setAttribute( key, value ) {
-		value = String( value );
-
-		this._fireChange( 'attributes', this );
-
-		if ( key == 'class' ) {
-			parseClasses( this._classes, value );
-		} else if ( key == 'style' ) {
-			parseInlineStyles( this._styles, value );
-		} else {
-			this._attrs.set( key, value );
-		}
-	}
-
-	/**
 	 * Inserts a child node or a list of child nodes on the given index and sets the parent of these nodes to
 	 * this element.
 	 *
@@ -686,12 +665,12 @@ export default class Element extends Node {
  	 *
 	 * For example:
 	 *
-	 *		const element = new ViewElement( 'foo' );
-	 *		element.setAttribute( 'banana', '10' );
-	 *		element.setAttribute( 'apple', '20' );
-	 *		element.setStyle( 'color', 'red' );
-	 *		element.setStyle( 'border-color', 'white' );
-	 *		element.addClass( 'baz' );
+	 *		const element = new ViewElement( 'foo', {
+	 *			banana: '10',
+	 *			apple: '20',
+	 *			style: 'color: red; border-color: white;',
+	 *			class: 'baz'
+	 *		} );
 	 *
 	 *		// returns 'foo class="baz" style="border-color:white;color:red" apple="20" banana="10"'
 	 *		element.getIdentity();
@@ -712,6 +691,28 @@ export default class Element extends Node {
 	}
 
 	/**
+	 * Adds or overwrite attribute with a specified key and value.
+	 *
+	 * @protected
+	 * @param {String} key Attribute key.
+	 * @param {String} value Attribute value.
+	 * @fires module:engine/view/node~Node#change
+	 */
+	_setAttribute( key, value ) {
+		value = String( value );
+
+		this._fireChange( 'attributes', this );
+
+		if ( key == 'class' ) {
+			parseClasses( this._classes, value );
+		} else if ( key == 'style' ) {
+			parseInlineStyles( this._styles, value );
+		} else {
+			this._attrs.set( key, value );
+		}
+	}
+
+	/**
 	 * Returns block {@link module:engine/view/filler filler} offset or `null` if block filler is not needed.
 	 *
 	 * @abstract

+ 1 - 1
packages/ckeditor5-engine/src/view/placeholder.js

@@ -42,7 +42,7 @@ export function attachPlaceholder( view, element, placeholderText, checkFunction
 
 	// Store text in element's data attribute.
 	// This data attribute is used in CSS class to show the placeholder.
-	element.setAttribute( 'data-placeholder', placeholderText );
+	element._setAttribute( 'data-placeholder', placeholderText );
 
 	// Store information about placeholder.
 	documentPlaceholders.get( document ).set( element, checkFunction );

+ 70 - 63
packages/ckeditor5-engine/src/view/writer.js

@@ -182,6 +182,10 @@ export default class Writer {
 		return new UIElement( name, attributes );
 	}
 
+	setAttribute( key, value, element ) {
+		element._setAttribute( key, value );
+	}
+
 	/**
 	 * Breaks attribute nodes at provided position or at boundaries of provided range. It breaks attribute elements inside
 	 * up to a container element.
@@ -878,7 +882,7 @@ export default class Writer {
 	 */
 	_wrapRange( range, attribute ) {
 		// Range is inside single attribute and spans on all children.
-		if ( rangeSpansOnAllChildren( range ) && wrapAttributeElement( attribute, range.start.parent ) ) {
+		if ( rangeSpansOnAllChildren( range ) && this._wrapAttributeElement( attribute, range.start.parent ) ) {
 			const parent = range.start.parent;
 
 			const end = this.mergeAttributes( Position.createAfter( parent ) );
@@ -894,7 +898,7 @@ export default class Writer {
 		if ( breakEnd.isEqual( breakStart.getShiftedBy( 1 ) ) ) {
 			const node = breakStart.nodeAfter;
 
-			if ( node instanceof AttributeElement && wrapAttributeElement( attribute, node ) ) {
+			if ( node instanceof AttributeElement && this._wrapAttributeElement( attribute, node ) ) {
 				const start = this.mergeAttributes( breakStart );
 
 				if ( !start.isEqual( breakStart ) ) {
@@ -979,6 +983,70 @@ export default class Writer {
 		// If position is next to text node - move position inside.
 		return movePositionToTextNode( newPosition );
 	}
+
+	/**
+	 * 	Wraps one {@link module:engine/view/attributeelement~AttributeElement AttributeElement} into another by
+	 * 	merging them if possible. When merging is possible - all attributes, styles and classes are moved from wrapper
+	 * 	element to element being wrapped.
+	 *
+	 * 	@private
+	 * 	@param {module:engine/view/attributeelement~AttributeElement} wrapper Wrapper AttributeElement.
+	 * 	@param {module:engine/view/attributeelement~AttributeElement} toWrap AttributeElement to wrap using wrapper element.
+	 * 	@returns {Boolean} Returns `true` if elements are merged.
+	 */
+	_wrapAttributeElement( wrapper, toWrap ) {
+		// Can't merge if name or priority differs.
+		if ( wrapper.name !== toWrap.name || wrapper.priority !== toWrap.priority ) {
+			return false;
+		}
+
+		// Check if attributes can be merged.
+		for ( const key of wrapper.getAttributeKeys() ) {
+			// Classes and styles should be checked separately.
+			if ( key === 'class' || key === 'style' ) {
+				continue;
+			}
+
+			// If some attributes are different we cannot wrap.
+			if ( toWrap.hasAttribute( key ) && toWrap.getAttribute( key ) !== wrapper.getAttribute( key ) ) {
+				return false;
+			}
+		}
+
+		// Check if styles can be merged.
+		for ( const key of wrapper.getStyleNames() ) {
+			if ( toWrap.hasStyle( key ) && toWrap.getStyle( key ) !== wrapper.getStyle( key ) ) {
+				return false;
+			}
+		}
+
+		// Move all attributes/classes/styles from wrapper to wrapped AttributeElement.
+		for ( const key of wrapper.getAttributeKeys() ) {
+			// Classes and styles should be checked separately.
+			if ( key === 'class' || key === 'style' ) {
+				continue;
+			}
+
+			// Move only these attributes that are not present - other are similar.
+			if ( !toWrap.hasAttribute( key ) ) {
+				this.setAttribute( key, wrapper.getAttribute( key ), toWrap );
+			}
+		}
+
+		for ( const key of wrapper.getStyleNames() ) {
+			if ( !toWrap.hasStyle( key ) ) {
+				toWrap.setStyle( key, wrapper.getStyle( key ) );
+			}
+		}
+
+		for ( const key of wrapper.getClassNames() ) {
+			if ( !toWrap.hasClass( key ) ) {
+				toWrap.addClass( key );
+			}
+		}
+
+		return true;
+	}
 }
 
 // Helper function for `view.writer.wrap`. Checks if given element has any children that are not ui elements.
@@ -1230,67 +1298,6 @@ function mergeTextNodes( t1, t2 ) {
 	return new Position( t1, nodeBeforeLength );
 }
 
-// Wraps one {@link module:engine/view/attributeelement~AttributeElement AttributeElement} into another by merging them if possible.
-// When merging is possible - all attributes, styles and classes are moved from wrapper element to element being
-// wrapped.
-//
-// @param {module:engine/view/attributeelement~AttributeElement} wrapper Wrapper AttributeElement.
-// @param {module:engine/view/attributeelement~AttributeElement} toWrap AttributeElement to wrap using wrapper element.
-// @returns {Boolean} Returns `true` if elements are merged.
-function wrapAttributeElement( wrapper, toWrap ) {
-	// Can't merge if name or priority differs.
-	if ( wrapper.name !== toWrap.name || wrapper.priority !== toWrap.priority ) {
-		return false;
-	}
-
-	// Check if attributes can be merged.
-	for ( const key of wrapper.getAttributeKeys() ) {
-		// Classes and styles should be checked separately.
-		if ( key === 'class' || key === 'style' ) {
-			continue;
-		}
-
-		// If some attributes are different we cannot wrap.
-		if ( toWrap.hasAttribute( key ) && toWrap.getAttribute( key ) !== wrapper.getAttribute( key ) ) {
-			return false;
-		}
-	}
-
-	// Check if styles can be merged.
-	for ( const key of wrapper.getStyleNames() ) {
-		if ( toWrap.hasStyle( key ) && toWrap.getStyle( key ) !== wrapper.getStyle( key ) ) {
-			return false;
-		}
-	}
-
-	// Move all attributes/classes/styles from wrapper to wrapped AttributeElement.
-	for ( const key of wrapper.getAttributeKeys() ) {
-		// Classes and styles should be checked separately.
-		if ( key === 'class' || key === 'style' ) {
-			continue;
-		}
-
-		// Move only these attributes that are not present - other are similar.
-		if ( !toWrap.hasAttribute( key ) ) {
-			toWrap.setAttribute( key, wrapper.getAttribute( key ) );
-		}
-	}
-
-	for ( const key of wrapper.getStyleNames() ) {
-		if ( !toWrap.hasStyle( key ) ) {
-			toWrap.setStyle( key, wrapper.getStyle( key ) );
-		}
-	}
-
-	for ( const key of wrapper.getClassNames() ) {
-		if ( !toWrap.hasClass( key ) ) {
-			toWrap.addClass( key );
-		}
-	}
-
-	return true;
-}
-
 // Unwraps {@link module:engine/view/attributeelement~AttributeElement AttributeElement} from another by removing corresponding attributes,
 // classes and styles. All attributes, classes and styles from wrapper should be present inside element being unwrapped.
 //

+ 3 - 1
packages/ckeditor5-engine/tests/conversion/downcast-converters.js

@@ -1307,7 +1307,9 @@ describe( 'downcast-converters', () => {
 					} );
 
 					viewContainer.setCustomProperty( 'removeHighlight', element => {
-						element.setAttribute( 'class', '' );
+						controller.view.change( writer => {
+							writer.setAttribute( 'class', '', element );
+						} );
 					} );
 
 					return viewContainer;

+ 2 - 2
packages/ckeditor5-engine/tests/conversion/viewconsumable.js

@@ -474,8 +474,8 @@ describe( 'ViewConsumable', () => {
 		} );
 
 		it( 'should add all attributes', () => {
-			el.setAttribute( 'title', 'foobar' );
-			el.setAttribute( 'href', 'https://ckeditor.com' );
+			el._setAttribute( 'title', 'foobar' );
+			el._setAttribute( 'href', 'https://ckeditor.com' );
 
 			const consumables = ViewConsumable.consumablesFromElement( el );
 			expect( consumables.attribute.length ).to.equal( 2 );

+ 3 - 9
packages/ckeditor5-engine/tests/view/domconverter/view-to-dom.js

@@ -29,9 +29,7 @@ describe( 'DomConverter', () => {
 		it( 'should create tree of DOM elements from view elements', () => {
 			const viewImg = new ViewElement( 'img' );
 			const viewText = new ViewText( 'foo' );
-			const viewP = new ViewElement( 'p' );
-
-			viewP.setAttribute( 'class', 'foo' );
+			const viewP = new ViewElement( 'p', { class: 'foo' } );
 
 			viewP.appendChildren( viewImg );
 			viewP.appendChildren( viewText );
@@ -59,9 +57,7 @@ describe( 'DomConverter', () => {
 		it( 'should create tree of DOM elements from view elements and bind elements', () => {
 			const viewImg = new ViewElement( 'img' );
 			const viewText = new ViewText( 'foo' );
-			const viewP = new ViewElement( 'p' );
-
-			viewP.setAttribute( 'class', 'foo' );
+			const viewP = new ViewElement( 'p', { class: 'foo' } );
 
 			viewP.appendChildren( viewImg );
 			viewP.appendChildren( viewText );
@@ -98,9 +94,7 @@ describe( 'DomConverter', () => {
 		it( 'should create tree of DOM elements from view element without children', () => {
 			const viewImg = new ViewElement( 'img' );
 			const viewText = new ViewText( 'foo' );
-			const viewP = new ViewElement( 'p' );
-
-			viewP.setAttribute( 'class', 'foo' );
+			const viewP = new ViewElement( 'p', { class: 'foo' } );
 
 			viewP.appendChildren( viewImg );
 			viewP.appendChildren( viewText );

+ 21 - 21
packages/ckeditor5-engine/tests/view/element.js

@@ -247,8 +247,8 @@ describe( 'Element', () => {
 			const other1 = el.clone();
 			const other2 = el.clone();
 			const other3 = el.clone();
-			other1.setAttribute( 'baz', 'qux' );
-			other2.setAttribute( 'foo', 'not-bar' );
+			other1._setAttribute( 'baz', 'qux' );
+			other2._setAttribute( 'foo', 'not-bar' );
 			other3.removeAttribute( 'foo' );
 			expect( el.isSimilar( other1 ) ).to.be.false;
 			expect( el.isSimilar( other2 ) ).to.be.false;
@@ -432,14 +432,14 @@ describe( 'Element', () => {
 
 		describe( 'setAttribute', () => {
 			it( 'should set attribute', () => {
-				el.setAttribute( 'foo', 'bar' );
+				el._setAttribute( 'foo', 'bar' );
 
 				expect( el._attrs.has( 'foo' ) ).to.be.true;
 				expect( el._attrs.get( 'foo' ) ).to.equal( 'bar' );
 			} );
 
 			it( 'should cast attribute value to a string', () => {
-				el.setAttribute( 'foo', true );
+				el._setAttribute( 'foo', true );
 
 				expect( el._attrs.get( 'foo' ) ).to.equal( 'true' );
 			} );
@@ -450,11 +450,11 @@ describe( 'Element', () => {
 					done();
 				} );
 
-				el.setAttribute( 'foo', 'bar' );
+				el._setAttribute( 'foo', 'bar' );
 			} );
 
 			it( 'should set class', () => {
-				el.setAttribute( 'class', 'foo bar' );
+				el._setAttribute( 'class', 'foo bar' );
 
 				expect( el._attrs.has( 'class' ) ).to.be.false;
 				expect( el._classes.has( 'foo' ) ).to.be.true;
@@ -462,8 +462,8 @@ describe( 'Element', () => {
 			} );
 
 			it( 'should replace all existing classes', () => {
-				el.setAttribute( 'class', 'foo bar baz' );
-				el.setAttribute( 'class', 'qux' );
+				el._setAttribute( 'class', 'foo bar baz' );
+				el._setAttribute( 'class', 'qux' );
 
 				expect( el._classes.has( 'foo' ) ).to.be.false;
 				expect( el._classes.has( 'bar' ) ).to.be.false;
@@ -474,7 +474,7 @@ describe( 'Element', () => {
 			it( 'should replace all styles', () => {
 				el.setStyle( 'color', 'red' );
 				el.setStyle( 'top', '10px' );
-				el.setAttribute( 'style', 'border:none' );
+				el._setAttribute( 'style', 'border:none' );
 
 				expect( el.hasStyle( 'color' ) ).to.be.false;
 				expect( el.hasStyle( 'top' ) ).to.be.false;
@@ -485,7 +485,7 @@ describe( 'Element', () => {
 
 		describe( 'getAttribute', () => {
 			it( 'should return attribute', () => {
-				el.setAttribute( 'foo', 'bar' );
+				el._setAttribute( 'foo', 'bar' );
 
 				expect( el.getAttribute( 'foo' ) ).to.equal( 'bar' );
 				expect( el.getAttribute( 'bom' ) ).to.not.be.ok;
@@ -515,15 +515,15 @@ describe( 'Element', () => {
 
 		describe( 'getAttributes', () => {
 			it( 'should return attributes', () => {
-				el.setAttribute( 'foo', 'bar' );
-				el.setAttribute( 'abc', 'xyz' );
+				el._setAttribute( 'foo', 'bar' );
+				el._setAttribute( 'abc', 'xyz' );
 
 				expect( Array.from( el.getAttributes() ) ).to.deep.equal( [ [ 'foo', 'bar' ], [ 'abc', 'xyz' ] ] );
 			} );
 
 			it( 'should return class and style attribute', () => {
-				el.setAttribute( 'class', 'abc' );
-				el.setAttribute( 'style', 'width:20px;' );
+				el._setAttribute( 'class', 'abc' );
+				el._setAttribute( 'style', 'width:20px;' );
 				el.addClass( 'xyz' );
 				el.setStyle( 'font-weight', 'bold' );
 
@@ -535,7 +535,7 @@ describe( 'Element', () => {
 
 		describe( 'hasAttribute', () => {
 			it( 'should return true if element has attribute', () => {
-				el.setAttribute( 'foo', 'bar' );
+				el._setAttribute( 'foo', 'bar' );
 
 				expect( el.hasAttribute( 'foo' ) ).to.be.true;
 				expect( el.hasAttribute( 'bom' ) ).to.be.false;
@@ -556,8 +556,8 @@ describe( 'Element', () => {
 
 		describe( 'getAttributeKeys', () => {
 			it( 'should return keys', () => {
-				el.setAttribute( 'foo', true );
-				el.setAttribute( 'bar', true );
+				el._setAttribute( 'foo', true );
+				el._setAttribute( 'bar', true );
 
 				const expected = [ 'foo', 'bar' ];
 				let i = 0;
@@ -572,7 +572,7 @@ describe( 'Element', () => {
 
 			it( 'should return class key', () => {
 				el.addClass( 'foo' );
-				el.setAttribute( 'bar', true );
+				el._setAttribute( 'bar', true );
 				const expected = [ 'class', 'bar' ];
 				let i = 0;
 
@@ -584,7 +584,7 @@ describe( 'Element', () => {
 
 			it( 'should return style key', () => {
 				el.setStyle( 'color', 'black' );
-				el.setAttribute( 'bar', true );
+				el._setAttribute( 'bar', true );
 				const expected = [ 'style', 'bar' ];
 				let i = 0;
 
@@ -597,7 +597,7 @@ describe( 'Element', () => {
 
 		describe( 'removeAttribute', () => {
 			it( 'should remove attributes', () => {
-				el.setAttribute( 'foo', true );
+				el._setAttribute( 'foo', true );
 
 				expect( el.hasAttribute( 'foo' ) ).to.be.true;
 
@@ -609,7 +609,7 @@ describe( 'Element', () => {
 			} );
 
 			it( 'should fire change event with attributes type', done => {
-				el.setAttribute( 'foo', 'bar' );
+				el._setAttribute( 'foo', 'bar' );
 				el.once( 'change:attributes', eventInfo => {
 					expect( eventInfo.source ).to.equal( el );
 					done();

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

@@ -303,8 +303,7 @@ describe( 'Node', () => {
 
 		beforeEach( () => {
 			text = new Text( 'foo' );
-			img = new Element( 'img' );
-			img.setAttribute( 'src', 'img.png' );
+			img = new Element( 'img', { 'src': 'img.png' } );
 
 			root = new Element( 'p', { renderer: { markToSync: rootChangeSpy } } );
 			root.appendChildren( [ text, img ] );
@@ -323,14 +322,14 @@ describe( 'Node', () => {
 				imgChangeSpy( 'attributes', node );
 			} );
 
-			img.setAttribute( 'width', 100 );
+			img._setAttribute( 'width', 100 );
 
 			sinon.assert.calledOnce( imgChangeSpy );
 			sinon.assert.calledWith( imgChangeSpy, 'attributes', img );
 		} );
 
 		it( 'should be fired on the parent', () => {
-			img.setAttribute( 'width', 100 );
+			img._setAttribute( 'width', 100 );
 
 			sinon.assert.calledOnce( rootChangeSpy );
 			sinon.assert.calledWith( rootChangeSpy, 'attributes', img );
@@ -338,7 +337,7 @@ describe( 'Node', () => {
 
 		describe( 'setAttribute()', () => {
 			it( 'should fire change event', () => {
-				img.setAttribute( 'width', 100 );
+				img._setAttribute( 'width', 100 );
 
 				sinon.assert.calledOnce( rootChangeSpy );
 				sinon.assert.calledWith( rootChangeSpy, 'attributes', img );

+ 3 - 3
packages/ckeditor5-engine/tests/view/renderer.js

@@ -51,7 +51,7 @@ describe( 'Renderer', () => {
 		} );
 
 		it( 'should mark attributes which need update', () => {
-			viewRoot.setAttribute( 'class', 'foo' );
+			viewRoot._setAttribute( 'class', 'foo' );
 
 			renderer.markToSync( 'attributes', viewRoot );
 
@@ -151,7 +151,7 @@ describe( 'Renderer', () => {
 		} );
 
 		it( 'should update attributes', () => {
-			viewRoot.setAttribute( 'class', 'foo' );
+			viewRoot._setAttribute( 'class', 'foo' );
 
 			renderer.markToSync( 'attributes', viewRoot );
 			renderer.render();
@@ -162,7 +162,7 @@ describe( 'Renderer', () => {
 		} );
 
 		it( 'should remove attributes', () => {
-			viewRoot.setAttribute( 'class', 'foo' );
+			viewRoot._setAttribute( 'class', 'foo' );
 			domRoot.setAttribute( 'id', 'bar' );
 			domRoot.setAttribute( 'class', 'bar' );
 

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

@@ -432,7 +432,7 @@ describe( 'view', () => {
 			expect( domRoot.childNodes.length ).to.equal( 1 );
 			expect( domRoot.childNodes[ 0 ].getAttribute( 'class' ) ).to.equal( 'foo' );
 
-			viewP.setAttribute( 'class', 'bar' );
+			viewP._setAttribute( 'class', 'bar' );
 			view.render();
 
 			expect( domRoot.childNodes.length ).to.equal( 1 );