Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The default configuration options for ImageStyle plugin are not properly cloned #11328

Closed
psmyrek opened this issue Feb 22, 2022 · 4 comments · Fixed by #11338
Closed

The default configuration options for ImageStyle plugin are not properly cloned #11328

psmyrek opened this issue Feb 22, 2022 · 4 comments · Fixed by #11338
Assignees
Labels
package:image squad:platform Issue to be handled by the Platform team. type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.

Comments

@psmyrek
Copy link
Contributor

psmyrek commented Feb 22, 2022

📝 Provide detailed reproduction steps (if any)

It's difficult for me right now to prepare a few short steps to reproduce this bug, so let me just describe the problem.

The problem occurred during updating CKE5-related dependencies in the ckeditor5-angular repository, because one test started to fail in this line: https://github.com/ckeditor/ckeditor5-angular/blob/master/src/ckeditor/ckeditor.component.spec.ts#L324.

This test fails because unexpectedly the second component in this test also started to emit an error, but it shouldn't. The reason for this is that since CKE5 v29 we have an ImageInline plugin and the default options for ImageStyle have changed. It looks like they are not properly cloned when they are created, but they share the same arrays as modelElements in each editor, i.e.

modelElements: [ 'imageInline' ],

✔️ Expected result

Editors do not share common properties.

❌ Actual result

Editors share common properties from ImageStyle plugin.

❓ Possible solution

Make sure that default options are properly cloned.

📃 Other details

  • First affected CKEditor version: v29

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@psmyrek psmyrek added type:bug This issue reports a buggy (incorrect) behavior. package:image type:regression This issue reports a bug that was not present in the previous releases. squad:platform Issue to be handled by the Platform team. labels Feb 22, 2022
@pomek
Copy link
Member

pomek commented Feb 23, 2022

A similar problem we had in the font-size feature:

get tiny() {
return {
title: 'Tiny',
model: 'tiny',
view: {
name: 'span',
classes: 'text-tiny',
priority: 7
}
};
},
.

Scope:

  • Add getters for default options.
  • Write tests that cover the change (one for change in utils, one integration)

@CKEditorBot CKEditorBot added status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. labels Feb 24, 2022
@Reinmar
Copy link
Member

Reinmar commented Feb 24, 2022

We'd need a better description of what's wrong from the editor's perspective (in the editor's code). This is, extending this:

It looks like they are not properly cloned when they are created, but they share the same arrays as modelElements in each editor, i.e.

@Reinmar
Copy link
Member

Reinmar commented Feb 24, 2022

Actually, what's needed is – how to crash the editor without Angular. Naturally, Angular uses its API, but then how it uses its API to make it crash.

@psmyrek
Copy link
Contributor Author

psmyrek commented Feb 24, 2022

I probably did not describe the problem precisely.

The problem is not that the editor crashes. The problem is that if we have a context watchdog that watches over at least 2 editors, then an error in one of them causes that all of them are restarted (not as expected). The necessary condition for such incorrect reboots to take place is that each of these editors must have Image and ImageStyle plugins. The ImageStyle plugin shares some of its default options among all editor instances, which makes all editors connected together.

pomek added a commit that referenced this issue Feb 25, 2022
Fix (image): Always create new instances of the default options for the `ImageStyle` plugin. Closes #11328.
@pomek pomek reopened this Feb 25, 2022
@pomek pomek closed this as completed Feb 25, 2022
@pomek pomek added this to the iteration 51 milestone Feb 25, 2022
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:image squad:platform Issue to be handled by the Platform team. type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants