Bug with skin, nonsupporting chameleon feature and specified uiColor

[Danil]

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)

[Danil]

I could try to fix it but could you point me to the right way?

[Piotrek Koszuliński]

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.

[Danil]

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.

[Artur Delura]

After investigation I end up with following conclusions:

• We have no control of what is done in skin.js file. We have no information when CKEDITOR.skin object is modified i.e. when skin.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).

[Danil]

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

[Artur Delura]

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.

[Artur Delura]

Changes and tests in ​branch:t/12387.

[Danil]

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).

[Artur Delura]

No skin is set up in CKEditor until the first editor instance is created. When the first skin is loaded, all CSS files are loaded as well. If next editor instance is created with a diffrent skin, to make it work, previous CSS files, which are connected with the first skin, should be unloaded. At the moment no such mechanism exists for CKEditor, though. This solution might also break the previous CKEditor instance.

If 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 seems to be to create an instance in the scope i.e. in an iframe.

[Piotrek Koszuliński]

Tests:

1. Create tests/core/skin/ directory and move both test files related to skins there.
2. skinnochameleon.js will be a better name.
3. _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 ;).
4. 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).

[Artur Delura]

Done. Changes in the same branch.

[Piotrek Koszuliński]

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.

[Danil]

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?

[Danil]

​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?

[Piotrek Koszuliński]

​​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.

[Piotrek Koszuliński]

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.

[Danil]

And this is now true.

So, according to my jsfiddle demo, this is false. So, skin must always define chameleon property.

[Piotrek Koszuliński]

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.

[Danil]

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.

[Piotrek Koszuliński]

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?

[Danil]

Yes, that what I talking about )

null, or maybe false. And () => '' for backwards compatibility.

[Piotrek Koszuliński]

Ok, I'll update the docs. Thanks for checking this!

[Piotrek Koszuliński]

I fixed the docs today: ​https://github.com/ckeditor/ckeditor-docs/commit/9aef2552588f73d9d9c008fe66ff80ad78637326.

[Danil]

Nice! Thank you Reinmar.