Skip to content

Commit

Permalink
Merge pull request #9249 from ckeditor/i/8921
Browse files Browse the repository at this point in the history 
Fix (engine): While setting attributes upon upcast conversion, don't override attributes that were already set. The correct behavior is to keep the "information" applied by the deepest nodes in the view tree as, in most cases, the deepest node will have precedence (e.g. inline style applied by the deepest node). Closes #8921.
  • Loading branch information
@jacekbogdanski
jacekbogdanski authored Apr 9, 2021
2 parents 3ad2aa5 + 1dfb0d0 commit 9a819fe
Show file tree
Hide file tree
Showing 6 changed files with 258 additions and 9 deletions.
45 changes: 45 additions & 0 deletions packages/ckeditor5-alignment/tests/integration.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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( '<div style="text-align: right;">foo</div>' );

// 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( '<div customAlignment="right">foo</div>' );
expect( editor.getData() ).to.equal( '<div style="text-align:right;">foo</div>' );
} );
} );
} );
24 changes: 19 additions & 5 deletions packages/ckeditor5-engine/src/conversion/upcasthelpers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) );
Expand All @@ -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 );
}
Expand All @@ -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.
Expand All @@ -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;
Expand Down
119 changes: 119 additions & 0 deletions packages/ckeditor5-engine/tests/conversion/upcasthelpers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,125 @@ describe( 'UpcastHelpers', () => {
'<paragraph><$text bold="true">Foo</$text></paragraph>'
);
} );

// #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( '<span style="font-size:9px"><span style="font-size:11px">Bar</span></span>' );

expectResult(
viewElement,
'<$text fontSize="11">Bar</$text>'
);
} );

it( 'should convert text before the nested duplicated attribute with the most outer value', () => {
const viewElement = viewParse( '<span style="font-size:9px">Foo<span style="font-size:11px">Bar</span></span>' );

expectResult(
viewElement,
'<$text fontSize="9">Foo</$text><$text fontSize="11">Bar</$text>'
);
} );

it( 'should convert text after the nested duplicated attribute with the most outer values', () => {
const viewElement = viewParse( '<span style="font-size:9px"><span style="font-size:11px">Bar</span>Bom</span>' );

expectResult(
viewElement,
'<$text fontSize="11">Bar</$text><$text fontSize="9">Bom</$text>'
);
} );

it( 'should convert texts before and after the nested duplicated attribute with the most outer value', () => {
const viewElement = viewParse( '<span style="font-size:9px">Foo<span style="font-size:11px">Bar</span>Bom</span>' );

expectResult(
viewElement,
'<$text fontSize="9">Foo</$text><$text fontSize="11">Bar</$text><$text fontSize="9">Bom</$text>'
);
} );

it( 'should work with multiple duplicated attributes', () => {
const viewElement = viewParse(
'<span style="font-size:9px;color: #0000ff"><span style="font-size:11px;color: #ff0000">Bar</span></span>'
);

expectResult(
viewElement,
'<$text fontColor="#ff0000" fontSize="11">Bar</$text>'
);
} );

it( 'should convert non-duplicated attributes from the most outer element', () => {
const viewElement = viewParse(
'<span style="font-size:9px;color: #0000ff"><span style="font-size:11px;">Bar</span></span>'
);

expectResult(
viewElement,
'<$text fontColor="#0000ff" fontSize="11">Bar</$text>'
);
} );

// 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(
'<span style="font-size:9px;"><span style="font-size:11px;">Bar</span></span>'
);

expectResult(
viewElement,
'<$text fontSize="11">Bar</$text>'
);
} );
} );
} );

describe( 'attributeToAttribute()', () => {
Expand Down
55 changes: 55 additions & 0 deletions packages/ckeditor5-font/tests/fontcolor/fontcolorediting.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -343,5 +343,60 @@ describe( 'FontColorEditing', () => {
'<p>foo<span style="color:rgb(10,20,30);">bar</span>o</p>'
);
} );

// #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 = '<p><font color="#00ffff"><font color="#ff0000">foo</font></font></p>';

editor.setData( data );

expect( getModelData( doc ) ).to.equal(
'<paragraph><$text fontColor="#ff0000">[]foo</$text></paragraph>'
);
expect( editor.getData() ).to.equal(
'<p><span style="color:#ff0000;">foo</span></p>'
);
} );

it( 'should use the most inner value while converting a span with defined the color styles', () => {
const data = '<p><font color="#00ffff"><span style="color:#ff0000">foo</span></font></p>';

editor.setData( data );

expect( getModelData( doc ) ).to.equal(
'<paragraph><$text fontColor="#ff0000">[]foo</$text></paragraph>'
);
expect( editor.getData() ).to.equal(
'<p><span style="color:#ff0000;">foo</span></p>'
);
} );

it( 'should use the most inner value while converting the font element inside a span', () => {
const data = '<p><span style="color:#ff0000"><font color="#00ffff">foo</font></span></p>';

editor.setData( data );

expect( getModelData( doc ) ).to.equal(
'<paragraph><$text fontColor="#00ffff">[]foo</$text></paragraph>'
);
expect( editor.getData() ).to.equal(
'<p><span style="color:#00ffff;">foo</span></p>'
);
} );

it( 'should use the most inner value while converting nested spans with the color styles', () => {
const data = '<p><span style="color:#00ffff"><span style="color:#ff0000">foo</span></span></p>';

editor.setData( data );

expect( getModelData( doc ) ).to.equal(
'<paragraph><$text fontColor="#ff0000">[]foo</$text></paragraph>'
);
expect( editor.getData() ).to.equal(
'<p><span style="color:#ff0000;">foo</span></p>'
);
} );
} );
} );
} );
9 changes: 5 additions & 4 deletions packages/ckeditor5-highlight/tests/highlightediting.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,12 @@ describe( 'HighlightEditing', () => {
expect( editor.getData() ).to.equal( data );
} );

it( 'should convert only one defined marker classes', () => {
editor.setData( '<p>f<mark class="marker-green marker-yellow">o</mark>o</p>' );
// 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( '<p>f<mark class="marker-yellow marker-green">o</mark>o</p>' );

expect( getModelData( model ) ).to.equal( '<paragraph>[]f<$text highlight="greenMarker">o</$text>o</paragraph>' );
expect( editor.getData() ).to.equal( '<p>f<mark class="marker-green">o</mark>o</p>' );
expect( getModelData( model ) ).to.equal( '<paragraph>[]f<$text highlight="yellowMarker">o</$text>o</paragraph>' );
expect( editor.getData() ).to.equal( '<p>f<mark class="marker-yellow">o</mark>o</p>' );
} );

it( 'should not convert undefined marker classes', () => {
Expand Down
15 changes: 15 additions & 0 deletions packages/ckeditor5-language/tests/textpartlanguageediting.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,21 @@ describe( 'TextPartLanguageEditing', () => {

expect( editor.getData() ).to.equal( '<p><span lang="fr" dir="ltr">foo</span>bar</p>' );
} );

it( 'should respect nested element language ', () => {
editor.setData( '<p><span dir="rtl" lang="he">hebrew<span dir="ltr" lang="fr">french</span>hebrew</span></p>' );

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

expect( editor.getData() ).to.equal( '<p>' +
'<span lang="he" dir="rtl">hebrew</span>' +
'<span lang="fr" dir="ltr">french</span>' +
'<span lang="he" dir="rtl">hebrew</span></p>' );
} );
} );

describe( 'editing pipeline conversion', () => {
Expand Down

0 comments on commit 9a819fe

Please sign in to comment.