Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#9992 closed Task (fixed)

Styles combo integration with allowed content filter

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

Description (last modified by Jakub Ś)

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 (12)

comment:1 Changed 11 years ago by Piotrek Koszuliński

Status: newconfirmed

Related ticket: #9255.

comment:2 Changed 11 years ago by Piotrek Koszuliński

Type: BugTask

comment:3 Changed 11 years ago by Jakub Ś

Description: modified (diff)

comment:4 Changed 11 years ago by 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.

comment:5 Changed 11 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

comment:6 Changed 11 years ago by Piotrek Koszuliński

Status: assignedreview

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 11 years ago by 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.

comment:8 Changed 11 years ago by Frederico Caldeira Knabben

Status: reviewreview_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 Changed 11 years ago by Piotrek Koszuliński

Status: review_failedreview

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 11 years ago by Frederico Caldeira Knabben

Status: reviewreview_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 11 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

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 11 years 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 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy