Opened 3 years ago

Closed 3 years ago

#13771 closed Bug (fixed)

contents.css not built into ckeditor.js

Reported by: Sergiy Kuzmenko Owned by: Tomasz Jakut
Priority: Normal Milestone: CKEditor 4.5.5
Component: General Version:
Keywords: Cc:

Description (last modified by Marek Lewandowski)

Steps to reproduce

  1. Download my custom build: http://ckeditor.com/builder/c21338e58b4ef94bfe0b9aff003760dd
  2. Unzip and grep -r contents.css *
  3. contents.css is not present in ckeditor.js

On the contrast if downloading a standard build ckeditor.js contains a like like this: CKEDITOR.config.contentsCss=CKEDITOR.getUrl("contents.css");

Expected result

Since my build includes Rich Combo and plugins which require contents.css it is expected to be part of the build. Without contents.css all plugins based on Rich Combo use browser default font style.

Change History (14)

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

Component: Project : CKBuilderGeneral
Milestone: CKEditor 4.5.4
Status: newconfirmed
Version: 4.5.3

Confirmed. Interesting issue. I don't think it's related top the builder, but rather to the chosen plugins. Somehow, missing one of them makes dropdowns not aware of contents.css. It may also be something related to the skin, so that should be checked too.

comment:2 Changed 3 years ago by Sergiy Kuzmenko

Probably not the skin: downloading a standard build with Moono Dark works fine. Custom builds do include contents.css, it's just not built into ckeditor.js

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

The contents.css file is never built into ckeditor.js. It's loaded automatically though if everything works fine.

comment:4 Changed 3 years ago by Sergiy Kuzmenko

Here's what I get:

  1. content.css is always present in the build (whether its standard or custom build)
  2. The difference is ckeditor.js content:

Standard build:

$ grep contents.css ckeditor.js 
this.editor,d=a.document,a=a.window.getFrame();j.baseProto.detach.call(this);this.clearCustomData();d.getDocumentElement().clearCustomData();a.clearCustomData();CKEDITOR.tools.removeFunction(this._.frameLoadedHandler);(d=a.removeCustomData("onResize"))&&d.removeListener();a.remove()}}})})();CKEDITOR.config.disableObjectResizing=!1;CKEDITOR.config.disableNativeTableHandles=!0;CKEDITOR.config.disableNativeSpellChecker=!0;CKEDITOR.config.contentsCss=CKEDITOR.getUrl("contents.css");(function(){function e(b,a){a||(a=b.getSelection().getSelectedElement());if(a&&a.is("img")&&!a.data("cke-realelement")&&!a.isReadOnly())return a}function f(b){var a=b.getStyle("float");if("inherit"==a||"none"==a)a=0;a||(a=b.getAttribute("align"));return a}CKEDITOR.plugins.add("image",{requires:"dialog",init:function(b){if(!b.plugins.image2){CKEDITOR.dialog.add("image",this.path+"dialogs/image.js");var a="img[alt,!src]{border-style,border-width,float,height,margin,margin-bottom,margin-left,margin-right,margin-top,width}";

Custom build:

$ grep contents.css ckeditor.js
<nothing>

This definitely looks like a build issue to me. Especially that at runtime there's no attempt to load contents.css.

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

That's exactly the question - which plugin loads contents.css and what to do if it's not enabled in one's editor.

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

Milestone: CKEditor 4.5.4CKEditor 4.5.5

comment:7 Changed 3 years ago by Tomasz Jakut

Owner: set to Tomasz Jakut
Status: confirmedassigned

comment:8 Changed 3 years ago by Tomasz Jakut

Status: assignedreview

The problem was caused by the fact that contents.css was loaded only if wysiwygarea plugin was present. Without it that file wouldn't be available.

The fix is rather simple: move the config entry for contents.css from plugin to the main config file - it's on branch:t/13771.

comment:9 Changed 3 years ago by Marek Lewandowski

Description: modified (diff)
Status: reviewreview_failed

#13771

Fix code is fine, while tests could definitely be a little simplified. E.g.

  • there's no point in making name var, as it's used only once, and it does not improve readability. Same goes for bot, just compare:
'check if font size combo loads proper stylesheet': function() {
	var bot = this.editorBot,
		name = 'FontSize';

	bot.combo( name, function( combo ) {
		var isPresent = searchForStylesheet( combo );

		assert.isTrue( isPresent, 'check if stylesheet is present in font size combo\'s iframe' );
	} );
},

to:

'check if font size combo loads proper stylesheet': function() {
	this.editorBot.combo( 'FontSize', function( combo ) {
		var isPresent = searchForStylesheet( combo );

		assert.isTrue( isPresent, 'check if stylesheet is present in font size combo\'s iframe' );
	} );
},

isPresent is a different case here as

var isPresent = searchForStylesheet( combo );
assert.isTrue( isPresent, 'check if stylesheet is present in font size combo\'s iframe' );

might actually be more readable than:

assert.isTrue( searchForStylesheet( combo ), 'check if stylesheet is present in font size combo\'s iframe' );

So I can see how it might improve readability - that being said I'm fine with one way or another in this particular case.

  • Stay DRY, extract searchForStylesheet function to _helper/tools.js in your tests directory.
  • Utilize advantage of our CKEDITOR.dom.* wrappers, initial lines in searchForStylesheet method might be simplified down to:
var frameDoc = combo._.panel.element.getFirst().getFrameDocument(),
	stylesheets = frameDoc.find( 'link[rel="stylesheet"]' ).$;

Not only that, using this you no longer need to check rel attribute.

Then you can actually think about rewriting it to:

function searchForStylesheet( combo ) {
	var frameDoc = combo._.panel.element.getFirst().getFrameDocument();
	// Look for any stylesheet link elems that end with desired CSS file.
	return Boolean( frameDoc.findOne('link[rel="stylesheet"][href$="contents.css"]') );
}

I'd suggest you just to go through most important namespaces now:

Then read other CKEDITOR.dom.* member docs the other day! :)

  • It's great that you've thought about multiple editor creators! You can do it way simpler, without code duplication. Use bender.editors property, for example usage see table plugin tests.

comment:10 Changed 3 years ago by Tomasz Jakut

Status: review_failedreview

Ok, I have simplified the tests & pushed them to branch:t/13771

comment:11 Changed 3 years ago by Marek Lewandowski

Description: modified (diff)

I've accidentally modified descr, reverting.

comment:12 Changed 3 years ago by Marek Lewandowski

Status: reviewreview_failed
  1. Prefix your test method names with test. The thing is that it might catch you by surprise that your test won't be considered by Bender. It's because YUI will use as TC only methods that starts with "test". Today I learned that it will also include if it includes a space :) But we're using it with test prefix.
  2. Unfortunately you have misused test methods it a little bit.

Without going into too much details: bot.combo method is an asynchronous test, as such it uses YUI's wait() call. The funny (and hard to predict) implementation of this method is that it throws an YUI.YUITest.Wait exception so this way you'r code wont ever do any further executin in for ( var bot in bots ) { loop.

So what you need to have is a separate function for each editor. You can do that with bender.tools.createTestsForEditors helper method. The drawback here is that because of that you have a different cxontext object in your test cases, and this object does not have properties such as editorBots as we normally have in tests. So we need to use global bender variable to obtain editor bot.

Pushed fixes to t/13771, but we can simplify it even more once #13859 is done.

comment:13 Changed 3 years ago by Tomasz Jakut

Status: review_failedreview

Simplified tests thanks to #13859 & pushed branch:t/13771.

comment:14 Changed 3 years ago by Marek Lewandowski

Resolution: fixed
Status: reviewclosed

GJ, merged to master with git:8eb2434.

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