Invalid handling of CKEDITOR.style instance with "styles" property by CKEDITOR.filter

[Olek Nowodziński]

A followup of ​https://www.drupal.org/node/2598070.

Revealed by ​http://cgit.drupalcode.org/drupal/commit/?id=cea9c14, which was meant to fix ​https://www.drupal.org/node/2589779.

Steps to reproduce

0. For a given filter

   var filter = new CKEDITOR.filter( 'a {color}' );
   

1. Define a style with styles: {}. Like

   var style = new CKEDITOR.style( { element: 'a', styles: {} } )
   

2. Check in in the filter.

   filter.check( style );
   

Expected result

Check passes.

Actual result

Check fails. But if defined a key in styles it will pass.

var style = new CKEDITOR.style( { element: 'a', styles: { color: 'red' } } );
filter.check( style ) -> true.

Also if removed styles key from definition it will pass.

var style = new CKEDITOR.style( { element: 'a' } );
filter.check( style ) -> true.

Ideas

Something could be wrong with ​https://github.com/ckeditor/ckeditor-dev/blob/7856fe93f844be5053fa62cad1ece1ba6fc8635b/core/filter.js#L996-L1017.

[Olek Nowodziński]

Pushed a test to ​branch:t/13886.

[Wim Leers]

Overarching ticket: #13898.

[Wim Leers]

If my lengthy explanation in ​https://www.drupal.org/node/2598070#comment-10523740 stands up to scrutiny, we won't be needing this fix in a hurry (because it won't be causing a huge bug in Drupal 8 anymore). Rather, it'd make more sense to look at this as just one part of #13898.

[kkrzton]

Changes in ​t/13886.

var style = new CKEDITOR.style( { element: 'a' } )
var style = new CKEDITOR.style( { element: 'a', styles: {} } )

Empty styles attr is removed during CKEDITOR.style instance creation. Both lines above behaves the same, creating equal instances.

[Marek Lewandowski]

Branch rebased on current master.

There's an issue with the current implementation, that the client-provided definition will not be preserved (styles property might get removed, even though explicitly added).

This will cause a regression in following case:

// Plugin A defines a style which is about to be applied. This style could get modified by other plugins using event.

var newStyle = new CKEditor.style( {
		element: 'h1',
		styles: {}
	} );

if ( plugin.fire( 'applyStyle' ) !== false ) {
	editor.applyStyle( newStyle );
}

// Plugin B listens on this event and modifies the definition.

editor.pluginA.on( 'applyStyle', function( evt ) {
	evt.data.getDefinition().styles.color = 'red';
} );

// Plugin C also listens to this event.

editor.pluginA.on( 'applyStyle', function( evt ) {
	evt.data.getDefinition().styles[ 'font-size' ] = '24px';
} );

In order to avoid breaking code it would require client code to do an extra check, like so:

editor.pluginA.on( 'applyStyle', function( evt ) {
	var def = evt.data.getDefinition();

	if ( def.styles ) {
		def.styles[ 'font-size' ] = '24px';
	} else {
		def.styles = {
			'font-size': = '24px'
		};
	}
} );

Could we improve it in a way so that it doesn't modify the definition? Fixing ​style if in filter.js mockElementFromStyle seems to do the trick.

[kkrzton]

Changes in ​t/13886.

Fixed filter.js mockElementFromStyle method to properly handle empty styles.

[Marek Lewandowski]

Fixed with ​git:91346ef (merged to master).

[Wim Leers]

Yay!

Would it be worth for Drupal to add back the styles: {} declarations it removed? See ​https://www.drupal.org/node/2598070#comment-11047329

[Marek Lewandowski]

@Wim Leers it's really up to you what's more comfortable with you at this point :)