Merge pull request #11707 from ckeditor/ck/11000-alignment-ghs-clash

Fix (html-support,engine): Attributes should not be set if parent was converted into a collapsed range. Closes #11000.

packages/ckeditor5-engine/src/conversion/upcasthelpers.js

@@ -867,6 +867,13 @@ function prepareToAttributeConverter( config, shallow ) {
 	const matcher = new Matcher( config.view );
 
 	return ( evt, data, conversionApi ) => {
+		// Converting an attribute of an element that has not been converted to anything does not make sense
+		// because there will be nowhere to set that attribute on. At this stage, the element should've already
+		// been converted (https://github.com/ckeditor/ckeditor5/issues/11000).
+		if ( !data.modelRange && shallow ) {
+			return;
+		}
+
 		const match = matcher.match( data.viewItem );
 
 		// If there is no match, this callback should not do anything.

packages/ckeditor5-engine/tests/conversion/upcasthelpers.js

@@ -735,6 +735,30 @@ describe( 'UpcastHelpers', () => {
 			);
 		} );
 
+		// https://github.com/ckeditor/ckeditor5/issues/11000
+		it( 'should not set an attribute on child nodes if parent was not converted', () => {
+			upcastHelpers.elementToElement( { view: 'p', model: 'paragraph' } );
+			upcastHelpers.attributeToAttribute( { view: { key: 'foo' }, model: 'foo' } );
+
+			schema.extend( 'paragraph', {
+				allowAttributes: [ 'foo' ]
+			} );
+
+			schema.extend( '$text', {
+				allowAttributes: [ 'foo' ]
+			} );
+
+			const viewElement = viewParse(
+				'<div foo="foo-value">abc</div>' +
+				'<p foo="foo-value">def</p>'
+			);
+
+			expectResult(
+				viewElement,
+				'abc<paragraph foo="foo-value">def</paragraph>'
+			);
+		} );
+
 		// #9536.
 		describe( 'calling the `model.value()` callback', () => {
 			it( 'should not call the `model.view()` callback if the attribute was already consumed', () => {

packages/ckeditor5-html-support/src/converters.js

@@ -159,7 +159,11 @@ export function attributeToViewInlineConverter( { priority, view: viewName } ) {
 export function viewToModelBlockAttributeConverter( { view: viewName }, dataFilter ) {
 	return dispatcher => {
 		dispatcher.on( `element:${ viewName }`, ( evt, data, conversionApi ) => {
-			if ( !data.modelRange ) {
+			// Converting an attribute of an element that has not been converted to anything does not make sense
+			// because there will be nowhere to set that attribute on. At this stage, the element should've already
+			// been converted. A collapsed range can show up in to-do lists (<input>) or complex widgets (e.g. table).
+			// (https://github.com/ckeditor/ckeditor5/issues/11000).
+			if ( !data.modelRange || data.modelRange.isCollapsed ) {
 				return;
 			}
 

packages/ckeditor5-html-support/tests/datafilter.js

@@ -722,6 +722,26 @@ describe( 'DataFilter', () => {
 			expect( editor.getData() ).to.equal( '<section><p>foo</p></section>' );
 		} );
 
+		// https://github.com/ckeditor/ckeditor5/issues/11000
+		it( 'should not consume element attributes if the element was consumed into a collapsed range', () => {
+			dataFilter.allowElement( 'input' );
+			dataFilter.allowAttributes( { name: 'input', attributes: true } );
+
+			editor.data.upcastDispatcher.on( 'element:input', ( evt, data, conversionApi ) => {
+				if ( conversionApi.consumable.consume( data.viewItem, { name: true } ) ) {
+					data.modelRange = conversionApi.writer.createRange( data.modelCursor );
+				}
+			} );
+
+			editor.data.upcastDispatcher.on( 'element:input', ( evt, data, conversionApi ) => {
+				const areConsumable = conversionApi.consumable.test( data.viewItem, { attributes: [ 'type', 'disabled' ] } );
+
+				expect( areConsumable ).to.be.true;
+			}, { priority: 'lowest' } );
+
+			editor.setData( '<p>foo<input type="checkbox" disabled="disabled">bar</p>' );
+		} );
+
 		it( 'should not create empty htmlA (upcast)', () => {
 			editor.conversion.for( 'upcast' ).add( dispatcher => {
 				dispatcher.on( 'element:a', ( evt, data, conversionApi ) => {

packages/ckeditor5-html-support/tests/integrations/table.js

@@ -877,6 +877,61 @@ describe( 'TableElementSupport', () => {
 		);
 	} );
 
+	// https://github.com/ckeditor/ckeditor5/issues/11000
+	it( 'should not strip allowed attributes from elements that are not directly upcasted (like <thead> or <tbody>) ' +
+		'if another upcast converter exists for all possible view elements', async () => {
+		const editor = await ClassicTestEditor.create( editorElement, {
+			plugins: [ Table, TableCaption, Paragraph, GeneralHtmlSupport, function( editor ) {
+				editor.conversion.for( 'upcast' ).attributeToAttribute( {
+					view: 'align',
+					model: 'alignment'
+				} );
+			} ],
+			htmlSupport: {
+				allow: [
+					{
+						name: /^(figure|table|tbody|thead|tr|th|td)$/,
+						attributes: true
+					}
+				]
+			}
+		} );
+
+		editor.setData(
+			'<table>' +
+				'<thead align="right" dir="ltr" lang="en" valign="bottom">' +
+					'<tr>' +
+						'<th>Bar</th>' +
+					'</tr>' +
+				'</thead>' +
+				'<tbody align="right" dir="ltr" lang="en" valign="bottom">' +
+					'<tr align="right" valign="bottom">' +
+						'<td align="right" valign="bottom">Foo</td>' +
+					'</tr>' +
+				'</tbody>' +
+			'</table>'
+		);
+
+		expect( editor.getData() ).to.equalMarkup(
+			'<figure class="table">' +
+				'<table>' +
+					'<thead valign="bottom" lang="en" dir="ltr" align="right">' +
+						'<tr>' +
+							'<th>Bar</th>' +
+						'</tr>' +
+					'</thead>' +
+					'<tbody valign="bottom" lang="en" dir="ltr" align="right">' +
+						'<tr valign="bottom" align="right">' +
+							'<td valign="bottom" align="right">Foo</td>' +
+						'</tr>' +
+					'</tbody>' +
+				'</table>' +
+			'</figure>'
+		);
+
+		await editor.destroy();
+	} );
+
 	describe( 'TableCaption', () => {
 		// Sanity tests verifying if table caption is correctly handled by default converters.
 

packages/ckeditor5-table/tests/tableproperties/tablepropertiesediting.js

@@ -952,7 +952,7 @@ describe( 'table properties', () => {
 				} );
 
 				it( 'should upcast width from <figure>', () => {
-					editor.setData( '<figure style="width:1337px"><table><tr><td>foo</td></tr></table></figure>' );
+					editor.setData( '<figure class="table" style="width:1337px"><table><tr><td>foo</td></tr></table></figure>' );
 					const table = model.document.getRoot().getNodeByPath( [ 0 ] );
 
 					expect( table.getAttribute( 'tableWidth' ) ).to.equal( '1337px' );
@@ -1026,7 +1026,7 @@ describe( 'table properties', () => {
 				} );
 
 				it( 'should upcast height from <figure>', () => {
-					editor.setData( '<figure style="height:1337px"><table><tr><td>foo</td></tr></table></figure>' );
+					editor.setData( '<figure class="table" style="height:1337px"><table><tr><td>foo</td></tr></table></figure>' );
 					const table = model.document.getRoot().getNodeByPath( [ 0 ] );
 
 					expect( table.getAttribute( 'tableHeight' ) ).to.equal( '1337px' );
@@ -1323,7 +1323,7 @@ describe( 'table properties', () => {
 				} );
 
 				it( 'should not upcast the default `width` value from <figure>', () => {
-					editor.setData( '<figure style="width:250px"><table><tr><td>foo</td></tr></table></figure>' );
+					editor.setData( '<figure class="table" style="width:250px"><table><tr><td>foo</td></tr></table></figure>' );
 					const table = model.document.getRoot().getNodeByPath( [ 0 ] );
 
 					expect( table.getAttribute( 'width' ) ).to.be.undefined;
@@ -1339,7 +1339,7 @@ describe( 'table properties', () => {
 				} );
 
 				it( 'should not upcast the default `height` value from <figure>', () => {
-					editor.setData( '<figure style="height:150px"><table><tr><td>foo</td></tr></table></figure>' );
+					editor.setData( '<figure class="table" style="height:150px"><table><tr><td>foo</td></tr></table></figure>' );
 					const table = model.document.getRoot().getNodeByPath( [ 0 ] );
 
 					expect( table.getAttribute( 'height' ) ).to.be.undefined;