Should "isSupportedEnvironment" prevent plugins from loading on unsupported browsers

[f1ames]

Type of report

Feature request

Provide description of the new feature

We have introduced pluginDefinition.isSupportedEnvironment() method in #2692. When the method is called it says if the plugin is supported in the current environment (browser). And that's basically it. The main motivation was to use it in tests to prevent running tests which shouldn't be run on a particular browsers.

Still, even if the method is implemented the plugin author needs to manually call it and add some additional code to prevent plugin from initializing in unsupported environment. I wonder if it would be reasonable to add a mechanism which will automatically prevent plugin initialization on unsupported envs.

1. There are probably some cases (tableselection?) where we need more custom handling to prevent specific code.
2. There are some plugins now, which are not supported but still somehow available (not disabled) on unsupported browser, e.g. mathjax on IE8?

WDYT?

[wwalc]

How will this work with ACF?

[jacekbogdanski]

That was the intention of providing isSupportedEnvironment member, but we missed this point somewhere when working on this ticket. I was thinking about not initializing plugin if it's not supported as soon as possible i.e during plugins loading mechanism, so init method (and related) are not called. Although, it will require some mechanism resolving dependencies when they are also not supported (this point should be probably covered by the plugin itself 🤔 )

How will this work with ACF?

Do you mean plugins which are not supported but provides ACF rules? 🤔

[engineering-this]

If certain env is not supported by some plugin, but it kinda works we shouldn't break this. So we could do this only for new plugins via some flag, eg. pluginDefinition.requireSupportedEnv, but that brings extra complexity, I wonder if it'd be beneficial for us.

[jacekbogdanski]

If certain env is not supported by some plugin, but it kinda works we shouldn't break this. So we could do this only for new plugins via some flag, eg. pluginDefinition.requireSupportedEnv, but that brings extra complexity, I wonder if it'd be beneficial for us.

If a plugin is not officially supported for env, I'm not sure if we should care, especially that it may provide some editor issues. Something like using private variables.

[wwalc]

Do you mean plugins which are not supported but provides ACF rules? 🤔

Yes

[jacekbogdanski]

Yeah, it may be an issue here. Like if someone is loading editor HTML from database modified with Chrome but at the same time editing this HTML in IE8 may result in partially removed content. Maybe we could still include ACF rules in that case? Not sure if it can be possible / makes sense anymore.

[Comandeer]

Maybe we could still include ACF rules in that case? Not sure if it can be possible / makes sense anymore.

It's at least tricky at the moment, due to #678. We will have to add plugin's button to the toolbar, otherwise ACF rules could be skipped. Even fixing this issue wouldn't help us that much, as we will have to separate ACF, command and button registration to really allow handling them on their own.

The most elegant method to register ACF separately would be IMO adding something like filterRules property to CKEDITOR.pluginDefinition, but yet again: it's a really big refactoring without gaining enough to justify it.

OTOH Easy Image seems to just explode in old IE, so in such case it would make the whole thing more elegant to the user. Yet, at the same moment, I'm wondering: in EI only upload is not working correctly and displaying images would work normally even in IE 8. It seems that browser support in our plugins is more subtle than simple "doesn't work"/"works correctly".

[wwalc]

@Comandeer summed up my concerns/doubts pretty well. Also refactoring plugins now to resolve the user experience of users running IE8-IE10 sounds a bit like a waste of time (I mean the approach to solve all issues of a similar kind at once). I'd rather identify the most critical issues coming from browser incompatibilities (focusing on modern environments) and address them one by one, if they are significant enough to be addressed.

[f1ames]

So there are two reasonable options ATM without putting too much effort in this issue:

1. Leave isSupportedEnvironment as is and use it manually.
2. Introduce some additional flag which when defined will trigger auto-checking mechanism as @engineering-this mentioned in :

If certain env is not supported by some plugin, but it kinda works we shouldn't break this. So we could do this only for new plugins via some flag, eg. pluginDefinition.requireSupportedEnv, but that brings extra complexity, I wonder if it'd be beneficial for us.

and use this flag for new plugins or plugins which doesn't need to run any code in unsupported browsers (not using ACF, etc) - probably easyimage or tableselection.

WDYT?

[jacekbogdanski]

Maybe instead of a simple flag, it should be fallback function executed when a plugin is not supported on the environment? So it's still possible to ignore plugin using standard mechanism but e.g. make sure that ACF is correctly configured. It also can result in falsy value to prevent ignoring.

pluginDefinition.beforeIgnore = function( editor ) {
// add ACF rules or whatever you want
// executed only when `isSupportedEnvironment`
// fails instead of `pluginDefinition.init` methods
}

Nevertheless, we are adding additional complexity instead of removing it, so probably leaving it as is makes more sense.

[f1ames]

Ok, so let's leave it as is to not further complicate the code/API.