From 25f6d36c89895972cf49a1e58074130210fae7cb Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Thu, 5 May 2022 14:33:35 +0200 Subject: [PATCH 1/4] Do not set an attribute on child nodes if parent was not converted. --- .../src/conversion/upcasthelpers.js | 7 ++++++ .../tests/conversion/upcasthelpers.js | 24 +++++++++++++++++++ .../ckeditor5-html-support/src/converters.js | 2 +- 3 files changed, 32 insertions(+), 1 deletion(-) 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..70d3af39625 100644 --- a/packages/ckeditor5-html-support/src/converters.js +++ b/packages/ckeditor5-html-support/src/converters.js @@ -159,7 +159,7 @@ export function attributeToViewInlineConverter( { priority, view: viewName } ) { export function viewToModelBlockAttributeConverter( { view: viewName }, dataFilter ) { return dispatcher => { dispatcher.on( `element:${ viewName }`, ( evt, data, conversionApi ) => { - if ( !data.modelRange ) { + if ( !data.modelRange || data.modelRange.isCollapsed ) { return; } From d8074ecdc7cc349526c42429524b7968e5aa0b94 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Thu, 5 May 2022 15:49:08 +0200 Subject: [PATCH 2/4] Tests: Corrected table tests that were accidentally upcasting attribute values despite figure missing the "table" class and not being upcasted. --- .../tests/tableproperties/tablepropertiesediting.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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; From bbb8e7dc5e1be2c8204988fbe0fc9cd19a7d832b Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Fri, 6 May 2022 16:00:25 +0200 Subject: [PATCH 3/4] Added a test to check if attributes of an element that converted into a collapsed range are consumed by GHS. --- .../ckeditor5-html-support/src/converters.js | 4 ++++ .../tests/datafilter.js | 20 +++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/packages/ckeditor5-html-support/src/converters.js b/packages/ckeditor5-html-support/src/converters.js index 70d3af39625..b11e747c192 100644 --- a/packages/ckeditor5-html-support/src/converters.js +++ b/packages/ckeditor5-html-support/src/converters.js @@ -159,6 +159,10 @@ export function attributeToViewInlineConverter( { priority, view: viewName } ) { export function viewToModelBlockAttributeConverter( { view: viewName }, dataFilter ) { return dispatcher => { dispatcher.on( `element:${ viewName }`, ( 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. 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 ) => { From 7e7dbd021c448a7e64ec250fbaac4196e60c8edc Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Fri, 6 May 2022 16:29:12 +0200 Subject: [PATCH 4/4] Tests: Added an integration test. --- .../tests/integrations/table.js | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) 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.