Ticket #9992 (closed Task: fixed)

Opened 20 months ago

Last modified 19 months ago

Styles combo integration with allowed content filter

Reported by: Reinmar Owned by: Reinmar
Priority: Normal Milestone: CKEditor 4.1 RC
Component: Core : Styles Version:
Keywords: Drupal Cc: wim.leers@…

Description (last modified by j.swiderski) (diff)

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?

Change History

comment:1 Changed 20 months ago by Reinmar

  • Status changed from new to confirmed

Related ticket: #9255.

comment:2 Changed 20 months ago by Reinmar

  • Type changed from Bug to Task

comment:3 Changed 20 months ago by j.swiderski

  • Description modified (diff)

comment:4 Changed 20 months ago by Reinmar

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

comment:5 Changed 19 months ago by Reinmar

  • Status changed from confirmed to assigned
  • Owner set to Reinmar

comment:6 Changed 19 months ago by Reinmar

  • Status changed from assigned to review

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.

comment:7 Changed 19 months ago by Reinmar

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.

comment:8 Changed 19 months ago by fredck

  • Status changed from review to review_failed

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.

comment:9 follow-up: ↓ 10 Changed 19 months ago by Reinmar

  • Status changed from review_failed to review

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... ;|

comment:10 in reply to: ↑ 9 Changed 19 months ago by fredck

  • Status changed from review to review_passed

Replying to Reinmar:

  1. 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.

comment:11 Changed 19 months ago by Reinmar

  • Status changed from review_passed to closed
  • Resolution set to fixed

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.

comment:12 Changed 19 months ago by Wim Leers

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

Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy