Merge pull request #920 from ckeditor/t/919

Fix: `ViewConverterBuilder#fromAttribute()` should not create incorrect matcher object for `Matcher` if passed attribute was other than `class` or `style`. Closes #919.

Minor upgrades to `ViewConversionBuilder`:

* converters from `ViewConversionBuilder` will not convert if "creator function" returned `null`.
* simplified view converters building by making `ViewConversionBuilder#toAttribute()` `value` param optional. If not set, the attribute value is taken from converted view element.

src/conversion/buildviewconverter.js

@@ -143,9 +143,24 @@ class ViewConverterBuilder {
 	 */
 	fromAttribute( key, value = /.*/ ) {
 		let pattern = {};
-		pattern[ key ] = value;
 
-		return this.from( pattern );
+		if ( key === 'style' || key === 'class' ) {
+			pattern[ key ] = value;
+		} else {
+			pattern.attribute = {};
+			pattern.attribute[ key ] = value;
+		}
+
+		const matcher = new Matcher( pattern );
+
+		this._from.push( {
+			matcher: matcher,
+			consume: false,
+			priority: null,
+			attributeKey: key
+		} );
+
+		return this;
 	}
 
 	/**
@@ -191,9 +206,9 @@ class ViewConverterBuilder {
 	 *			.toAttribute( 'bold', 'true' } );
 	 *
 	 *		buildViewConverter().for( dispatcher )
-	 *			.fromElement( 'img' ).consuming( { name: true, attributes: [ 'src', 'title' ] } )
+	 *			.fromElement( 'img' ).consuming( { name: true, attribute: [ 'src', 'title' ] } )
 	 *			.toElement( ( viewElement ) => new ModelElement( 'image', { src: viewElement.getAttribute( 'src' ),
-	 *																		title: viewElement.getAttribute( 'title' ) } );
+	 *			                                                            title: viewElement.getAttribute( 'title' ) } );
 	 *
 	 * **Note:** All and only values from passed object has to be consumable on converted view element. This means that
 	 * using `consuming` method, you can either make looser conversion conditions (like in first example) or tighter
@@ -271,6 +286,11 @@ class ViewConverterBuilder {
 					// Create model element basing on creator function or element name.
 					const modelElement = element instanceof Function ? element( data.input ) : new ModelElement( element );
 
+					// Do not convert if element building function returned falsy value.
+					if ( !modelElement ) {
+						continue;
+					}
+
 					// Check whether generated structure is okay with `Schema`.
 					const keys = Array.from( modelElement.getAttributeKeys() );
 
@@ -314,13 +334,15 @@ class ViewConverterBuilder {
 	 * representing attribute key and attribute value or a function that returns an object with `key` and `value` properties.
 	 * If you provide creator function, it will be passed converted view element as first and only parameter.
 	 *
-	 *		buildViewConverter().for( dispatcher ).fromAttribute( 'style', { 'font-weight': 'bold' } ).toAttribute( 'bold', 'true' );
+	 *		buildViewConverter().for( dispatcher ).fromAttribute( 'alt' ).toAttribute( 'alt' );
+	 *		buildViewConverter().for( dispatcher ).fromAttribute( 'style', { 'font-weight': 'bold' } ).toAttribute( 'bold', true );
 	 *		buildViewConverter().for( dispatcher )
 	 *			.fromAttribute( 'class' )
-	 *			.toAttribute( ( viewElement ) => ( { key: 'class', value: viewElement.getAttribute( 'class' ) } ) );
+	 *			.toAttribute( ( viewElement ) => ( { key: 'class', value: 'class-' + viewElement.getAttribute( 'class' ) } ) );
 	 *
 	 * @param {String|Function} keyOrCreator Attribute key or a creator function.
-	 * @param {String} [value] Attribute value. Required if `keyOrCreator` is a `string`. Ignored otherwise.
+	 * @param {String} [value] Attribute value. Ignored if `keyOrCreator` is not a `string`. If `keyOrCreator` is `string`,
+	 * if `value` is not set, attribute value from converted element will be used.
 	 */
 	toAttribute( keyOrCreator, value ) {
 		function eventCallbackGen( from ) {
@@ -348,7 +370,20 @@ class ViewConverterBuilder {
 					}
 
 					// Use attribute creator function, if provided.
-					let attribute = keyOrCreator instanceof Function ? keyOrCreator( data.input ) : { key: keyOrCreator, value: value };
+					let attribute;
+
+					if ( keyOrCreator instanceof Function ) {
+						attribute = keyOrCreator( data.input );
+
+						if ( !attribute ) {
+							return;
+						}
+					} else {
+						attribute = {
+							key: keyOrCreator,
+							value: value ? value : data.input.getAttribute( from.attributeKey )
+						};
+					}
 
 					// Set attribute on current `output`. `Schema` is checked inside this helper function.
 					setAttributeOn( data.output, attribute, data, conversionApi );

src/view/matcher.js

@@ -40,7 +40,7 @@ export default class Matcher {
 	 * Example pattern matching element's attributes:
 	 *
 	 *		matcher.add( {
-	 *			attributes: {
+	 *			attribute: {
 	 *				title: 'foobar',
 	 *				foo: /^\w+/
 	 *			}
@@ -130,9 +130,9 @@ export default class Matcher {
 	 *			pattern: <pattern used to match found element>,
 	 *			match: {
 	 *				name: true,
- 	 *				attributes: [ 'title', 'href' ],
-	 *				classes: [ 'foo' ],
- 	 *				styles: [ 'color', 'position' ]
+	 *				attribute: [ 'title', 'href' ],
+	 *				class: [ 'foo' ],
+	 *				style: [ 'color', 'position' ]
 	 *			}
 	 *		}
 	 *
@@ -302,7 +302,7 @@ function matchAttributes( patterns, element ) {
 				} else {
 					return null;
 				}
-			} else if ( attribute === pattern  ) {
+			} else if ( attribute === pattern ) {
 				match.push( name );
 			} else {
 				return null;

tests/conversion/buildviewconverter.js

@@ -157,21 +157,28 @@ describe( 'View converter builder', () => {
 	} );
 
 	it( 'should convert from view attribute and key to model attribute', () => {
+		schema.allow( { name: 'paragraph', attributes: [ 'type' ], inside: '$root' } );
+
 		dispatcher.on( 'documentFragment', convertToModelFragment() );
 
 		buildViewConverter().for( dispatcher ).fromElement( 'p' ).toElement( 'paragraph' );
 		buildViewConverter().for( dispatcher ).fromAttribute( 'class', 'important' ).toAttribute( 'important', true );
 		buildViewConverter().for( dispatcher ).fromAttribute( 'class', 'theme-nice' ).toAttribute( 'theme', 'nice' );
+		buildViewConverter().for( dispatcher ).fromAttribute( 'data-type' ).toAttribute( 'type' );
 
 		const viewStructure = new ViewDocumentFragment( [
 			new ViewContainerElement( 'p', { class: 'important' }, new ViewText( 'foo' ) ),
-			new ViewContainerElement( 'p', { class: 'important theme-nice' }, new ViewText( 'bar' ) )
+			new ViewContainerElement( 'p', { class: 'important theme-nice' }, new ViewText( 'bar' ) ),
+			new ViewContainerElement( 'p', { 'data-type': 'foo' }, new ViewText( 'xyz' ) )
 		] );
 
 		const conversionResult = dispatcher.convert( viewStructure, objWithContext );
 
-		expect( modelToString( conversionResult ) )
-			.to.equal( '<paragraph important="true">foo</paragraph><paragraph important="true" theme="nice">bar</paragraph>' );
+		expect( modelToString( conversionResult ) ).to.equal(
+			'<paragraph important="true">foo</paragraph>' +
+			'<paragraph important="true" theme="nice">bar</paragraph>' +
+			'<paragraph type="foo">xyz</paragraph>'
+		);
 	} );
 
 	it( 'should convert from multiple view entities to model attribute', () => {
@@ -508,4 +515,38 @@ describe( 'View converter builder', () => {
 
 		expect( modelToString( conversionResult ) ).to.equal( '<paragraph>foo</paragraph>' );
 	} );
+
+	it( 'should stop to element conversion if creating function returned null', () => {
+		buildViewConverter().for( dispatcher ).fromElement( 'p' ).toElement(
+			( viewElement ) => viewElement.hasAttribute( 'stop' ) ? null : new ModelElement( 'paragraph' )
+		);
+
+		let viewElement = new ViewContainerElement( 'p' );
+		let conversionResult = dispatcher.convert( viewElement, objWithContext );
+		expect( modelToString( conversionResult ) ).to.equal( '<paragraph></paragraph>' );
+
+		viewElement.setAttribute( 'stop', true );
+		conversionResult = dispatcher.convert( viewElement, objWithContext );
+		expect( conversionResult ).to.be.null;
+	} );
+
+	it( 'should stop to attribute conversion if creating function returned null', () => {
+		schema.allow( { name: 'paragraph', attributes: [ 'type' ], inside: '$root' } );
+
+		buildViewConverter().for( dispatcher ).fromElement( 'p' ).toElement( 'paragraph' );
+
+		buildViewConverter().for( dispatcher ).fromAttribute( 'data-type' ).toAttribute( ( viewElement ) => {
+			const value = viewElement.getAttribute( 'data-type' );
+
+			return value == 'stop' ? null : { key: 'type', value: value };
+		} );
+
+		let viewElement = new ViewContainerElement( 'p', { 'data-type': 'foo' } );
+		let conversionResult = dispatcher.convert( viewElement, objWithContext );
+		expect( modelToString( conversionResult ) ).to.equal( '<paragraph type="foo"></paragraph>' );
+
+		viewElement.setAttribute( 'data-type', 'stop' );
+		conversionResult = dispatcher.convert( viewElement, objWithContext );
+		expect( modelToString( conversionResult ) ).to.equal( '<paragraph></paragraph>' );
+	} );
 } );