#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 11 years ago by
comment:2 Changed 11 years ago by
Keywords: | cache image skin removed |
---|---|
Status: | new → confirmed |
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:7 Changed 10 years ago by
Milestone: | CKEditor 4.4.2 → CKEditor 4.4.3 |
---|
comment:8 Changed 10 years ago by
If those files are included in CSS -files there really isn't much that can be done.
Options in that case would be:
- Add version number to every changed url() resource.
- Load theme CSS by Ajax replace url().
comment:9 Changed 10 years ago by
Milestone: | CKEditor 4.4.3 → CKEditor 4.4.4 |
---|
The 4.4.3 milestone was shortened, so the remaining tickets must be postponed.
comment:10 Changed 10 years ago by
Owner: | set to Artur Delura |
---|---|
Status: | confirmed → assigned |
comment:11 Changed 10 years ago by
Status: | assigned → review |
---|
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 10 years ago by
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 10 years ago by
Owner: | changed from Artur Delura to Wiktor Walc |
---|---|
Status: | review → assigned |
comment:14 Changed 10 years ago by
Milestone: | CKEditor 4.4.4 → CKEditor 4.4.5 |
---|
comment:15 Changed 10 years ago by
Milestone: | CKEditor 4.4.5 → CKEditor 4.4.6 |
---|
comment:16 Changed 10 years ago by
Cc: | t.bussmann@… added |
---|
comment:17 Changed 10 years ago by
Milestone: | CKEditor 4.4.6 → CKEditor 4.6.0 |
---|---|
Version: | 4.1.1 → 4.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 9 years ago by
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 9 years ago by
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 9 years ago by
It's worth mentioning that concatenating all files is considered best practices.
comment:21 Changed 9 years ago by
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 9 years ago by
+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.)
comment:24 Changed 9 years ago by
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:26 Changed 9 years ago by
Owner: | changed from Wiktor Walc to Tade0 |
---|
comment:27 Changed 9 years ago by
Status: | assigned → review |
---|
Created a pull request in the CKBuilder repo that solves this issue: https://github.com/ckeditor/ckbuilder/pull/12.
comment:28 Changed 9 years ago by
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 9 years ago by
Milestone: | CKEditor 4.6.0 → CKEditor 4.5.9 |
---|
comment:30 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | review → closed |
Fixed with commit d2155771c06a5a7437f30be9f3b42ef60bc184c2
in https://github.com/ckeditor/ckbuilder.
comment:32 Changed 9 years ago by
Yay! I've been watching this for years and I'm totally stoked to have it fixed. Thanks all!
I will add that this issue would also apply to any other UI images loaded by the ckeditor, like for plugins, skins, dialogs, etc.