Add isSupportedEnvironment virtual member

[jacekbogdanski]

What is the purpose of this pull request?

Task

Closes #2692

[Comandeer]

In fact this method should probably be bound to the plugin's namespace, not instance, as it's done in tableselection plugin.

---

If we add such method, lets refactor plugins at once. There are several plugins, that can use isSupportedEnvironment function:

• tableselection (for now there is property, not method)
• easyimage
• balloontoolbar
• copyformatting
• uploadwidget
• etc.

[Comandeer]

It's not virtual, it's very real 😛

[jacekbogdanski]

https://en.wikipedia.org/wiki/Virtual_function

However, taking into account that this function is not implemented by default, abstract may better define its purpose.

Actually, I see that the whole pluginDefinition is marked as virtual, so I will just skip modifier to not repeat it needlessly.

[Comandeer]

Probably it would be better to introduce such change in next major release, 4.12.0, not in 4.11.2.

[engineering-this]

It would be nice to refactor all the plugins which has limited environment support. We need to ignore tests once isSupportedEnvironment property/method is accessible, hence it should be in listener to pluginsLoaded. I'd make sense to put such logic in one general helper which would be accessible by any test.
This proposal comes from #2694 (comment)

[jacekbogdanski]

I'm not sure if this member should be attached into plugin namespace. In that case, we are unable to unify this function among the plugins. AFAIK we don't have any recommendation nor abstract interface how to write a plugin namespace, so we will be unable to define it in our docs easily, which can be done with pluginDefinition. I see that it will require some refactoring like for tableselection plugin but it still has to be done for other plugins like easyimage which uses isUnsupportedEnvironment in tests (this member has to be moved into the plugin itself).

Also, if I have a simple plugin like autolink, I will have to create special, separate plugin namespace to keep isSupportedEnvironment member. Writing something like:

CKEDITOR.plugins.autolink = { 
   isSupportedEnvironment: fn 
};

CKEDITOR.plugins.add( autolink, {
   init: function() { // init stuff }
 } );

seems less coder friendly for me than just:

CKEDITOR.plugins.add( autolink, {
   init: function() { // init stuff },
   isSupportedEnvironment: fn
 } );

Currently, our options are:

1. Define virtual method as a member of pluginDefinition
2. Put this function into the plugin namespace (not sure how to document such a requirement).
3. ?

[engineering-this]

I'd go with first option.

I have one suggestion. If this method is optional, then maybe it should have default value function(){ return true; }. Thanks to this we could update bender so it ignores every test for unsupported env based on bender-ckeditor-plugins declared in test file. We wouldn't have to ever write

if ( plugin.isSupportedEnvironment ) {
    bender.ignore(); // or assert.ignore();
 }

[jacekbogdanski]

I was also thinking about it. It also fixes the issue when you have to manually check if isSupportedEnvironment is declared, like:

if ( plugin.isSupportedEnvironment && plugin.isSupportedEnvironment()) {
// do stuff
}

[f1ames]

I'm late to the party, but agree with @engineering-this on adding virtual method to pluginDefinition. And integrating it with bender would be nice 🤩

[jacekbogdanski]

It seems like it makes sense to pass an optional editor instance as an argument. It will complicate refactoring a bit, although some plugins require information about editor state. As an example, docprops plugin only works for a classic editor with config.fullPage = true.

// Example for docprops
pluginDef.isSupportedEnvironment = function( editor ) {
    return !editor.editable().isInline() && editor.config.fullPage;
}

We could even extend this feature to automatically detect unsupported plugins and prevent executing initialization methods.

[jacekbogdanski]

I refactored tableselection, easyimage and balloontoolbar plugins + tests. There is much more work to do thus this PR will require reviewing all our plugins in the context of a supported environment. I would like to take it slowly because if the current implementation will be rejected, we will spend a lot of time refactoring again.

Please, take a look if you are fine with the current changes and if so, I will continue refactoring/implementing support sniffing methods.

[f1ames]

Taking over review of this PR.

[f1ames]

Rebased on the latest major.

[f1ames]

Looking good 👍

I have one suggestion/idea. What if bender.tools.ignoreUnsupportedEnvironment() method could be called automatically by bender without a need to add it in every test suite? You could just create a plugin with proper isSupportedEnvironment() function and create tests without really thinking about correct ignoreUnsupportedEnvironment() calls (tests may also have other plugins which can have env restriction and then one would have to check them, or just add all used plugins to ignoreUnsupportedEnvironment() call).

From what I see the proper place to add such automatic call could be bender extensions where under tests var you have current test suit (and can modify setUp calls) and under bender.plugins a list of plugins loaded for this test suite. WDYT? (Btw. This code is rn only for unit tests, so manual ones will need different place to hook in).

[f1ames]

Was there any particular reason why this code was added here and not in plugin.js, somewhere near: https://github.com/ckeditor/ckeditor-dev/blob/680bdd9f66dfa21e0822a1154573ddda743a1a75/core/plugins.js#L71 ?

[jacekbogdanski]

onLoad method may not be provided, but yeah it makes more sense to attach default method implementation inside core/plugins.js file.

[f1ames]

onLoad method may not be provided.

Yes, I meant the line above. It shouldn't depend on presence of onLoad.

[jacekbogdanski]

Looks like the safest place is https://github.com/ckeditor/ckeditor-dev/blob/14e3bab99f1d4b968dd44e9b4e5b408e97b26758/core/plugins.js#L37 because it makes sure that the function won't be attached multiple times.

[jacekbogdanski]

As we already talked F2F, there are some popular use cases where we cannot simply automate ignoring tests, like:

• using multiple declared editors by bender.editors
• declared editors may use a different set of plugins than information in bender-ckeditor-plugins tag

I updated an additional list of plugins. Please, tell if you still see some plugins requiring covering.

Taken over PR review.