Styles combo integration with allowed content filter

[Piotrek Koszuliński]

This issue is extracted from #9829.

Due to asynchronous nature of styles.js loading it's now impossible to integrate it with filter. This has to be done before data are set in editor, because otherwise all styles allowed by stylescombo are filtered out.

Possible solutions:

• Force loading styles.js together with plugins files.
• Merge it with config.js - is there a reason to keep it separately?

[Piotrek Koszuliński]

Related ticket: #9255.

[Piotrek Koszuliński]

Another aspect of this issue: when stylescombo is not enabled in toolbar styles.js should not be loaded. Now it's preloaded on instanceReady.

[Piotrek Koszuliński]

Pushed ​t/9992 on dev and tests.

For reviewer: please, verify changes I've done in the docs. They show that the docs for config.stylesSet were outdated and perhaps that v4.0 introduced a bug. Since then setting stylesSet to just a name (without URL) doesn't make much sense, because regardless of the name, the same file (styles.js in root folder) will be loaded. Before 4.0 the name was used to load proper file (name + '.js') from styles directory.

In short - 4.0 changed this settings behaviour and now it looks weird.

Another thing - I think that we can remove backward compatibility with config.stylesCombo_stylesSet - I already removed the note from docs and IMO it's also time to remove it from code, because we're modifying the default behaviour anyway. That name has been changed before [5280], so long before the dinosaurs.

[Piotrek Koszuliński]

I've realised that there's a small issue with styles registered by stylescombo. They are allowing 'img(!left)' and 'img(!right)' (our samples' default styles). That unfortunately causes an issue when image button is removed, because attributes like src, alt are removed and empty <img class="left"> is left.

I think that this issue should be fixed by #10073 (so by the htmlDataProcessor), because it targets similar issue with <a></a> being left when only one of Anchor/Link buttons are enabled.

The downside of fixing those cases by htmlDP is that standalone version of ACF will output incorrect HTML. Only ACF+htmlDP duet will be truly valid. But if we think of ACF as of filter which just filters out things, not caring about correctness it may be still a good solution. Otherwise - we'll have to fix those cases in filter and htmlDP too.

Note: Content: <p><img></p> isn't fixed on master.

[Frederico Caldeira Knabben]

The idea about having styles.js is exactly to avoid bloating config.js. It should work just like an additional configuration file, which is dedicated to styles only.

We've been using styles.js for way too long already and there is zero benefit on removing it, other than avoiding a HTTP request (and making config.js maintenance harder). If one is concerned about the performance issue, it should be still possible to have the styles defined in config,js. By default, as historically done, style.js should be used.

The original idea as simply chancing the moment when styles.js is loaded. This doesn't bring any backward compatibility concerns. It is just a editor loading workflow change.

[Piotrek Koszuliński]

I pushed both branches once more :)

1. I reverted the merge styles.js into config.js. It's again a separate file.
2. Default value of stylesSet is again 'default'. But it is still possible to set it to false to prevent loading any styles set. I think that this is important since now we preload styles set before loading plugins, so without div and stylescombo plugins styles.js file is still preloaded what doesn't make sense.
3. We discussed the possibility to add 'needsStyles' flag to plugin definition but it would make editor's loading code more complicated. First, plugins would need to be loaded, then all should be checked whether they have 'needsStyles' flag, if yes, then before initializing plugins the styles set file would have to be preloaded... ;|

[Frederico Caldeira Knabben]

Replying to Reinmar:

3. We discussed the possibility to add 'needsStyles' flag to plugin definition but it would make editor's loading code more complicated. First, plugins would need to be loaded, then all should be checked whether they have 'needsStyles' flag, if yes, then before initializing plugins the styles set file would have to be preloaded... ;|

This pre-processing is already done in plugins.js, so it should not be a big issue to just add an additional check for it.

I'll leave this to you to decide as this may be a clean way to have styles loaded on demand, by simply marking plugins that need it.

[Piotrek Koszuliński]

Merged to major with ​git:6a0fd35 on dev and b48b207 on tests.

I haven't decided to try implementing needsStyles flag. It may be possible to do that pretty easily, but I feel uncomfortable with that code (cause I haven't worked on it yet) so this may stop me for too long or cause issues in the future.

[Wim Leers]

Thanks to this patch, we were able to eliminate 1 HTTP request for Drupal 8's use of CKEditor :) Thanks!