ckeditor / ckeditor4

The best enterprise-grade WYSIWYG editor. Fully customizable with countless features and plugins.
https://ckeditor.com/ckeditor-4
Other
5.79k stars 2.47k forks source link

possible ckeditor bug in appendTimestamp starting 4.17.2 #5142

Closed mason88 closed 2 years ago

mason88 commented 2 years ago

Type of report

Bug

Description

Starting version 4.17.2, adding strinsert plugin causes bug in ckeditor.js with messages:

Uncaught TypeError: a.charAt is not a function

The strinsert plugin seems to load fine into ckeditor, and this message only shows up when clicking on the button created by the plugin.

I was able to work around this bug with following additional line to the core/ckeditor_base.js:

appendTimestamp: function( resource ) {
    if ( !this.timestamp ||
        (typeof resource != 'string') ||  <--- added this line
        resource.charAt( resource.length - 1 ) === '/' ||
        ( /[&?]t=/ ).test( resource )
    ) {

I'm not certain if this is a bug in ckeditor itself and could possibly affect other plugins. Please review and update as necessary.

Thank you.

Other details

sculpt0r commented 2 years ago

Hi @mason88, The plugin you are referring to is a third-party one, and it is not maintained by CKSource. Since we cannot fix issues with the code that does not belong to us, we are unable to help solve your issue. Please redirect your question to the plugin’s author who should be able to review the problem and suggest a solution.

If, however, after contacting the plugin’s maintainer you find out the issue lies on the editor’s side, and you can provide a test case file showing the problem, we will be happy to have a look at it.

gemal commented 2 years ago

I'm having the exact same problem. It's my own plugin that stopped working in 4.17.2 it works fine in 4.17.1

gemal commented 2 years ago

Tracked it down to using this key in the "panel" of the editor.ui.addRichCombo css: [config.contentsCss, CKEDITOR.getUrl(CKEDITOR.skin.getPath('editor') + 'editor.css')],

gemal commented 2 years ago

changing it to: css: [ CKEDITOR.skin.getPath( 'editor' ) ].concat( config.contentsCss ),

seems to make it work

jacekbogdanski commented 2 years ago

Thanks, @gemal for that finding. I've checked this 3rd party plugin strinsert to make sure we didn't introduce a regression as the appendTimestamp function has been changed in the 4.17.2 release (but that change shouldn't cause a regression). However, looking at the code of this plugin, it incorrectly concats rich combo CSS styles by calling:

editor.ui.addRichCombo('strinsert', {
  // (...)
  css: [ editor.config.contentsCss, CKEDITOR.skin.getPath('editor') ],
  // (...)
}

The produced css property will look like: [ 'some/url/to/contentsCss', [ 'another/url' ] ] but it's expected that this property accepts only a flat array type (Array<string>). You can see very similar examples in a core editor codebase where this method is used, and the CSS property is properly concatenated:

https://github.com/ckeditor/ckeditor4/blob/19678ef913d2897e01b8642870fce556be42ea28/plugins/font/plugin.js#L199

I have to admit that this addRichCombo method is poorly documented (I will request a ticket for that) but the issue is caused due to incorrect method usage.