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

CKBox plugin order matters, and it shouldn't #15581

Closed
Witoso opened this issue Dec 21, 2023 · 0 comments Β· Fixed by #15898
Closed

CKBox plugin order matters, and it shouldn't #15581

Witoso opened this issue Dec 21, 2023 · 0 comments Β· Fixed by #15898
Assignees
Labels
package:ckbox type:bug This issue reports a buggy (incorrect) behavior.

Comments

@Witoso
Copy link
Member

Witoso commented Dec 21, 2023

πŸ“ Provide detailed reproduction steps (if any)

  1. Add the CKBox plugin before the Β ImageBlock and ImageInline plugins.
  2. Change image from inline to block (or reverse)

βœ”οΈ Expected result

The ckboxImageId attribute stays.

❌ Actual result

The ckboxImageId is dropped.

❓ Possible solution

Discovered in docs. The reason for this is that in the build-classic.js snippet we add CKBox before the Image:

CKBox,
CloudServices,
EasyImage,
Heading,
Image,

Because of that, when schema in CKBox is being registered, the ImageBlock and ImageInline are not loaded yet. That's why in the code below the attribute ckboxImageId is not registered for images.

if ( schema.isRegistered( 'imageBlock' ) ) {
schema.extend( 'imageBlock', { allowAttributes: [ 'ckboxImageId', 'ckboxLinkId' ] } );
}
if ( schema.isRegistered( 'imageInline' ) ) {
schema.extend( 'imageInline', { allowAttributes: [ 'ckboxImageId', 'ckboxLinkId' ] } );
}

There is a similar problem with usage CKBox and BlockToolbar, the order influenced loading. Discussed internally:

Why CKBoxUI registers the ckbox button in the component factory upon afterInit()? There is no comment. There are no tests that verify that. What was the reason? (it causes problems with plugin initialisation and block toolbar)

(...) I believe, we used afterInit() due to

// Do not register the `ckbox` button if the command does not exist. if ( !editor.commands.get( 'ckbox' ) ) { return; }

What if there was a helper like
Β 

function canCKBoxWork() {
    const hasConfiguration = !!editor.config.get( 'ckbox' );
    const isLibraryLoaded = !!window.CKBox;
    return hasConfiguration && isLibraryLoaded
}

and both editing and UI used it on init?

But this

πŸ“ƒ Other details

  • Browser: …
  • OS: …
  • First affected CKEditor version: …
  • Installed CKEditor plugins: …

If you'd like to see this fixed sooner, add a πŸ‘ reaction to this post.

@Witoso Witoso added type:bug This issue reports a buggy (incorrect) behavior. package:ckbox labels Dec 21, 2023
@mmotyczynska mmotyczynska self-assigned this Feb 19, 2024
@CKEditorBot CKEditorBot added the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Feb 19, 2024
niegowski added a commit that referenced this issue Feb 26, 2024
Fix (ckbox): Plugin order should not matter when it comes to registering schema for `ckboxImageId` attribute. Closes #15581.

Fix (ui): BlockToolbar and BalloonToolbar plugins order should not matter when it comes to registering toolbar items. Closes #15581.

MINOR BREAKING CHANGE (ui): The contents of the `BlockToolbar` and `BalloonToolbar` are now filled on the `EditorUIReadyEvent` instead of `afterInit()`.
@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 26, 2024
@CKEditorBot CKEditorBot added this to the iteration 72 milestone Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:ckbox type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
3 participants