Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#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 4 years ago by Danil

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

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

Milestone: CKEditor 4.4.7
Status: newconfirmed

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 4 years ago by 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.

comment:4 Changed 4 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:5 Changed 4 years ago by 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).

comment:6 Changed 4 years ago by 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

comment:7 Changed 4 years ago by 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.

Last edited 4 years ago by Artur Delura (previous) (diff)

comment:8 Changed 4 years ago by Artur Delura

Status: assignedreview

Changes and tests in branch:t/12387.

comment:9 Changed 4 years ago by 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).

comment:10 Changed 4 years ago by Artur Delura

Status: reviewassigned

comment:11 Changed 4 years ago by Artur Delura

Status: assignedreview

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.

Last edited 4 years ago by Artur Delura (previous) (diff)

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

Status: reviewreview_failed

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

comment:13 Changed 4 years ago by Artur Delura

Status: review_failedreview

Done. Changes in the same branch.

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

Resolution: fixed
Status: reviewclosed

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 4 years ago by 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?

comment:16 Changed 4 years ago by 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?

comment:17 Changed 4 years ago by 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.

Last edited 4 years ago by Piotrek Koszuliński (previous) (diff)

comment:18 Changed 4 years ago by 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.

comment:19 Changed 4 years ago by Danil

And this is now true.

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

comment:20 Changed 4 years ago by 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.

Last edited 4 years ago by Piotrek Koszuliński (previous) (diff)

comment:21 Changed 4 years ago by 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.

comment:22 Changed 4 years ago by 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?

comment:23 Changed 4 years ago by Danil

Yes, that what I talking about )

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

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

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

comment:26 Changed 4 years ago by Danil

Nice! Thank you Reinmar.

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