Opened 5 years ago

Closed 2 years ago

Last modified 2 years ago

#10685 closed Bug (fixed)

Skin sprite image caching causes garbled toolbar icons after upgrade

Reported by: Jeremy Jannotta Owned by: Tade0
Priority: Normal Milestone: CKEditor 4.5.9
Component: UI : Skins Version: 4.0 Beta
Keywords: Cc: t.bussmann@…

Description

We just upgraded from v4.1.1 -> v4.1.3, and upon viewing the first time, everyone sees garbled/unreadable toolbar icons. Refreshing or clearing cache fixes it, but it appears very broken the first time people load it after upgrade.

Looks like there's no cache version on the skin's icons.png file (ckeditor/skins/moono/icons.png).

We'd like to see the same cache timestamp that's applied to other files, applied to the sprite image, to prevent this going forward. So in the case of 4.1.3, the sprite url would somethinglook like 'ckeditor/skins/moono/icons.png?t=D6IC'.

Change History (33)

comment:1 Changed 5 years ago by Jeremy Jannotta

I will add that this issue would also apply to any other UI images loaded by the ckeditor, like for plugins, skins, dialogs, etc.

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

Keywords: cache image skin removed
Status: newconfirmed

From what I see these files are loaded without a timestamp:

  • icons strip,
  • contents.css,
  • close.png image used in a dialog.

Or other files - lang, css, dialogs etc are loaded with timestamps. At least icons strip and contents.css could be loaded with a timestamp too.

comment:3 Changed 4 years ago by Jakub Ś

This ticket is related to #11625.

comment:4 Changed 4 years ago by Jakub Ś

#11345 was marked as duplicate.

comment:5 Changed 4 years ago by bmulholland

+1 - this is affecting my users on most ckeditor upgrades.

comment:6 Changed 4 years ago by Wiktor Walc

Milestone: CKEditor 4.4.2

Let's see what we can do with this.

comment:7 Changed 4 years ago by Wiktor Walc

Milestone: CKEditor 4.4.2CKEditor 4.4.3

comment:8 Changed 4 years ago by Matti Järvinen

If those files are included in CSS -files there really isn't much that can be done.

Options in that case would be:

  1. Add version number to every changed url() resource.
  2. Load theme CSS by Ajax replace url().

comment:9 Changed 4 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.3CKEditor 4.4.4

The 4.4.3 milestone was shortened, so the remaining tickets must be postponed.

comment:10 Changed 4 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:11 Changed 4 years ago by Artur Delura

Status: assignedreview

Now when generating release version on CKEditor in CKBuilder we add random 6 digit hash to each image in CSS file. Changes are in https://github.com/ckeditor/ckbuilder/tree/t/10685.

comment:12 Changed 4 years ago by Piotrek Koszuliński

Without changes in online builder this feature does not make sense yet, so until we've got all parts done we'll not be reviewing this.

comment:13 Changed 4 years ago by Wiktor Walc

Owner: changed from Artur Delura to Wiktor Walc
Status: reviewassigned

comment:14 Changed 4 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.4CKEditor 4.4.5

comment:15 Changed 4 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.5CKEditor 4.4.6

comment:16 Changed 4 years ago by Tobias Bussmann

Cc: t.bussmann@… added

comment:17 Changed 4 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.6CKEditor 4.6.0
Version: 4.1.14.0 Beta

Unfortunately, despite the importance of this issue, we will not have any resources to work on the builder part for at least next 2-3 months. This forces us to postpone this ticket even further, so currently the most reasonable milestone is 4.6.0.

comment:18 Changed 3 years ago by Jonathan

Can we bump up the priority on this one? We inevitably have users that run into this issue when we update the editor to a new version.

comment:19 Changed 3 years ago by Piotrek Koszuliński

4.6 is the closest possible milestone. 4.5 will be released in couple of days and change like this one cannot be included in a minor release. The good news is that 4.6 will not take so much time as 4.5. We we'll be back to our ~4 month release schedule.

In the meantime - we really do recommend serving every CKE version from a different directory (e.g. ckeditor/4.4.8/ckeditor.js). Unless you try to concatenate all files together this solves the issue.

comment:20 Changed 3 years ago by bmulholland

It's worth mentioning that concatenating all files is considered best practices.

comment:21 Changed 3 years ago by Piotrek Koszuliński

It's worth mentioning that concatenating all files is considered best practices.

Yes and no. If you have a website where CKEditor is used in 3 of 10 subpages, then it does not make sense to serve the same asset on all of these 10 subpages. It's better to split the asset so CKEditor can be loaded conditionally. Also, I cannot find the article now, but I remember reading that the optimal number of assets isn't really 1. If I remember correctly it was because browser waits so long to download it, while it could start parsing and executing some other file if the asset was split.

Anyway, not important here. We understand that some developers want to concatenate CKEditor with other scripts or that URL cannot be changed.

comment:22 Changed 3 years ago by Steve James

+1 for this from me, also. This has been a source of pain for me with regards to CKEditor upgrades.

In the meantime - we really do recommend serving every CKE version from 
a different directory (e.g. ckeditor/4.4.8/ckeditor.js). Unless you try 
to concatenate all files together this solves the issue. 

I do not entirely agree with this approach for CKEditor. I recognize this is considered a best practice regarding cache management by many, but my view is that it really is most applicable to resources in the folder structure that do not end up getting referenced via <a>/<img> tags in the content.

A specific example would be the smileys. A huge headache that I ran into was that I was ending up with a huge bunch of broken <img tags for smileys when upgrading from one ckeditor version to another and using the folder version approach. (300,000+ users... major pain) The reason, of course, was that the smileys are essentially content that is bundled with the smileys plugin in editor builds, and they are inserted into the content via <img tags that reference their file folders in the current editor folder structure, *unless you specifically tell the editor to look in another folder location in the config*. It took me a while to sort out isolating the smileys folder separate from the cke folder structure and setting the smiley folder location in the editor config.

Therefore, the folder version approach can cause problems for anything that the editor *inserts into the content* via a link or image tag that references bundled content in the current folder structure. Since I do not know what types of content (beyond smileys) you may bundle with the editor in the future, I specifically choose not to use the folder version approach, and I am also very much in favor of using a version/timestamp query parameter instead. What I would prefer is that you include it as an version/timestamp value as "buried" default config setting that I can override via a config setting I specify--that will help with special cases where I may need to force a resource reload for my users. (A specific example is forcing a reload for mono vs color toolbar icons.)

Last edited 3 years ago by Steve James (previous) (diff)

comment:23 Changed 3 years ago by Jakub Ś

#13897 was marked as duplicate.

comment:24 Changed 2 years ago by jkallay

The priority of doing something about this should be elevated, because in the current state this (like all caching-related bugs) breaks things in an unpredictable way, and when it does break it happens in a spectacular way (in the case of a cached icons strip).

The big problem here is that CKEditor advertises that it takes care of cache busting with the build version it appends everywhere. To make matters worse, with the icons the markup makes it appear that the right thing is happening, because each toolbar button has an inline style with a resolved background image url and a build number--the version of the url without the build number is sneakily winning because the rule in the skin's editor.css is !important. If something is known to be broken, it's better for it to be broken in an obvious way.

comment:25 Changed 2 years ago by Marek Lewandowski

This issue has been a subject of #2702171 in Drupal bug tracker.

comment:26 Changed 2 years ago by Tade0

Owner: changed from Wiktor Walc to Tade0

comment:27 Changed 2 years ago by Tade0

Status: assignedreview

Created a pull request in the CKBuilder repo that solves this issue: https://github.com/ckeditor/ckbuilder/pull/12.

comment:28 Changed 2 years ago by Marek Lewandowski

Looking good. All we're missing is putting 2.3.1 version at our host, and updating build.sh in ckeditor-dev and presets.

Let's try to ship it with 4.5.9.

comment:29 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.0CKEditor 4.5.9

comment:30 Changed 2 years ago by Tade0

Resolution: fixed
Status: reviewclosed

Fixed with commit d2155771c06a5a7437f30be9f3b42ef60bc184c2 in https://github.com/ckeditor/ckbuilder.

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

Hurray! Thanks guys :)

comment:32 Changed 2 years ago by bmulholland

Yay! I've been watching this for years and I'm totally stoked to have it fixed. Thanks all!

comment:33 Changed 2 years ago by Marek Lewandowski

Perfect, glad to hear it will help you! :)

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