Implement a balloon toolbar factory

[oleq]

ATM the following features share the same concept of a toolbar attached to a widget:

• Image,
• Table,
• MediaEmbed

but they don't share the code, which is almost the same. We need a utility to create this kind of component easily to DRY and centralize the maintenance. Otherwise it's a waste of time and LOC.

cc @ma2ciek, @Reinmar

[ma2ciek]

Toolbars currently diff in only a few places:

• plugin name
• config's field from which the toolbar is generated (e.g. the image.toolbar)
• different is*Selected fn.

Note that for some widget we can need two toolbars - e.g. for table cell and the table widget.

[ma2ciek]

Extracting @pjasiun comment from @ckeditor5-collaboration:

What we should do is:

• introduce in widget package base WidgetToolbar plugin,
• each of image, table and media packages should keep their own plugins which extends WidgetToolbar and overwrites:
  • isSelected method
  • config getter (or getConfig method).

[ma2ciek]

An abstract class and simple inheritance like the above should be pretty easy to implement. The downcast of this approach is the Plugin->WidgetToolbar->ImageToolbar inheritance. I'd only change the isSelected() method to the isVisible().

There's also a common part that could be actually moved to the BallonToolbar directly:

// If the `BalloonToolbar` plugin is loaded, it should be disabled for tables
// which have their own toolbar to avoid duplication.
// https://github.com/ckeditor/ckeditor5-image/issues/110
if ( balloonToolbar ) {
	this.listenTo( balloonToolbar, 'show', evt => {
		if ( isTableWidgetSelected( editor.editing.view.document.selection ) ) {
			evt.stop();
		}
	}, { priority: 'high' } );
}

IMO the balloonToolbar should be hidden by default when the widget is selected as every widget has its own toolbar (correct me if I'm wrong here).

And I'm not sure how the second table toolbar config should be named (for the table widget). As the table: toolbar: []} is already reserved.

[Reinmar]

Do we have to introduce this by a plugin and inheritance? It's ok to me that there is some WidgetToolbar plugin for enabling this functionality in general, but I'd recommend that it only enables what it can (generically) and then provides an API to do the rest.

The ckeditor5-widget package already does things similarly. You load the Widget plugin only to enable the support for widgets but then you use utils such as toWidget() to do the rest.

Additionally, unless you see that concretising this feature to widgets only makes it simpler, then I'd consider keeping generic to e.g. all kinds of elements.

[Reinmar]

PS. This is the kind of API that greatly improves the DX and this, in fact, I think it was already requested by the community, so I assigned it to "next".

[ma2ciek]

After a F2F call, I had another idea, which should be pretty easy to implement and works similarly to how the ContextualBalloon works.

We could make the WidgetToolbar plugin a collection of widget toolbars. It would listen once to events and handle together all widget toolbars.

Then, the WidgetToolbar plugin could be used as follows:

export default class ImageToolbar extends Plugin {
    static get requires() {
		return [ WidgetToolbar ];
	}

    afterInit() {
        const editor = this.editor;
        const widgetToolbar = editor.plugins.get( 'WidgetToolbar' );

        widgetToolbar.add( 'image', {
            items: editor.config.get( 'image.toolbar' ) || [],
            isSelected: isImageWidgetSelected,
        } );
    }

	// Optionally. `WidgetToolbar#destroy()` would remove all listeners.
	destroy() {
		const widgetToolbar = editor.plugins.get( 'WidgetToolbar' );

		widgetToolbar.remove( 'image' );
	}
}

The WidgetToolbar plugin could also prevent from rendering BalloonToolbar and widget toolbar at the same time with one listener for all widgets in the init.

WDYT?

[pjasiun]

I like this idea. It is simple to describe, does not force you to create own plugin to add a widget toolbar (all you need to do is to load one) and consistent with other parts of the API (we have a few plugins which are this kind of utils, for instance, UploadRepository). At the same time, since widget toolbar is a plugin is can attach all its listener when it is needed. Also keeping all block toolbar in a single plugin give us some control can be useful if we will work on ui#update refactoring. Long story short 👍 :)

[ma2ciek]

I'm not sure if the isSelected is the best name for the current callback. It specifies when the toolbar should visible, so I'd go rather in this direction.

And I can't go with isWidgetSelected because it has to work with a toolbar for table cells.

My first thought was isVisible, but it's not descriptive enough.

Maybe:

• whenIsVisible
• visibleWhen

WDYT?

[Reinmar]

visibleWhen 👍 Or showWhen – even better because it indicates that that's an action to perform - show/hide.

[pjasiun]

Balloon toolbar has decorative show method. We might have something similar, but I need to see a bigger picture to propose something.

[Reinmar]

        widgetToolbar.add( 'image', {
            items: editor.config.get( 'image.toolbar' ) || [],
            isSelected: isImageWidgetSelected,
        } );

widgetToolbar.add() doesn't sound good to me. If you'd try to read the code above you would think it adds an image to the widget toolbar.

The first problematic thing is the plugin name. Its instance widgetToolbar seems to be a toolbar, while in reality, this is something like a repository of toolbars. That's not necessary, but we can consider renaming it to sth like WidgetToolbarFactory or WidgetToolbarRepository.

We can keep the short name too, but then we need to rename add() to something that plays better. define(), register() or create()` might be good.

[Reinmar]

Hehe :) Even the documentation has a problem with the toolbar vs collection thing:

https://github.com/ckeditor/ckeditor5-widget/pull/54/files#diff-0ce103b5aa5a8a551ca18426ddb7bb45R89

[pjasiun]

Lets move this discussion to the PR: https://github.com/ckeditor/ckeditor5-widget/pull/54/files