Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#13886 closed Bug (fixed)

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

Reported by: Olek Nowodziński Owned by: kkrzton
Priority: Normal Milestone: CKEditor 4.5.9
Component: General Version: 4.5.5
Keywords: Drupal Cc: wim.leers@…

Description

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

  1. For a given filter
    var filter = new CKEDITOR.filter( 'a {color}' );
    
  2. Define a style with styles: {}. Like
    var style = new CKEDITOR.style( { element: 'a', styles: {} } )
    
  3. 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.

Change History (19)

comment:1 Changed 3 years ago by Olek Nowodziński

Pushed a test to branch:t/13886.

comment:2 Changed 3 years ago by Wim Leers

Cc: wim.leers@… added
Keywords: Drupal added

comment:3 Changed 3 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: newassigned

comment:4 Changed 3 years ago by Marek Lewandowski

Owner: Marek Lewandowski deleted
Status: assignedconfirmed

comment:5 Changed 3 years ago by Wim Leers

Overarching ticket: #13898.

comment:6 Changed 3 years ago by 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.

comment:7 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.5CKEditor 4.5.6

comment:8 Changed 3 years ago by kkrzton

Owner: set to kkrzton
Status: confirmedassigned

comment:9 Changed 3 years ago by kkrzton

Status: assignedreview

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.

comment:10 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.6CKEditor 4.5.7

comment:11 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.7CKEditor 4.5.8

comment:12 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.8CKEditor 4.5.9

comment:13 Changed 2 years ago by Marek Lewandowski

Status: reviewreview_failed

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.

comment:14 Changed 2 years ago by kkrzton

Status: review_failedassigned

comment:15 Changed 2 years ago by kkrzton

Status: assignedreview

Changes in t/13886.

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

comment:16 Changed 2 years ago by Marek Lewandowski

Status: reviewreview_passed

comment:17 Changed 2 years ago by Marek Lewandowski

Resolution: fixed
Status: review_passedclosed

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

Last edited 2 years ago by Marek Lewandowski (previous) (diff)

comment:18 Changed 2 years ago by 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

comment:19 Changed 2 years ago by Marek Lewandowski

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

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy