diff --git a/packages/ckeditor5-engine/src/conversion/upcasthelpers.js b/packages/ckeditor5-engine/src/conversion/upcasthelpers.js index c51c2789584..b67d64e7611 100644 --- a/packages/ckeditor5-engine/src/conversion/upcasthelpers.js +++ b/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. diff --git a/packages/ckeditor5-engine/tests/conversion/upcasthelpers.js b/packages/ckeditor5-engine/tests/conversion/upcasthelpers.js index 16dc3da1784..f658308d9b7 100644 --- a/packages/ckeditor5-engine/tests/conversion/upcasthelpers.js +++ b/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( + '
abc
' + + '

def

' + ); + + expectResult( + viewElement, + 'abcdef' + ); + } ); + // #9536. describe( 'calling the `model.value()` callback', () => { it( 'should not call the `model.view()` callback if the attribute was already consumed', () => { diff --git a/packages/ckeditor5-html-support/src/converters.js b/packages/ckeditor5-html-support/src/converters.js index f7365d13968..b11e747c192 100644 --- a/packages/ckeditor5-html-support/src/converters.js +++ b/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 () or complex widgets (e.g. table). + // (https://github.com/ckeditor/ckeditor5/issues/11000). + if ( !data.modelRange || data.modelRange.isCollapsed ) { return; } diff --git a/packages/ckeditor5-html-support/tests/datafilter.js b/packages/ckeditor5-html-support/tests/datafilter.js index b1b085be66a..006485b88d4 100644 --- a/packages/ckeditor5-html-support/tests/datafilter.js +++ b/packages/ckeditor5-html-support/tests/datafilter.js @@ -722,6 +722,26 @@ describe( 'DataFilter', () => { expect( editor.getData() ).to.equal( '

foo

' ); } ); + // 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( '

foobar

' ); + } ); + it( 'should not create empty htmlA (upcast)', () => { editor.conversion.for( 'upcast' ).add( dispatcher => { dispatcher.on( 'element:a', ( evt, data, conversionApi ) => { diff --git a/packages/ckeditor5-html-support/tests/integrations/table.js b/packages/ckeditor5-html-support/tests/integrations/table.js index bb6545f5f56..2fe56478da9 100644 --- a/packages/ckeditor5-html-support/tests/integrations/table.js +++ b/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 or ) ' + + '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( + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '
Bar
Foo
' + ); + + expect( editor.getData() ).to.equalMarkup( + '
' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '
Bar
Foo
' + + '
' + ); + + await editor.destroy(); + } ); + describe( 'TableCaption', () => { // Sanity tests verifying if table caption is correctly handled by default converters. diff --git a/packages/ckeditor5-table/tests/tableproperties/tablepropertiesediting.js b/packages/ckeditor5-table/tests/tableproperties/tablepropertiesediting.js index 969b2192742..e90761980cc 100644 --- a/packages/ckeditor5-table/tests/tableproperties/tablepropertiesediting.js +++ b/packages/ckeditor5-table/tests/tableproperties/tablepropertiesediting.js @@ -952,7 +952,7 @@ describe( 'table properties', () => { } ); it( 'should upcast width from
', () => { - editor.setData( '
foo
' ); + editor.setData( '
foo
' ); 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
', () => { - editor.setData( '
foo
' ); + editor.setData( '
foo
' ); 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
', () => { - editor.setData( '
foo
' ); + editor.setData( '
foo
' ); 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
', () => { - editor.setData( '
foo
' ); + editor.setData( '
foo
' ); const table = model.document.getRoot().getNodeByPath( [ 0 ] ); expect( table.getAttribute( 'height' ) ).to.be.undefined;