ckeditor4 icon indicating copy to clipboard operation
ckeditor4 copied to clipboard

widget startupData object is not being used by widget template

Open spacevoyager78 opened this issue 5 years ago • 4 comments

Accorrding to the widget docs, I can use:

editor.execCommand( 'simplebox', {
    startupData: {
        align: 'left'
    }
} );

to create a widget with initial widget data and this data object will overwrite the default data.

However, this doesn't modify the widget template at all.

I have created an example here: http://users.sch.gr/vandr/ckeditor4/

I use unoptimized CKEditor 4.13.0 with simplebox widget. I have modified the simplebox widget by including "title" and "content" placeholders like so:

template:
    '<div class="simplebox">' +
    '<h2 class="simplebox-title">{title}</h2>' +
    '<div class="simplebox-content"><p>{content}</p></div>' +
    '</div>',

and I have registered another copy named simplebox2 widget with defaults object:

defaults: {
    title: 'default title',
    content: 'default content'
},

The plugin source is here: http://users.sch.gr/vandr/ckeditor4/plugins/simplebox/plugin.js I created CKEditor like this:

var editor1 = CKEDITOR.replace('editor1', {
	extraPlugins: 'simplebox',
	on: {
		instanceReady: function(evt) {
			evt.editor.execCommand('simplebox2', {
				startupData: {
					title: 'startupData title',
					content: 'startupData content'
				}
			});
		},
		save: function(evt) {
			evt.cancel();
			evt.editor.execCommand('simplebox', {
				startupData: {
					title: 'startupData title',
					content: 'startupData content'
				}
			});
		}
	}
});

Initially, simplebox2 widget is inserted (which has defaults object), but startupData is wrongly never used in the template and the defaults object is used. If you press Save to insert simplebox widget (which doesn't have defaults object), startupData is also wrongly not used and I get error:

template.js:64 Uncaught TypeError: Cannot read property 'title' of undefined at template.js:64 at String.replace () at CKEDITOR.template.output (template.js:63) at CKEDITOR.command.exec (plugin.js:1990) at CKEDITOR.command.exec (command.js:56) at Editor.execCommand (editor.js:982) at Editor.save ((index):22) at Editor.listenerFirer (event.js:144) at Editor.CKEDITOR.event.fire (event.js:290) at Editor.CKEDITOR.editor.fire (editor_basic.js:24)

So, I cannot use execCommand with startupData to create a widget and modify its template. I also found an old question in StackOverflow: https://stackoverflow.com/questions/32149518/ckeditor-widget-receives-data-after-it-has-been-rendered

spacevoyager78 avatar Oct 04 '19 17:10 spacevoyager78

I have found a solution, but I don't how to make a PR, so I'll put here, so someone could test it, please! Basically, in line 1990 from plugins\widget\plugin.js widgetDef.template.output( defaults ) has to be modified to widgetDef.template.output( { ...defaults, ...commandData.startupData } ) or widgetDef.template.output( Object.assign( {}, defaults, commandData.startupData ) ) or perhaps use some code to support Internet Explorer like this

var mergedData = Object( defaults );
for ( var key in commandData.startupData ) {
	if ( commandData.startupData.hasOwnProperty( key ) ) {
		mergedData[ key ] = commandData.startupData[ key ];
	}
}
widgetDef.template.output( mergedData )

In conclusion, the easy fix is to merge defaults object with commandData.startupData object, with the latter overriding the former, just before outputting the template..

spacevoyager78 avatar Oct 04 '19 18:10 spacevoyager78

I can reproduce the issue using your sample. To simplify it a bit, here are reproduction steps for the issue:

  1. Execute below code to initialize CKEditor:
CKEDITOR.plugins.add( 'simplebox', {
	requires: 'widget',
	init: function( editor ) {
		editor.widgets.add( 'simplebox', {
			allowedContent: 'div(!simplebox); div(!simplebox-content); h2(!simplebox-title)',

			requiredContent: 'div(simplebox)',

			editables: {
				title: {
					selector: '.simplebox-title',
					allowedContent: 'br strong em'
				},
				content: {
					selector: '.simplebox-content',
					allowedContent: 'p br ul ol li strong em'
				}
			},

			template:
			'<div class="simplebox">' +
			'<h2 class="simplebox-title">{title}</h2>' +
			'<div class="simplebox-content"><p>{content}</p></div>' +
			'</div>',

			upcast: function( element ) {
				return element.name == 'div' && element.hasClass( 'simplebox' );
			},

			defaults: {
				title: 'default title',
				content: 'default content'
			},
		} );
	}
} );

CKEDITOR.replace( 'editor', {
	extraPlugins: 'simplebox'
} );
  1. Focus the editor.
  2. Execute
editor.execCommand( 'simplebox', {
  startupData: {
    title: 'updated title',
    content: 'updated content'
  }
} );

Expected

Widget should be initialized with updated placeholders i.e updated title and updated content

Actual

Widget is initialized with default values.

Overall, there is no strict information in our docs that widgetDef.startupData will be used to populate the template. However, as widgetDef.defaults is used for this purpose and there is information about higher startupData priority over defaults, it probably makes sense to unify this behavior.

However, it seems for me more like a feature than a bug as there is no well-written specification for that (at least I couldn't find) and the whole concept is rather based on assumptions than docs.

jacekbogdanski avatar Oct 07 '19 09:10 jacekbogdanski

Yes, you are right, there is no direct information that startupData is used in the template. It's what I inferred, based on the fact that the template uses default data and startupData overrides it. I think it's very useful to have it and the fix seems (to me at least) rather easy. Then again, I can't speak for the whole internal structure of widget plugin, that's why I believe it's best to leave it for the devs. Temporarily, I'm using my fix and I'm happy with it.

spacevoyager78 avatar Oct 07 '19 11:10 spacevoyager78

I realize this ticket is a bit old now, but, is there a solid work-around that doesn't involve the patch?

bricas avatar Sep 15 '22 19:09 bricas