diff --git a/packages/ckeditor5-alignment/tests/integration.js b/packages/ckeditor5-alignment/tests/integration.js index 7ebb228c963..eff47a4a30a 100644 --- a/packages/ckeditor5-alignment/tests/integration.js +++ b/packages/ckeditor5-alignment/tests/integration.js @@ -95,4 +95,49 @@ describe( 'Alignment integration', () => { ); } ); } ); + + describe( 'compatibility with \'to-model-attribute\' converter', () => { + it( 'should not set the "alignment" attribute if the schema does not allow', () => { + // See: https://github.com/ckeditor/ckeditor5/pull/9249#issuecomment-815658459. + editor.model.schema.register( 'div', { + inheritAllFrom: '$block', + allowAttributes: [ 'customAlignment' ] + } ); + + // Does not allow for setting the "alignment" attribute for `div` elements. + editor.model.schema.addAttributeCheck( ( context, attributeName ) => { + if ( context.endsWith( 'div' ) && attributeName == 'alignment' ) { + return false; + } + } ); + + editor.conversion.elementToElement( { model: 'div', view: 'div' } ); + + editor.conversion.attributeToAttribute( { + model: { + name: 'div', + key: 'customAlignment', + values: [ 'right' ] + }, + view: { + right: { + key: 'style', + value: { + 'text-align': 'right' + } + } + } + } ); + + // Conversion for the `style[text-align]` attribue will be called twice. + // - The first one comes from the AlignmentEditing plugin, + // - The second one from the test. + editor.setData( '
foo
' ); + + // As we do not allow for the `alignment` attribute for the `div` element, we expect + // that the `customAlignment` property will be set. + expect( getModelData( editor.model, { withoutSelection: true } ) ).to.equal( '
foo
' ); + expect( editor.getData() ).to.equal( '
foo
' ); + } ); + } ); } ); diff --git a/packages/ckeditor5-engine/src/conversion/upcasthelpers.js b/packages/ckeditor5-engine/src/conversion/upcasthelpers.js index 5a828846172..a6ae345b1f6 100644 --- a/packages/ckeditor5-engine/src/conversion/upcasthelpers.js +++ b/packages/ckeditor5-engine/src/conversion/upcasthelpers.js @@ -889,8 +889,8 @@ function prepareToAttributeConverter( config, shallow ) { return; } - // Since we are converting to attribute we need an range on which we will set the attribute. - // If the range is not created yet, we will create it. + // Since we are converting to attribute we need a range on which we will set the attribute. + // If the range is not created yet, let's create it by converting children of the current node first. if ( !data.modelRange ) { // Convert children and set conversion result as a current data. data = Object.assign( data, conversionApi.convertChildren( data.viewItem, data.modelCursor ) ); @@ -899,6 +899,8 @@ function prepareToAttributeConverter( config, shallow ) { // Set attribute on current `output`. `Schema` is checked inside this helper function. const attributeWasSet = setAttributeOn( data.modelRange, { key: modelKey, value: modelValue }, shallow, conversionApi ); + // It may happen that a converter will try to set an attribute that is not allowed in the given context. + // In such a situation we cannot consume the attribute. See: https://github.com/ckeditor/ckeditor5/pull/9249#issuecomment-815658459. if ( attributeWasSet ) { conversionApi.consumable.consume( data.viewItem, match.match ); } @@ -923,6 +925,8 @@ function onlyViewNameIsDefined( viewConfig, viewItem ) { // Helper function for to-model-attribute converter. Sets model attribute on given range. Checks {@link module:engine/model/schema~Schema} // to ensure proper model structure. // +// If any node on the given range has already defined an attribute with the same name, its value will not be updated. +// // @param {module:engine/model/range~Range} modelRange Model range on which attribute should be set. // @param {Object} modelAttribute Model attribute to set. // @param {module:engine/conversion/upcastdispatcher~UpcastConversionApi} conversionApi Conversion API. @@ -934,11 +938,21 @@ function setAttributeOn( modelRange, modelAttribute, shallow, conversionApi ) { // Set attribute on each item in range according to Schema. for ( const node of Array.from( modelRange.getItems( { shallow } ) ) ) { - if ( conversionApi.schema.checkAttribute( node, modelAttribute.key ) ) { - conversionApi.writer.setAttribute( modelAttribute.key, modelAttribute.value, node ); + // Skip if not allowed. + if ( !conversionApi.schema.checkAttribute( node, modelAttribute.key ) ) { + continue; + } - result = true; + // Mark the node as consumed even if the attribute will not be updated because it's in a valid context (schema) + // and would be converted if the attribute wouldn't be present. See #8921. + result = true; + + // Do not override the attribute if it's already present. + if ( node.hasAttribute( modelAttribute.key ) ) { + continue; } + + conversionApi.writer.setAttribute( modelAttribute.key, modelAttribute.value, node ); } return result; diff --git a/packages/ckeditor5-engine/tests/conversion/upcasthelpers.js b/packages/ckeditor5-engine/tests/conversion/upcasthelpers.js index 5f1d12baf04..37ecbe6bf15 100644 --- a/packages/ckeditor5-engine/tests/conversion/upcasthelpers.js +++ b/packages/ckeditor5-engine/tests/conversion/upcasthelpers.js @@ -376,6 +376,125 @@ describe( 'UpcastHelpers', () => { '<$text bold="true">Foo' ); } ); + + // #8921. + describe( 'overwriting attributes while converting nested elements', () => { + beforeEach( () => { + schema.extend( '$text', { + allowAttributes: [ 'fontSize', 'fontColor' ] + } ); + + upcastHelpers.elementToAttribute( { + view: { + name: 'span', + styles: { + 'font-size': /[\s\S]+/ + } + }, + model: { + key: 'fontSize', + value: viewElement => { + const fontSize = viewElement.getStyle( 'font-size' ); + const value = fontSize.substr( 0, fontSize.length - 2 ); + + return Number( value ); + } + } + } ); + + upcastHelpers.elementToAttribute( { + view: { + name: 'span', + styles: { + 'color': /#[a-f0-9]{6}/ + } + }, + model: { + key: 'fontColor', + value: viewElement => viewElement.getStyle( 'color' ) + } + } ); + } ); + + it( 'should not overwrite attributes if nested elements have the same attribute but different values', () => { + const viewElement = viewParse( 'Bar' ); + + expectResult( + viewElement, + '<$text fontSize="11">Bar' + ); + } ); + + it( 'should convert text before the nested duplicated attribute with the most outer value', () => { + const viewElement = viewParse( 'FooBar' ); + + expectResult( + viewElement, + '<$text fontSize="9">Foo<$text fontSize="11">Bar' + ); + } ); + + it( 'should convert text after the nested duplicated attribute with the most outer values', () => { + const viewElement = viewParse( 'BarBom' ); + + expectResult( + viewElement, + '<$text fontSize="11">Bar<$text fontSize="9">Bom' + ); + } ); + + it( 'should convert texts before and after the nested duplicated attribute with the most outer value', () => { + const viewElement = viewParse( 'FooBarBom' ); + + expectResult( + viewElement, + '<$text fontSize="9">Foo<$text fontSize="11">Bar<$text fontSize="9">Bom' + ); + } ); + + it( 'should work with multiple duplicated attributes', () => { + const viewElement = viewParse( + 'Bar' + ); + + expectResult( + viewElement, + '<$text fontColor="#ff0000" fontSize="11">Bar' + ); + } ); + + it( 'should convert non-duplicated attributes from the most outer element', () => { + const viewElement = viewParse( + 'Bar' + ); + + expectResult( + viewElement, + '<$text fontColor="#0000ff" fontSize="11">Bar' + ); + } ); + + // See https://github.com/ckeditor/ckeditor5/pull/9249#issuecomment-813935851 + it( 'should consume both elements even if the attribute from the most inner element will be used', () => { + upcastDispatcher.on( 'element:span', ( evt, data, conversionApi ) => { + const viewItem = data.viewItem; + const wasConsumed = conversionApi.consumable.consume( viewItem, { + styles: [ 'font-size' ] + } ); + + expect( wasConsumed, `span[fontSize=${ viewItem.getStyle( 'font-size' ) }]` ).to.equal( false ); + }, { priority: 'lowest' } ); + + const viewElement = viewParse( + 'Bar' + ); + + expectResult( + viewElement, + '<$text fontSize="11">Bar' + ); + } ); + } ); } ); describe( 'attributeToAttribute()', () => { diff --git a/packages/ckeditor5-font/tests/fontcolor/fontcolorediting.js b/packages/ckeditor5-font/tests/fontcolor/fontcolorediting.js index 3ce37df8c90..c12fa27f3b4 100644 --- a/packages/ckeditor5-font/tests/fontcolor/fontcolorediting.js +++ b/packages/ckeditor5-font/tests/fontcolor/fontcolorediting.js @@ -343,5 +343,60 @@ describe( 'FontColorEditing', () => { '

foobaro

' ); } ); + + // #8921 + describe( 'nested elements that will be converted into the fontColor attribute', () => { + it( 'should use the most inner value while converting nested font elements', () => { + const data = '

foo

'; + + editor.setData( data ); + + expect( getModelData( doc ) ).to.equal( + '<$text fontColor="#ff0000">[]foo' + ); + expect( editor.getData() ).to.equal( + '

foo

' + ); + } ); + + it( 'should use the most inner value while converting a span with defined the color styles', () => { + const data = '

foo

'; + + editor.setData( data ); + + expect( getModelData( doc ) ).to.equal( + '<$text fontColor="#ff0000">[]foo' + ); + expect( editor.getData() ).to.equal( + '

foo

' + ); + } ); + + it( 'should use the most inner value while converting the font element inside a span', () => { + const data = '

foo

'; + + editor.setData( data ); + + expect( getModelData( doc ) ).to.equal( + '<$text fontColor="#00ffff">[]foo' + ); + expect( editor.getData() ).to.equal( + '

foo

' + ); + } ); + + it( 'should use the most inner value while converting nested spans with the color styles', () => { + const data = '

foo

'; + + editor.setData( data ); + + expect( getModelData( doc ) ).to.equal( + '<$text fontColor="#ff0000">[]foo' + ); + expect( editor.getData() ).to.equal( + '

foo

' + ); + } ); + } ); } ); } ); diff --git a/packages/ckeditor5-highlight/tests/highlightediting.js b/packages/ckeditor5-highlight/tests/highlightediting.js index 5d46aaa6371..4e4730facef 100644 --- a/packages/ckeditor5-highlight/tests/highlightediting.js +++ b/packages/ckeditor5-highlight/tests/highlightediting.js @@ -54,11 +54,12 @@ describe( 'HighlightEditing', () => { expect( editor.getData() ).to.equal( data ); } ); - it( 'should convert only one defined marker classes', () => { - editor.setData( '

foo

' ); + // After closing #8921, converted will be the last class in the alphabetical order that matches the configuration options. + it( 'should convert only one class even if the marker has a few of them', () => { + editor.setData( '

foo

' ); - expect( getModelData( model ) ).to.equal( '[]f<$text highlight="greenMarker">oo' ); - expect( editor.getData() ).to.equal( '

foo

' ); + expect( getModelData( model ) ).to.equal( '[]f<$text highlight="yellowMarker">oo' ); + expect( editor.getData() ).to.equal( '

foo

' ); } ); it( 'should not convert undefined marker classes', () => { diff --git a/packages/ckeditor5-language/tests/textpartlanguageediting.js b/packages/ckeditor5-language/tests/textpartlanguageediting.js index 0d3a829b11a..e30fb67fdb3 100644 --- a/packages/ckeditor5-language/tests/textpartlanguageediting.js +++ b/packages/ckeditor5-language/tests/textpartlanguageediting.js @@ -82,6 +82,21 @@ describe( 'TextPartLanguageEditing', () => { expect( editor.getData() ).to.equal( '

foobar

' ); } ); + + it( 'should respect nested element language ', () => { + editor.setData( '

hebrewfrenchhebrew

' ); + + expect( getModelData( model, { withoutSelection: true } ) ) + .to.equal( '' + + '<$text language="he:rtl">hebrew' + + '<$text language="fr:ltr">french' + + '<$text language="he:rtl">hebrew' ); + + expect( editor.getData() ).to.equal( '

' + + 'hebrew' + + 'french' + + 'hebrew

' ); + } ); } ); describe( 'editing pipeline conversion', () => {