#12387 closed Bug (fixed)
Bug with skin, nonsupporting chameleon feature and specified uiColor
Reported by: | Danil | Owned by: | Artur Delura |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 4.4.7 |
Component: | UI : Skins | Version: | 4.5.0 Beta |
Keywords: | Cc: |
Description
If skin does not support chameleon feature and user specifies uiColor setting he get either js errors (1) (if skin is builtin) or messed up skin (2) (if skin is external file and we have another builtin skin, that defined chameleon
feature.
See http://jsfiddle.net/danya_postfactum/dubhh1st/
CKEditor can be used in a CMS, that allows to specify skin and uiColor properties, so an user is not protected.
CK Docs say:
Of course we don’t recommend this, but if that is the case, it is enough to not defined the chameleon function in the skin.js file.
This is bad advise. To fix this bug we could check chameleon property is callable (1) and delete obsolete property when scriptLoader loads a new skin (or even recreate CKEDITOR.skin object)
Change History (26)
comment:1 Changed 10 years ago by
comment:2 Changed 10 years ago by
Milestone: | → CKEditor 4.4.7 |
---|---|
Status: | new → confirmed |
I can't reproduce this. Are you sure that the link (http://jsfiddle.net/danya_postfactum/dubhh1st/) is correct?
I also downloaded the bootstrapck skin, and ran it with uiColor setting. No erros (the skin actually supports the chameleon feature).
Edit: I also checked what happens if I remove the chameleon function and indeed it throws an error. Definitely a bug on our side. I'm adding this ticket to the next milestone, because I think that the fix should be pretty simple. If not, we'll need to remove it from the milestone, because we will not have a lot of time for bugfixes in 4.4.7.
comment:3 Changed 10 years ago by
Indeed. bootstrapck skin defines chameleon, but it looks ugly. Ok, this skin is irrelevant.
I updated demo: http://jsfiddle.net/danya_postfactum/dubhh1st/3/
There I used moono-dark skin. So, you can see how moono chamelemon function is applied to moono-dark skin :) It is easy to understand: builtin skin defines properties on global object CKEDITOR.skin. If loaded secondary skin will not override chameleon prop, it will be applied.
Workaround for skins:
CKEDITOR.skin.chameleon=function(){return""};
We should recreate skin when loading external skin.
comment:4 Changed 10 years ago by
Owner: | set to Artur Delura |
---|---|
Status: | confirmed → assigned |
comment:5 Changed 10 years ago by
After investigation I end up with following conclusions:
- We have no control of what is done in
skin.js
file. We have no information whenCKEDITOR.skin
object is modified i.e. whenskin.js
is executed. - Current implementation assume that there is only one skin available on page and each editor instance use the same. See this fiddle. When we load two skin into editor, first one is immediately overwritten.
- Global
CKEDITOR.skin
is accessible in various places in code.
Sice skin is a public API we don't want to introduce any changes in it, because we don't want force developers to make any changes in their skin code. I think it's not a minor deal to handle this issue propery, because we should implement it this way:
- Each skin definition should be stored in diffrent object (just not to lose any data):
CKEDITOR.skin[ 'moono-dark' ].name; // "moono-dark" CKEDITOR.skin[ 'moono-dark' ].chameleon; // undefined CKEDITOR.skin[ 'kama' ].chameleon; // function() {} // etc.
- Each editor instance should have property which will determine which skin it uses.
- Each editor instance should have it's own Skin instance with specific properties.
- We should drop using global
CKEDITOR.skin
. - We might introduce some public function
CKEDITOR.skind.add
which will give us more control over skins definition (or we can workaround this by using Object.watch).
comment:6 Changed 10 years ago by
Thank you. I understand. I think we should update docs: http://docs.ckeditor.com/#!/guide/skin_sdk_chameleon :
Note that adopting this feature is totally optional. A skin developer may decide to have a fixed color and not give the possibility to change it. Of course we don’t recommend this, but if that is the case, it is enough to not defined the chameleon function in the skin.js file.
We should say in docs that chameleon function is required. But it may return empty string. Or we could check chameleon property is callable, so this will fix issue 1 (see bug description) and this will also allow to write skin.chameleon = false
comment:7 Changed 10 years ago by
While we assume that there is only one skin available per page (current desing) chameleon
function in not required. As you mentioned - this is bug in core/skin
, and we will fix it :) Also documentation will be updated.
comment:9 Changed 10 years ago by
While we assume that there is only one skin available per page
My use case is a CMS, that allows to upload and select a custom skin, so there may be two skins loaded (first is built in).
comment:10 Changed 10 years ago by
Status: | review → assigned |
---|
comment:11 Changed 10 years ago by
Status: | assigned → review |
---|
No skin is set up in CKEditor until you create the first editor instance. When the first skin is loaded, all CSS files are loaded as well. If you wanted to create a new editor instance with a diffrent skin, you should unload the CSS files which are connected with the first skin. At the moment no such mechanism exists for CKEditor, though. This solution might also break the previous CKEditor instance.
I you create the first editor instance with a custom skin, everything should work well because nothing is set up yet.
If you have an existing instance on the page and want to create a new instance with a custom skin, the only solution sems to be to create an instance in the scope i.e. in an iframe.
comment:12 Changed 10 years ago by
Status: | review → review_failed |
---|
Tests:
- Create tests/core/skin/ directory and move both test files related to skins there.
skinnochameleon.js
will be a better name._assets/skins/myskin/skin.js
is pretty specific for that test, so better if it will be moved closer to it, so it's not reused in the future what might result in some other developer adding there the chameleon function... Of course you can add a comment warning against that, but it's just easier to make it a skin precisely for this test. "myskin" isn't a good name too ;).- Try to add another assertion to that test case because editor might not be initialised even though an error isn't thrown (an error may be caught).
comment:14 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | review → closed |
Fixed on master with git:0756717.
And I've just found that in http://docs.ckeditor.com/#!/api/CKEDITOR.config-cfg-skin it's mentioned that only one skin per page is supported and I don't see other place where we could mention this now.
Of course we don’t recommend this, but if that is the case, it is enough to not defined the chameleon function in the skin.js file.
And this is now true.
comment:15 Changed 10 years ago by
So, skin
option is intended to use only when you rebuild ckeditor with skin you are going to use. Right? So, you can't build ckeditor with skin1
, but pass skin
option with value skin2
. But since we use default skin (that built in ckeditor js file), are we required to pass skin
option?
comment:16 Changed 10 years ago by
http://jsfiddle.net/danya_postfactum/dubhh1st/3/ - here is demo of this issue: Whe use ONE skin per page. But that skin is not builtin. So, this usage is incorrect, right? If I want to use 'moono-dark' skin, I have to rebuild ckeditor script with this skin. Am I right?
comment:17 Changed 10 years ago by
http://jsfiddle.net/danya_postfactum/dubhh1st/3/ - here is demo of this issue: Whe use ONE skin per page. But that skin is not builtin. So, this usage is incorrect, right? If I want to use 'moono-dark' skin, I have to rebuild ckeditor script with this skin. Am I right?
No. You can use other skin than the one built in the package. I was only saying that you can't initialise two editors on the same page, one using skinA and the second one using skinB. The fiddle you linked to is totally correct.
comment:18 Changed 10 years ago by
PS. If you think that some article in our docs is misleading or could contain more precise information, let us know. I tried to find such, but it's hard for me, because I already know this.
comment:19 Changed 10 years ago by
And this is now true.
So, according to my jsfiddle demo, this is false. So, skin must always define chameleon
property.
comment:20 Changed 10 years ago by
So, according to my jsfiddle demo, this is false. So, skin must always define chameleon property.
Does it use the latest master version? We've just fixed the bug in https://github.com/ckeditor/ckeditor-dev/commit/0756717bca6ed4bebd44c3fdb81f4b77c34331cc. You don't have to define the chameleon property any more.
comment:21 Changed 10 years ago by
Does it use the latest master version?
No, but I don't think this patch makes sense in case of http://jsfiddle.net/danya_postfactum/dubhh1st/3/
The view is messed up because of moono-dark does not override chameleon
property (defined in builtin moono
).
Please read bug description again. As far as I understand this patch fixes issue 1, and does nothing with issue 2.
comment:22 Changed 10 years ago by
Ah... ok. I was totally confused because we talked here about at least 3 issues.
You're right that if the built-in skin defines CKEDITOR.skin.chameleon property, then it will be used for the dynamically loaded skin if it does not define its chameleon property. But... it sounds that the skin which does not want the chameleon feature should set CKEDITOR.skin.chameleon to null
. Am I right?
comment:23 Changed 10 years ago by
Yes, that what I talking about )
null
, or maybe false
. And () => ''
for backwards compatibility.
comment:25 Changed 10 years ago by
I fixed the docs today: https://github.com/ckeditor/ckeditor-docs/commit/9aef2552588f73d9d9c008fe66ff80ad78637326.
I could try to fix it but could you point me to the right way?