Support from Acquia helps fund testing for Drupal Acquia logo

Comments

uberhacker’s picture

Attached is a proposed fix for this issue.

uberhacker’s picture

Status: Active » Needs review
mgifford’s picture

Patch didn't apply to the git repo. I've re-rolled the patch.

EDIT: The functions moved around a bunch but I put it in ckeditor_load_toolbar_option() which I think will work.

Using http://gtmetrix.com to review my site it looks like a bunch of this is really just useless for anonymous users:

http://example.org/sites/all/modules/contrib/ckeditor/ckeditor.css?mj00wk: 1.7kB of 3.3kB is not used by the current page.

Devin Carlson’s picture

Version: 7.x-1.6 » 7.x-1.x-dev
Issue summary: View changes
FileSize
10.54 KB

A patch to split the current CSS up into three separate files (to prevent useless CSS from being loaded for certain users, as @mgifford mentioned in #3) and properly #attach them to form items/render arrays.

  • ckeditor.css - attached to text_format elements (loaded whenever a text_format is displayed)
  • ckeditor.editor.css - conditionally attached to text_format elements (loaded whenever CKEditor replaces a text_format)
  • ckeditor.admin.css - attached to the administration form (loaded whenever someone uses the administration page)
kaipee_s2s’s picture

Does the patch (#4) apply conditions around the loading of the css files so that they are only requested when CKEditor is actually loaded?

For example on my home page the ckeditor.css file gets loaded - but there isn't even any input field and CKeditor does not load.

In fact, I have 72 css files and 32 scripts being loaded, on average, on each page - most of which don't need to be (such as this ckeditor css file).

Devin Carlson’s picture

@kaipee_s2s Yes, the patch in #4 is to change CKEditor to only load CSS when necessary.

You can help test by applying the patch and checking whether the CKEditor CSS is only added where appropriate: when the page has a text_format, when the text_format is configured to load the editor and when you're editing the CKEditor settings on the administration page.

Devin Carlson’s picture

A reroll of #4.

Devin Carlson’s picture

FileSize
10.91 KB

An updated patch to add the missing files.

wwalc’s picture

@Anyone interested in having this committed: please let me know if the #9 patch makes sense.

Few notes:

1) ckeditor.css should be loaded on every single page, unfortunately, because it contains definitions of classes that could have been used to create content in the past. If user created a page where alignment was done with .rteleft, rteindent1, then this CSS file is critical in order to have the content displayed properly as before, and we're not talking here in the "edit mode" but simply when the page is rendered.

2) Actually ACF for CKEditor should be configured by default in this module to allow such classes like .rteleft, rteindent1 etc., considering that config.indentClasses are defined in ckeditor.config.js, but I'll handle this in a separate issue.

3) Comparing to the patch in #8:

  1. the hook_init() is back,
  2. paths to css files were corrected to match the new location (module_path/css/*)
  3. the following code was removed because hook_init() is back:
      // Attach the positioning CSS.
      $types['text_format']['#attached']['css'][] = drupal_get_path('module', 'ckeditor') . '/css/ckeditor.css';
    

4) I was feeling really bad whenever I saw that big ugly CSS loaded on every page. I'm happy to see the size of the file being reduced to the absolute minimum.

loopduplicate’s picture

loopduplicate’s picture

The patch didn't seem to apply cleanly for me after this morning's commit to the dev branch so I'm queuing it for retesting.

Status: Needs review » Needs work

The last submitted patch, 9: attach-ckeditor-css-1370894-9.patch, failed testing.

Devin Carlson’s picture

Status: Needs work » Needs review
FileSize
14.38 KB

A reroll of #9 that #attaches ckeditor.css to the page in hook_page_build() for improved cacheability and alterability. It also sets 'every_page' => TRUE so that the CSS gets aggregated with any other CSS that belongs on every page.

  • Commit 4801493 on 7.x-1.x authored by Devin Carlson, committed by wwalc:
    Issue #1370894 by uberhacker, mgifford, Devin Carlson: Conditionally...
wwalc’s picture

Status: Needs review » Fixed

Thank you Devin and everyone involved, the patch looks cool, so I pushed the changes. Hurray :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

mchaplin’s picture

Hi

Did this change ever make it into a normal release?

We have version 7.x-1.17.

Our home page references sites/all/modules/ckeditor/css/ckeditor.css

Thanks

Mike