From 032f83f870075a5cae32eb5fa58839545d4959c5 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Fri, 12 Jan 2024 20:15:04 +0100 Subject: [PATCH 1/3] Experimental handling of style and class attribute conversion. --- .../src/conversion/downcasthelpers.ts | 37 +++++++++++++++---- .../src/conversion/upcasthelpers.ts | 5 +-- .../ckeditor5-engine/src/view/stylesmap.ts | 6 +-- 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/packages/ckeditor5-engine/src/conversion/downcasthelpers.ts b/packages/ckeditor5-engine/src/conversion/downcasthelpers.ts index 9b42a54ecc3..08894318a79 100644 --- a/packages/ckeditor5-engine/src/conversion/downcasthelpers.ts +++ b/packages/ckeditor5-engine/src/conversion/downcasthelpers.ts @@ -43,6 +43,7 @@ import type ViewElement from '../view/element.js'; import type ViewNode from '../view/node.js'; import type ViewPosition from '../view/position.js'; import type ViewRange from '../view/range.js'; +import StylesMap from '../view/stylesmap.js'; import type { default as Mapper, MapperModelToViewPositionEvent @@ -1664,16 +1665,26 @@ function changeAttribute( attributeCreator: AttributeCreatorFunction ) { // First remove the old attribute if there was one. if ( data.attributeOldValue !== null && oldAttribute ) { if ( oldAttribute.key == 'class' ) { - const classes = toArray( oldAttribute.value ); + const classes = typeof oldAttribute.value == 'string' ? oldAttribute.value.split( /\s+/ ) : oldAttribute.value; for ( const className of classes ) { viewWriter.removeClass( className, viewElement ); } } else if ( oldAttribute.key == 'style' ) { - const keys = Object.keys( oldAttribute.value ); + if ( typeof oldAttribute.value == 'string' ) { + const styles = new StylesMap( viewWriter.document.stylesProcessor ); - for ( const key of keys ) { - viewWriter.removeStyle( key, viewElement ); + styles.setTo( oldAttribute.value ); + + for ( const [ key ] of styles.getStylesEntries() ) { + viewWriter.removeStyle( key, viewElement ); + } + } else { + const keys = Object.keys( oldAttribute.value ); + + for ( const key of keys ) { + viewWriter.removeStyle( key, viewElement ); + } } } else { viewWriter.removeAttribute( oldAttribute.key, viewElement ); @@ -1683,16 +1694,26 @@ function changeAttribute( attributeCreator: AttributeCreatorFunction ) { // Then set the new attribute. if ( data.attributeNewValue !== null && newAttribute ) { if ( newAttribute.key == 'class' ) { - const classes = toArray( newAttribute.value ); + const classes = typeof newAttribute.value == 'string' ? newAttribute.value.split( /\s+/ ) : newAttribute.value; for ( const className of classes ) { viewWriter.addClass( className, viewElement ); } } else if ( newAttribute.key == 'style' ) { - const keys = Object.keys( newAttribute.value ); + if ( typeof newAttribute.value == 'string' ) { + const styles = new StylesMap( viewWriter.document.stylesProcessor ); - for ( const key of keys ) { - viewWriter.setStyle( key, ( newAttribute.value as Record )[ key ], viewElement ); + styles.setTo( newAttribute.value ); + + for ( const [ key, value ] of styles.getStylesEntries() ) { + viewWriter.setStyle( key, value, viewElement ); + } + } else { + const keys = Object.keys( newAttribute.value ); + + for ( const key of keys ) { + viewWriter.setStyle( key, ( newAttribute.value as Record )[ key ], viewElement ); + } } } else { viewWriter.setAttribute( newAttribute.key, newAttribute.value as string, viewElement ); diff --git a/packages/ckeditor5-engine/src/conversion/upcasthelpers.ts b/packages/ckeditor5-engine/src/conversion/upcasthelpers.ts index e3fc251639b..ff4eb8866f5 100644 --- a/packages/ckeditor5-engine/src/conversion/upcasthelpers.ts +++ b/packages/ckeditor5-engine/src/conversion/upcasthelpers.ts @@ -977,17 +977,16 @@ function normalizeViewAttributeKeyValueConfig( config: any ) { } const key: string = config.view.key; + const value = typeof config.view.value == 'undefined' ? /[\s\S]*/ : config.view.value; let normalized: MatcherPattern; if ( key == 'class' || key == 'style' ) { const keyName = key == 'class' ? 'classes' : 'styles'; normalized = { - [ keyName ]: config.view.value + [ keyName ]: value }; } else { - const value = typeof config.view.value == 'undefined' ? /[\s\S]*/ : config.view.value; - normalized = { attributes: { [ key ]: value diff --git a/packages/ckeditor5-engine/src/view/stylesmap.ts b/packages/ckeditor5-engine/src/view/stylesmap.ts index ef165f9ccc4..61ed4ab6203 100644 --- a/packages/ckeditor5-engine/src/view/stylesmap.ts +++ b/packages/ckeditor5-engine/src/view/stylesmap.ts @@ -303,7 +303,7 @@ export default class StylesMap { return ''; } - return this._getStylesEntries() + return this.getStylesEntries() .map( arr => arr.join( ':' ) ) .sort() .join( ';' ) + ';'; @@ -411,7 +411,7 @@ export default class StylesMap { return this._styleProcessor.getStyleNames( this._styles ); } - const entries = this._getStylesEntries(); + const entries = this.getStylesEntries(); return entries.map( ( [ key ] ) => key ); } @@ -426,7 +426,7 @@ export default class StylesMap { /** * Returns normalized styles entries for further processing. */ - private _getStylesEntries(): Array { + public getStylesEntries(): Array { const parsed: Array = []; const keys = Object.keys( this._styles ); From ee7d9f6a1ac3a15c9ea9c0c7fe391a1724a1aeda Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Sat, 13 Jan 2024 13:40:34 +0100 Subject: [PATCH 2/3] Added tests ensuring proper consumables are consumed for style and class attributes. --- .../tests/conversion/upcasthelpers.js | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/packages/ckeditor5-engine/tests/conversion/upcasthelpers.js b/packages/ckeditor5-engine/tests/conversion/upcasthelpers.js index 3a0affab20b..a0a705b0fd2 100644 --- a/packages/ckeditor5-engine/tests/conversion/upcasthelpers.js +++ b/packages/ckeditor5-engine/tests/conversion/upcasthelpers.js @@ -603,6 +603,64 @@ describe( 'UpcastHelpers', () => { ); } ); + it( 'config.view does not have value set for style key', () => { + schema.extend( 'imageBlock', { + allowAttributes: [ 'styled' ] + } ); + + upcastHelpers.attributeToAttribute( { + view: 'style', + model: 'styled' + } ); + + // Ensure that proper consumables are consumed. + upcastDispatcher.on( 'element', ( evt, data, { consumable } ) => { + expect( consumable.test( data.viewItem, { styles: [ 'border', 'padding' ] } ) ).to.be.true; + expect( consumable.test( data.viewItem, { styles: [ 'border' ] } ) ).to.be.true; + expect( consumable.test( data.viewItem, { styles: [ 'padding' ] } ) ).to.be.true; + }, { priority: 'highest' } ); + + upcastDispatcher.on( 'element', ( evt, data, { consumable } ) => { + expect( consumable.test( data.viewItem, { styles: [ 'border', 'padding' ] } ) ).to.be.false; + expect( consumable.test( data.viewItem, { styles: [ 'border' ] } ) ).to.be.false; + expect( consumable.test( data.viewItem, { styles: [ 'padding' ] } ) ).to.be.false; + }, { priority: 'lowest' } ); + + expectResult( + new ViewAttributeElement( viewDocument, 'img', { 'style': 'border: 2px solid red; padding: 6px 3px;' } ), + '' + ); + } ); + + it( 'config.view does not have value set for class key', () => { + schema.extend( 'imageBlock', { + allowAttributes: [ 'classNames' ] + } ); + + upcastHelpers.attributeToAttribute( { + view: 'class', + model: 'classNames' + } ); + + // Ensure that proper consumables are consumed. + upcastDispatcher.on( 'element', ( evt, data, { consumable } ) => { + expect( consumable.test( data.viewItem, { classes: [ 'foo', 'bar' ] } ) ).to.be.true; + expect( consumable.test( data.viewItem, { classes: [ 'foo' ] } ) ).to.be.true; + expect( consumable.test( data.viewItem, { classes: [ 'bar' ] } ) ).to.be.true; + }, { priority: 'highest' } ); + + upcastDispatcher.on( 'element', ( evt, data, { consumable } ) => { + expect( consumable.test( data.viewItem, { classes: [ 'foo', 'bar' ] } ) ).to.be.false; + expect( consumable.test( data.viewItem, { classes: [ 'foo' ] } ) ).to.be.false; + expect( consumable.test( data.viewItem, { classes: [ 'bar' ] } ) ).to.be.false; + }, { priority: 'lowest' } ); + + expectResult( + new ViewAttributeElement( viewDocument, 'img', { 'class': 'foo bar' } ), + '' + ); + } ); + it( 'model attribute value is a string', () => { schema.extend( 'imageBlock', { allowAttributes: [ 'styled' ] From 4b6ab878721cf607b2c802b766297b4959d5b182 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Sat, 13 Jan 2024 14:07:20 +0100 Subject: [PATCH 3/3] Added tests proper downcast of style and class attributes. --- .../tests/conversion/downcasthelpers.js | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/packages/ckeditor5-engine/tests/conversion/downcasthelpers.js b/packages/ckeditor5-engine/tests/conversion/downcasthelpers.js index 833ed2a635d..1218825f92f 100644 --- a/packages/ckeditor5-engine/tests/conversion/downcasthelpers.js +++ b/packages/ckeditor5-engine/tests/conversion/downcasthelpers.js @@ -3298,6 +3298,62 @@ describe( 'DowncastHelpers', () => { expect( viewToString( viewRoot ) ).to.equal( '

foobar

' ); } ); + it( 'config.view and config.model as strings (class attribute)', () => { + const consoleWarnStub = testUtils.sinon.stub( console, 'warn' ); + + downcastHelpers.elementToElement( { model: 'paragraph', view: 'p' } ); + downcastHelpers.attributeToAttribute( { model: 'test', view: 'class' } ); + + model.change( writer => { + writer.insertElement( 'paragraph', { test: 'foo bar' }, modelRoot, 0 ); + writer.insertElement( 'paragraph', { test: 'abc def' }, modelRoot, 1 ); + } ); + + expectResult( + '

' + + '

' + ); + expect( consoleWarnStub.callCount ).to.equal( 0 ); + + model.change( writer => { + writer.setAttribute( 'test', 'bar', modelRoot.getChild( 0 ) ); + writer.removeAttribute( 'test', modelRoot.getChild( 1 ) ); + } ); + + expectResult( + '

' + + '

' + ); + } ); + + it( 'config.view and config.model as strings (style attribute)', () => { + const consoleWarnStub = testUtils.sinon.stub( console, 'warn' ); + + downcastHelpers.elementToElement( { model: 'paragraph', view: 'p' } ); + downcastHelpers.attributeToAttribute( { model: 'test', view: 'style' } ); + + model.change( writer => { + writer.insertElement( 'paragraph', { test: 'background: red; padding: 4px 10px;' }, modelRoot, 0 ); + writer.insertElement( 'paragraph', { test: 'color: blue; background: yellow;' }, modelRoot, 1 ); + } ); + + expectResult( + '

' + + '

' + ); + expect( consoleWarnStub.callCount ).to.equal( 0 ); + + model.change( writer => { + writer.setAttribute( 'test', 'background: pink;', modelRoot.getChild( 0 ) ); + writer.removeAttribute( 'test', modelRoot.getChild( 1 ) ); + } ); + + expectResult( + '

' + + '

' + ); + } ); + it( 'should be possible to override setAttribute', () => { downcastHelpers.attributeToAttribute( { model: 'class',