Skip to content

Commit

Permalink
Merge pull request #15898 from ckeditor/ck/15581
Browse files Browse the repository at this point in the history 
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()`.
  • Loading branch information
@niegowski
niegowski authored Feb 26, 2024
2 parents a793711 + 587921e commit 38ff3f1
Show file tree
Hide file tree
Showing 12 changed files with 222 additions and 64 deletions.
43 changes: 34 additions & 9 deletions packages/ckeditor5-ckbox/src/ckboxediting.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,29 +62,47 @@ export default class CKBoxEditing extends Plugin {
*/
public init(): void {
const editor = this.editor;
const hasConfiguration = !!editor.config.get( 'ckbox' );
const isLibraryLoaded = !!window.CKBox;

// Proceed with plugin initialization only when the integrator intentionally wants to use it, i.e. when the `config.ckbox` exists or
// the CKBox JavaScript library is loaded.
if ( !hasConfiguration && !isLibraryLoaded ) {
if ( !this._shouldBeInitialised() ) {
return;
}

this._checkImagePlugins();

// Registering the `ckbox` command makes sense only if the CKBox library is loaded, as the `ckbox` command opens the CKBox dialog.
if ( isLibraryLoaded() ) {
editor.commands.add( 'ckbox', new CKBoxCommand( editor ) );
}
}

/**
* @inheritdoc
*/
public afterInit(): void {
const editor = this.editor;

if ( !this._shouldBeInitialised() ) {
return;
}

// Extending the schema, registering converters and applying fixers only make sense if the configuration option to assign
// the assets ID with the model elements is enabled.
if ( !editor.config.get( 'ckbox.ignoreDataId' ) ) {
this._initSchema();
this._initConversion();
this._initFixers();
}
}

// Registering the `ckbox` command makes sense only if the CKBox library is loaded, as the `ckbox` command opens the CKBox dialog.
if ( isLibraryLoaded ) {
editor.commands.add( 'ckbox', new CKBoxCommand( editor ) );
}
/**
* Returns true only when the integrator intentionally wants to use the plugin, i.e. when the `config.ckbox` exists or
* the CKBox JavaScript library is loaded.
*/
private _shouldBeInitialised(): boolean {
const editor = this.editor;
const hasConfiguration = !!editor.config.get( 'ckbox' );

return hasConfiguration || isLibraryLoaded();
}

/**
Expand Down Expand Up @@ -421,3 +439,10 @@ function shouldUpcastAttributeForNode( node: Node ) {

return false;
}

/**
* Returns true if the CKBox library is loaded, false otherwise.
*/
function isLibraryLoaded(): boolean {
return !!window.CKBox;
}
8 changes: 4 additions & 4 deletions packages/ckeditor5-ckbox/src/ckboxui.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,17 @@ export default class CKBoxUI extends Plugin {
public afterInit(): void {
const editor = this.editor;

const command: CKBoxCommand | undefined = editor.commands.get( 'ckbox' );

// Do not register the `ckbox` button if the command does not exist.
if ( !command ) {
// This might happen when CKBox library is not loaded on the page.
if ( !editor.commands.get( 'ckbox' ) ) {
return;
}

const t = editor.t;
const componentFactory = editor.ui.componentFactory;

componentFactory.add( 'ckbox', locale => {
const command: CKBoxCommand = editor.commands.get( 'ckbox' )!;
const button = new ButtonView( locale );

button.set( {
Expand All @@ -64,7 +64,7 @@ export default class CKBoxUI extends Plugin {

imageInsertUI.registerIntegration( {
name: 'assetManager',
observable: command,
observable: () => editor.commands.get( 'ckbox' )!,

buttonViewCreator: () => {
const button = this.editor.ui.componentFactory.create( 'ckbox' ) as ButtonView;
Expand Down
67 changes: 52 additions & 15 deletions packages/ckeditor5-ckbox/tests/ckboxediting.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,36 @@ describe( 'CKBoxEditing', () => {

await editor.destroy();
} );

describe( 'CKBox loaded before the ImageBlock and ImageInline plugins', () => {
let editor, model, originalCKBox;

beforeEach( async () => {
TokenMock.initialToken = 'ckbox-token';

originalCKBox = window.CKBox;
window.CKBox = {};

editor = await createTestEditor( {
ckbox: {
tokenUrl: 'http://cs.example.com'
}
}, true );

model = editor.model;
} );

afterEach( async () => {
window.CKBox = originalCKBox;
await editor.destroy();
} );

// https://github.com/ckeditor/ckeditor5/issues/15581
it( 'should extend the schema rules for imageBlock and imageInline', () => {
expect( model.schema.checkAttribute( [ '$root', 'imageBlock' ], 'ckboxImageId' ) ).to.be.true;
expect( model.schema.checkAttribute( [ '$root', '$block', 'imageInline' ], 'ckboxImageId' ) ).to.be.true;
} );
} );
} );

describe( 'conversion', () => {
Expand Down Expand Up @@ -1802,22 +1832,29 @@ describe( 'CKBoxEditing', () => {
} );
} );

function createTestEditor( config = {} ) {
function createTestEditor( config = {}, loadCKBoxFirst = false ) {
const plugins = [
Paragraph,
ImageBlockEditing,
ImageInlineEditing,
ImageCaptionEditing,
LinkEditing,
LinkImageEditing,
PictureEditing,
ImageUploadEditing,
ImageUploadProgress,
CloudServices,
CKBoxUploadAdapter
];

if ( loadCKBoxFirst ) {
plugins.unshift( CKBoxEditing );
} else {
plugins.push( CKBoxEditing );
}

return VirtualTestEditor.create( {
plugins: [
Paragraph,
ImageBlockEditing,
ImageInlineEditing,
ImageCaptionEditing,
LinkEditing,
LinkImageEditing,
PictureEditing,
ImageUploadEditing,
ImageUploadProgress,
CloudServices,
CKBoxUploadAdapter,
CKBoxEditing
],
plugins,
substitutePlugins: [
CloudServicesCoreMock
],
Expand Down
3 changes: 1 addition & 2 deletions packages/ckeditor5-ckfinder/src/ckfinderui.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,10 @@ export default class CKFinderUI extends Plugin {

if ( editor.plugins.has( 'ImageInsertUI' ) ) {
const imageInsertUI: ImageInsertUI = editor.plugins.get( 'ImageInsertUI' );
const command: CKFinderCommand = editor.commands.get( 'ckfinder' )!;

imageInsertUI.registerIntegration( {
name: 'assetManager',
observable: command,
observable: () => editor.commands.get( 'ckfinder' )!,

buttonViewCreator: () => {
const button = this.editor.ui.componentFactory.create( 'ckfinder' ) as ButtonView;
Expand Down
8 changes: 4 additions & 4 deletions packages/ckeditor5-image/src/imageinsert/imageinsertui.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,11 @@ export default class ImageInsertUI extends Plugin {
requiresForm
}: {
name: string;
observable: Observable & { isEnabled: boolean };
observable: Observable & { isEnabled: boolean } | ( () => Observable & { isEnabled: boolean } );
buttonViewCreator: ( isOnlyOne: boolean ) => ButtonView;
formViewCreator: ( isOnlyOne: boolean ) => FocusableView;
requiresForm?: boolean;
} ): void {
} ): void {
if ( this._integrations.has( name ) ) {
/**
* There are two insert-image integrations registered with the same name.
Expand Down Expand Up @@ -174,7 +174,7 @@ export default class ImageInsertUI extends Plugin {
}

const dropdownView = this.dropdownView = createDropdown( locale, dropdownButton );
const observables = integrations.map( ( { observable } ) => observable );
const observables = integrations.map( ( { observable } ) => typeof observable == 'function' ? observable() : observable );

dropdownView.bind( 'isEnabled' ).toMany( observables, 'isEnabled', ( ...isEnabled ) => (
isEnabled.some( isEnabled => isEnabled )
Expand Down Expand Up @@ -250,7 +250,7 @@ export default class ImageInsertUI extends Plugin {
}

type IntegrationData = {
observable: Observable & { isEnabled: boolean };
observable: Observable & { isEnabled: boolean } | ( () => Observable & { isEnabled: boolean } );
buttonViewCreator: ( isOnlyOne: boolean ) => ButtonView;
formViewCreator: ( isOnlyOne: boolean ) => FocusableView;
requiresForm: boolean;
Expand Down
3 changes: 1 addition & 2 deletions packages/ckeditor5-image/src/imageinsert/imageinsertviaurlui.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,10 @@ export default class ImageInsertViaUrlUI extends Plugin {
*/
public afterInit(): void {
this._imageInsertUI = this.editor.plugins.get( 'ImageInsertUI' );
const insertImageCommand: InsertImageCommand = this.editor.commands.get( 'insertImage' )!;

this._imageInsertUI.registerIntegration( {
name: 'url',
observable: insertImageCommand,
observable: () => this.editor.commands.get( 'insertImage' )!,
requiresForm: true,
buttonViewCreator: isOnlyOne => this._createInsertUrlButton( isOnlyOne ),
formViewCreator: isOnlyOne => this._createInsertUrlView( isOnlyOne )
Expand Down
3 changes: 1 addition & 2 deletions packages/ckeditor5-image/src/imageupload/imageuploadui.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,10 @@ export default class ImageUploadUI extends Plugin {

if ( editor.plugins.has( 'ImageInsertUI' ) ) {
const imageInsertUI: ImageInsertUI = editor.plugins.get( 'ImageInsertUI' );
const command: UploadImageCommand = editor.commands.get( 'uploadImage' )!;

imageInsertUI.registerIntegration( {
name: 'upload',
observable: command,
observable: () => editor.commands.get( 'uploadImage' )!,

buttonViewCreator: () => {
const uploadImageButton = editor.ui.componentFactory.create( 'uploadImage' ) as FileDialogButtonView;
Expand Down
51 changes: 47 additions & 4 deletions packages/ckeditor5-image/tests/imageinsert/imageinsertui.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,22 @@ describe( 'ImageInsertUI', () => {
} );
} );

describe( 'single integration with form view required and observalbe as a function', () => {
beforeEach( async () => {
registerUrlIntegration( true );
} );

it( 'should bind isEnabled state to observable', () => {
const dropdown = editor.ui.componentFactory.create( 'insertImage' );

observableUrl.isEnabled = false;
expect( dropdown.isEnabled ).to.be.false;

observableUrl.isEnabled = true;
expect( dropdown.isEnabled ).to.be.true;
} );
} );

describe( 'multiple integrations', () => {
beforeEach( async () => {
registerUploadIntegration();
Expand Down Expand Up @@ -365,12 +381,39 @@ describe( 'ImageInsertUI', () => {
} );
} );

function registerUrlIntegration() {
describe( 'multiple integrations and observalbe as a function', () => {
beforeEach( async () => {
registerUploadIntegration( true );
registerUrlIntegration( true );
} );

it( 'should bind isEnabled state to observables', () => {
const dropdown = editor.ui.componentFactory.create( 'insertImage' );

observableUrl.isEnabled = false;
observableUpload.isEnabled = false;
expect( dropdown.isEnabled ).to.be.false;

observableUrl.isEnabled = true;
observableUpload.isEnabled = false;
expect( dropdown.isEnabled ).to.be.true;

observableUrl.isEnabled = false;
observableUpload.isEnabled = true;
expect( dropdown.isEnabled ).to.be.true;

observableUrl.isEnabled = true;
observableUpload.isEnabled = true;
expect( dropdown.isEnabled ).to.be.true;
} );
} );

function registerUrlIntegration( observableAsFunc ) {
observableUrl = new Model( { isEnabled: true } );

insertImageUI.registerIntegration( {
name: 'url',
observable: observableUrl,
observable: observableAsFunc ? () => observableUrl : observableUrl,
requiresForm: true,
buttonViewCreator( isOnlyOne ) {
const button = new ButtonView( editor.locale );
Expand All @@ -389,12 +432,12 @@ describe( 'ImageInsertUI', () => {
} );
}

function registerUploadIntegration() {
function registerUploadIntegration( observableAsFunc ) {
observableUpload = new Model( { isEnabled: true } );

insertImageUI.registerIntegration( {
name: 'upload',
observable: observableUpload,
observable: observableAsFunc ? () => observableUpload : observableUpload,
buttonViewCreator( isOnlyOne ) {
const button = new ButtonView( editor.locale );

Expand Down
14 changes: 5 additions & 9 deletions packages/ckeditor5-ui/src/toolbar/balloon/balloontoolbar.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,16 +192,12 @@ export default class BalloonToolbar extends Plugin {
this.listenTo<ToolbarViewGroupedItemsUpdateEvent>( this.toolbarView, 'groupedItemsUpdate', () => {
this._updatePosition();
} );
}

/**
* Creates toolbar components based on given configuration.
* This needs to be done when all plugins are ready.
*/
public afterInit(): void {
const factory = this.editor.ui.componentFactory;

this.toolbarView.fillFromConfig( this._balloonConfig, factory );
// Creates toolbar components based on given configuration.
// This needs to be done when all plugins are ready.
editor.ui.once<EditorUIReadyEvent>( 'ready', () => {
this.toolbarView.fillFromConfig( this._balloonConfig, this.editor.ui.componentFactory );
} );
}

/**
Expand Down
23 changes: 10 additions & 13 deletions packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import clickOutsideHandler from '../../bindings/clickoutsidehandler.js';
import normalizeToolbarConfig from '../normalizetoolbarconfig.js';

import type { ButtonExecuteEvent } from '../../button/button.js';
import type { EditorUIUpdateEvent } from '../../editorui/editorui.js';
import type { EditorUIReadyEvent, EditorUIUpdateEvent } from '../../editorui/editorui.js';

const toPx = toUnit( 'px' );

Expand Down Expand Up @@ -191,20 +191,17 @@ export default class BlockToolbar extends Plugin {
beforeFocus: () => this._showPanel(),
afterBlur: () => this._hidePanel()
} );
}

/**
* Fills the toolbar with its items based on the configuration.
*
* **Note:** This needs to be done after all plugins are ready.
*/
public afterInit(): void {
this.toolbarView.fillFromConfig( this._blockToolbarConfig, this.editor.ui.componentFactory );
// Fills the toolbar with its items based on the configuration.
// This needs to be done after all plugins are ready.
editor.ui.once<EditorUIReadyEvent>( 'ready', () => {
this.toolbarView.fillFromConfig( this._blockToolbarConfig, this.editor.ui.componentFactory );

// Hide panel before executing each button in the panel.
for ( const item of this.toolbarView.items ) {
item.on<ButtonExecuteEvent>( 'execute', () => this._hidePanel( true ), { priority: 'high' } );
}
// Hide panel before executing each button in the panel.
for ( const item of this.toolbarView.items ) {
item.on<ButtonExecuteEvent>( 'execute', () => this._hidePanel( true ), { priority: 'high' } );
}
} );
}

/**
Expand Down
Loading

0 comments on commit 38ff3f1

Please sign in to comment.