Providing fallback CSS for inlinetoolbar #1136

msamsel · 2017-11-08T14:48:01Z

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

What changes did you make?

Adding css for inlinetoolbar and balloonpanel (inline toolbar require it to proper work). CSS base on moono-lisa skin.

I thought to force loading moono-lisa as fallback, but it might be confusing if user will remove those css folders thinking that are unnecessary for his configuration. That's why I add -default suffix and separate files.

f1ames

Manual test is failing, inline toolbar is not working:

Also I don't think copying whole skin for test is a good idea, maybe it will be possible to overwrite appendStyleSheet method to just not load skin CSS file for inlinetoolbar?

This task is about inlinetoolbar and from what I recall the default CSS was supposed to be added only for inlinetoolbar plugin. If there are some additional styles from balloonpanel needed they probably should be placed in inlinetoolbar CSS file too.

Would it be possible to add unit test, which checks if specific styling was applied to inline toolbar (so basically two test, one with default CSS and one with skin)?

f1ames · 2017-11-10T11:46:15Z

plugins/inlinetoolbar/skins/inlinetoolbar-default.css

@@ -0,0 +1,61 @@
+/*


I would simply name this file default.css as it is already in the inlinetoolbar directory so the prefix seems redundant.

f1ames · 2017-11-10T12:05:22Z

tests/plugins/inlinetoolbar/manual/fallbackstyles.html

+	<p>&nbsp;</p>
+	<p>&nbsp;</p>
+</div>
+


You could also add inline editor here.

msamsel · 2017-11-14T12:51:38Z

@mlewand what do you think about that?

This task is about inlinetoolbar and from what I recall the default CSS was supposed to be added only for inlinetoolbar plugin. If there are some additional styles from balloonpanel needed they probably should be placed in inlinetoolbar CSS file too.

Inline toolbar inheritance styles from balloon panel related to:

drawing triangle below/above balloon
ballon position (like setting proper display mode, z-index, etc.)

msamsel · 2017-11-15T11:54:44Z

I haven't notice that after rebase there was change in API so create method became attach.
I've made al changes suggested in your review. I extract from balloonpanel only important styles (from perspective of inlinetoolbar).

I have to prepare 2 separate test suites with css. Unfortunately skins cannot be changed after loading, so I have to made proper thing at the very begging of each test suite.

f1ames

Unfortunately, there are some failing tests in Edge/IE realm:

Edge/IE11/IE10

One unit test failing:

IE9

Unit test failing:

And manual one as it seems default CSS is always used:

IE8

Both unit tests failing:

And manual to as the default skin CSS seems to be always used over specific skin CSS:

The manual tests on which I checked how default CSS works with specific skin are tests/plugins/inlinetoolbar/manual/[kama|moono|moono-lisa].

f1ames · 2017-11-16T07:35:07Z

tests/plugins/inlinetoolbar/cssdefault.js

+
+( function() {
+	'use strict';
+	// Please be aware that entire test suite has to use the same skin. That's why tests are separate between 2 files.


I think this comment is unnecessary. As it makes sense in the context of the whole PR, when someone will look at the test itself its lacks this context (same for the other test).

f1ames · 2017-11-16T07:52:36Z

tests/plugins/inlinetoolbar/cssdefault.js

+		}
+	};
+
+	// We need to set up skin at the very beginning befor files start to load.


Typo "before".

f1ames · 2017-11-16T07:53:03Z

tests/plugins/inlinetoolbar/cssdefault.js

+	// Overwrite prototype to simulate missing css file.
+	var oldFn = CKEDITOR.dom.document.prototype.appendStyleSheet;
+	CKEDITOR.dom.document.prototype.appendStyleSheet = function( cssFileUrl ) {
+		// Simualate missing css in skin.


Typo in "Simulate".

f1ames · 2017-11-16T07:59:49Z

tests/plugins/inlinetoolbar/cssdefault.js

+		if ( cssFileUrl.indexOf( 'inlinetoolbar.css' ) > -1 || cssFileUrl.indexOf( 'balloonpanel.css' ) > -1 ) {
+			cssFileUrl = '';
+		}
+		oldFn.call( CKEDITOR.document, cssFileUrl );


You could simplify it a little:

if ( !cssFileUrl.match( /(inlinetoolbar|balloonpanel)\.css/ ) ) { oldFn.call( CKEDITOR.document, cssFileUrl ); }

no need to call original function with empty file path.

f1ames · 2017-11-16T08:00:40Z

tests/plugins/inlinetoolbar/cssskin.js

+	CKEDITOR.skinName = 'moono';
+
+	var tests = {
+		'test check default css usage': function( editor ) {


Should it be test check specific|moono skin css usage?

f1ames · 2017-11-16T08:01:43Z

tests/plugins/inlinetoolbar/manual/fallbackstyles.html

+			cssFileUrl = '';
+		}
+
+		if ( this.$.createStyleSheet ) {


We didn't you use call to original function here same as in unit test?

I've made it earlier with stub and forget about it. I think option with calling origin function is simpler, and can be resistant for possible method changes.
I've update this to looks like in Unit test.

f1ames · 2017-11-16T08:04:20Z

tests/plugins/inlinetoolbar/manual/fallbackstyles.md

+@bender-ckeditor-plugins: wysiwygarea,toolbar,basicstyles,floatingspace,inlinetoolbar,link,image,resize,language,stylescombo
+
+Perform below steps in both editors:
+1. Click into image.


Click on image or just Click image.

msamsel · 2017-11-16T14:16:46Z

As I talk with @mlewand we currently should focus on most popular browsers, and older IE left for further improvements. That's why I ignore tests on IE below 11.
Additional info, all those errors which looks like default.css are used in IE8 etc:

And manual one as it seems default CSS is always used:

Those are also happen on t/933 without introducing default.css. So it's not depended from my change. It's cause just by inline toolbar which doesn't have special styleset for IE8 which replace linear-gradients (or other not supported styles).

I have add different assertion for the IE envs to have sure that "transparent" will be matched.

I also a little tune our tools method, which converts rgb to hex to also support rgba. If you consider it as separate issue, give me an info I'll extract it, but this might extend realisation of this issue. As this introduce dependancy here. Another option would be creating specific function just for those test purposes, but I'm not think is a good option, because this will duplicate the code.

f1ames

As I talk with @mlewand we currently should focus on most popular browsers, and older IE left for further improvements. That's why I ignore tests on IE below 11.

👍

Additional info, all those errors which looks like default.css are used in IE8 (...) are also happen on t/933 without introducing default.css.

Ok, haven't checked that, in this case it ok to leave it as is 👍

I also a little tune our tools method, which converts rgb to hex to also support rgba. If you consider it as separate issue, give me an info I'll extract it, but this might extend realisation of this issue.

It should be extracted, because it affects much wider part of the code. convertRgbToHex method is used together with CSS normalization which is used in many places in CKEditor. Also you can see that one unit test is failing:

The other problem is that your solution just ignores the alpha part, always producing hex representation from rgb part. But two colors with different transparency (e.g. rgba(150, 150, 150, 1) and rgba(150, 150, 150, 0.5)) are just different colors and their hex representation is also different.

Another option would be creating specific function just for those test purposes, but I'm not think is a good option, because this will duplicate the code.

Usually it is not the best approach, however I would go this way in this case. As we need this specific functionality only in this test you could create a function which will filter rgba and pass only rgb to convertRgbToHex function (regex or some split/replace).

You could extract it to some helper as it will be used in both unit tests. I would also place appendStyleSheet stub (https://github.com/ckeditor/ckeditor-dev/pull/1136/files#diff-497cd1ab5d84223f82e739984c6dd387R64) in this helper file as it is also used in two test.

f1ames

Almost there;)

f1ames · 2017-11-20T16:18:00Z

plugins/inlinetoolbar/skins/default.css

+	margin-left: -20px;
+}
+
+.cke_balloon.cke_inlinetoolbar


This is the same selector as in line 6 (https://github.com/ckeditor/ckeditor-dev/pull/1136/files#diff-a222e1835c2e1c3b856fdaf98d45e2faR6), it should be merged into one rule.

f1ames · 2017-11-20T16:19:01Z

plugins/inlinetoolbar/skins/default.css

+}
+
+/* align: [ hcenter ] */
+.cke_balloon.cke_inlinetoolbar .cke_balloon_triangle_outer.cke_balloon_triangle_align_hcenter


This is the same selector as in line 28 (https://github.com/ckeditor/ckeditor-dev/pull/1136/files#diff-a222e1835c2e1c3b856fdaf98d45e2faR28), it should be merged into one rule.

f1ames · 2017-11-20T16:28:18Z

tests/plugins/inlinetoolbar/_helpers/default.js

+
+'use strict';
+
+function replaceAppendStyleSheet() {


This looks good, however it doesn't provide a possibility to clean up after running tests (restore original function). You could do two things:

This function might return a function which will restore original one when called:

return function() { CKEDITOR.dom.document.prototype.appendStyleSheet = oldFn; }

Use sinon stub which has stub.restore() method.

It is good practise to always clean up after test run (especially for unit tests, for manual you may skip it). So after changing it, please add clean up to unit test (you may use tearDown function).

I decided to not use sinon.stub, because I wasn't able to find possibility to fire origin function within stub.
In documentation I found only possibility how to replace the method, with own function.
That's why I wrote some similar wrapper which could execute native method. I'll extend it a little bit to restore native method.

It works fine but it's a little over complicated for me. You have to create new instance, it has two methods which you have to call, etc. plus you reinvented a wheel (well, a part of it:D).

In documentation I found only possibility how to replace the method, with own function.

That's exactly where you can call the original function, the stub could look like this:

function stubAppendStyleSheet() { var originFn = CKEDITOR.dom.document.prototype.appendStyleSheet; return sinon.stub( CKEDITOR.dom.document.prototype, 'appendStyleSheet', function( cssFileUrl ) { // Simulate missing css in skin. if ( !cssFileUrl.match( /(inlinetoolbar|balloonpanel)\.css/ ) ) { originFn.call( CKEDITOR.document, cssFileUrl ); } } ); }

and then you can use it like:

var stub = stubAppendStyleSheet(); CKEDITOR.once( 'instanceReady', stub.restore );

It is shorter and simpler IMHO. Plus for everybody who knows sinon, you just take a look at the usage and it should be clear that these two lines stubs appendStyleSheet method and then restores it. With custom code you always have to dive in to check what's going on.

msamsel · 2017-11-21T11:19:32Z

I can use neither setUp nor tearDown. Those methods are executed too late. Skins are loaded when global CKEDITOR object is created and are applied to all editors at given page. That's why test cases are separated between 2 files, and that's why I cannot use it in setUp. Because those methods are executed after main lib is fully loaded.
That's why I made cleaning up in a different approach inside an event, when editor should have all css loaded, so modification shouldn't be required any longer.

f1ames

Check this comment: #1136 (comment).

f1ames

LGTM 👍

…d intgrate everything inside inlinetoolbar.

… custom skin.

… history.

Providing fallback CSS for inlinetoolbar

f1ames self-requested a review November 10, 2017 10:38

f1ames suggested changes Nov 10, 2017

View reviewed changes

mlewand changed the title ~~Providing fallback css for inlinetoolbar~~ Providing fallback CSS for inlinetoolbar Nov 13, 2017

msamsel force-pushed the t/933-3148 branch from 0526634 to 4bc0987 Compare November 15, 2017 11:47

msamsel requested a review from f1ames November 15, 2017 11:55

msamsel force-pushed the t/933-3148 branch from 4bc0987 to 736abe7 Compare November 15, 2017 12:23

f1ames suggested changes Nov 16, 2017

View reviewed changes

msamsel requested a review from f1ames November 16, 2017 14:21

f1ames suggested changes Nov 17, 2017

View reviewed changes

msamsel force-pushed the t/933-3148 branch from eda5ef3 to e682277 Compare November 20, 2017 12:01

msamsel requested a review from f1ames November 20, 2017 12:15

f1ames suggested changes Nov 20, 2017

View reviewed changes

msamsel requested a review from f1ames November 21, 2017 11:23

f1ames suggested changes Nov 21, 2017

View reviewed changes

msamsel requested a review from f1ames November 21, 2017 14:07

f1ames approved these changes Nov 23, 2017

View reviewed changes

Mateusz Samsel and others added 10 commits November 23, 2017 10:30

Providing default css for inlinetoollbar.

d723fab

Manual test checking fallback css.

eee92cf

Review: fix css name to be 'default'. Remove css from balloonpanel an…

ef01433

…d intgrate everything inside inlinetoolbar.

Review fix: manual test now use modified appendStyle instead of using…

bc169f9

… custom skin.

Review fix: adding unit test for default css for inlinetoolbar.

2fdbc3c

Review fixes: mostly typos, some minor code optimization.

0f15481

Review fixes: ignroe test cases on old IE. Adapt assertion for Edge/IE.

63d465b

Review fixes: extract helpers method, remove tools modifications from…

ea94125

… history.

Code formatting fix.

f318f09

Review fixes: css improvements, test helpers modification.

89a1911

Redesign helper to use sinon.stub.

72a4c19

f1ames force-pushed the t/933-3148 branch from 7301274 to 72a4c19 Compare November 23, 2017 09:30

f1ames merged commit d626f55 into t/933 Nov 23, 2017

f1ames deleted the t/933-3148 branch November 23, 2017 09:31

mlewand pushed a commit that referenced this pull request Nov 29, 2017

Merge pull request #1136 from ckeditor/t/933-3148

5c60258

Providing fallback CSS for inlinetoolbar

mlewand mentioned this pull request Dec 4, 2017

Balloon Toolbar #933

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Providing fallback CSS for inlinetoolbar #1136

Providing fallback CSS for inlinetoolbar #1136

msamsel commented Nov 8, 2017 •

edited

Loading

f1ames left a comment

f1ames Nov 10, 2017

f1ames Nov 10, 2017

msamsel commented Nov 14, 2017

msamsel commented Nov 15, 2017

f1ames left a comment

f1ames Nov 16, 2017

f1ames Nov 16, 2017

f1ames Nov 16, 2017

f1ames Nov 16, 2017

f1ames Nov 16, 2017

f1ames Nov 16, 2017

msamsel Nov 16, 2017

f1ames Nov 16, 2017

msamsel commented Nov 16, 2017 •

edited

Loading

f1ames left a comment

f1ames left a comment

f1ames Nov 20, 2017

f1ames Nov 20, 2017

f1ames Nov 20, 2017

msamsel Nov 21, 2017

f1ames Nov 21, 2017

msamsel commented Nov 21, 2017

f1ames left a comment

f1ames left a comment

Providing fallback CSS for inlinetoolbar #1136

Providing fallback CSS for inlinetoolbar #1136

Conversation

msamsel commented Nov 8, 2017 • edited Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel commented Nov 14, 2017

msamsel commented Nov 15, 2017

f1ames left a comment

Choose a reason for hiding this comment

Edge/IE11/IE10

IE9

IE8

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel commented Nov 16, 2017 • edited Loading

f1ames left a comment

Choose a reason for hiding this comment

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel commented Nov 21, 2017

f1ames left a comment

Choose a reason for hiding this comment

f1ames left a comment

Choose a reason for hiding this comment

msamsel commented Nov 8, 2017 •

edited

Loading

msamsel commented Nov 16, 2017 •

edited

Loading