Allow on justification for styled content in BR mode

[msamsel]

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?

• Add more strict checking if justification can be applied in current place
• Correct unit test for justification; assertCommandState is run asynchronously with setTimeout, so everything after that in test case was not checked

Close #1479

[jacekbogdanski]

Based on branch name the corresponding issue is #1479, although please update PR description with

Closes #1479

information. I could do it myself, however, would like to make sure if it's the correct and only one ticket which should be resolved with this PR.

[jacekbogdanski]

1. The manual test (step with selecting the whole list) is failing on Safari. However, it looks like a different issue (with path selection). If so, it requires a dedicated issue ticket.
2. This PR is pointing on the invalid upstream branch. Please, change it into master.
3. Could you simplify the manual test name? workscorrectinbrmode is very hard to read / type and I think that brmode would be sufficient. We can assume that manual tests checks if something works correct, so prefix doesn't provide additional information.

[jacekbogdanski]

Taking into an account that JavaScript is dynamic typing language, it's better to stick to EAFP rather than LBYL. Your check may introduce some issues when someone will use assertCommandState function with invalid callback type - the test will be fail positive. The issue can be mitigated a bit if you don't check callback type, just execute it if it exists.

[jacekbogdanski]

Actually, that's a pity that this function is asynchronous. It looks like this is a fix for #2369. It looks fine for me on Firefox without timeouts, but if they are really required, it should be done in normal command workflow by using
https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_editor.html#event-afterCommandExec event.

If you make assertion synchronous, you will be able to get rid of the callback. If the code really needs to be done in an asynchronous manner, it should be moved from assertion function to test cases / additional command execution helper. The assertion should just assert things, that's it.

[jacekbogdanski]

Please, merge also variables inside assertion function into one:

var leftCmd = editor.getCommand( 'justifyleft' );
var rightCmd = editor.getCommand( 'justifyright' );
var centerCmd = editor.getCommand( 'justifycenter' );
var justifyCmd = editor.getCommand( 'justifyblock' );

Let's leave this code better than we found it.

[jacekbogdanski]

So deep nesting gets unreadable easily. You could avoid "callback hell" by making assertion function synchronous.

[msamsel]

After consultation with @mlewand, assertion are now transformed to promises with library used for PFW tests.

[jacekbogdanski]

The tests are now much more complicated in comparison to the synchronous version. The whole complication is caused by the callback which has been added as a fix (#2369) for failing tests on MacOS Firefox (tests fails randomly around 30% fails) on the built version.

I have an impression that we are turning the small, incorrect fix into complicated workaround which will grow like a snowball if we don't stop it now and get back into the original issue.

I propose to revert fix from #2369 by removing introduced timeout and making the tests synchronous again. The tests will be simpler without the need for 3rd party library. Of course, tests will start failing on FF - they probably should be just ignored until we find a better solution for #2349

After our talk on Slack, I know that we don't agree totally on this case, so I think that it's a good moment to invite someone to a discussion to get a fresh opinion.

[msamsel]

During this fix I realized that proposed solution in #2369 is incorrect (It doesn't allow on multiple assert in test cases, as after first one test is ended and further timeout's are not checked).
That's why I modify this fix, to have sure that all assertion are checked.
This 3rd party lib was already used in PFW tests, and now is just exposed wider, to be used in multiple places.

I don't think, we should spend additional time on this issue. As we don't have real bug here, just some quirk in browser. Quirk which impact on test case only.
Also promises does not complicate code too much, as they preserve linear code execution in this case. Another thing is that most of the further test probably will be just copy-paste from current one, so it won't be too difficult to add new test.

That's why I don't see any snowballs here. From my perspective it's waste of time to search for other simpler synchronous solution which will be better and in the same moment it doesn't have any real impact for the users. I don't think we should research more and waste time for it, when there is perfectly working promise-based approach.

@mlewand could you give your opinion for this case?

@jacekbogdanski do you have any other review comments about this PR?

[jacekbogdanski]

I'm still really not sure about these changes. Be aware that any code requires maintenance. More code and complications, more work for a future developer. I don't propose that we need to start searching immediately for the fix for FF. It's just better to ignore the test case for the single browser rather than complicate it by asynchronous timeouts.

[mlewand]

for failing tests on MacOS Firefox (tests fails randomly around 30% fails) on the built version.

I'll just clarify that this is related to situation before #2369.

I don't find the solution from #2369 pretty nor solid. But it does mitigate the issue. Based on what I can see proper fix will take noticeable amount of time.

I don't think that adding promises to the mix increase the complexity, except for the internal code of wrapPromises, which uses funky Q interface. But you're not concerned with this except this particular PR. Also as mentioned we already use it in PFW, so there's a chance one will be already familiar with it.

I'll just mention that we should be adding some reasonable promises polyfill (at least in the tests) some time soon, and it will be a lib that have standard promises compliant API. We'll remove Q with this, so the above will no longer be an issue.

Considering both proposals:

• Reverting it and ignoring tests on firefox will leave us with no testing on FF (realistically speaking for a long time, maybe forever). So no protection from accidental regressions.
• Keeping this code and restructuring it, gives us protection against regression and a challenge to reorganize code.

Current implementation notes

I wonder whether it would improve readability if we added a wrapper for bender.editorBot.create that would return a promise too.

So the tests would become cleaner:

// Note, assertCommandState should be changed to accept bot as parameter instead of editor instance, and its returned promise
// should resolve to bot value (just to simplify the API).
// wrapPromises -> promisifyCase
	// #455
	'test alignment on disabled elements div mode': promisifyCase(
		promiseBotCreate( {
			name: 'editor_div_1',
			creator: 'inline',
			config: {
				plugins: 'justify,toolbar,divarea',
				allowedContent: 'div ul{text-align};li;',
				enterMode: CKEDITOR.ENTER_DIV
			}
		} )
		.then( function( bot ) {
			bot.setHtmlWithSelection( '<div>Foo</div><ul><li>on^e</li><li>two</li><li>three</li></ul>' );

			// Note: the API of assertCommandState should be changed, so the call is simply:
			// return assertCommandState( 0, 0, 0, 0, bot );
			return assertCommandState( 0, 0, 0, 0, editor )();
		} )
		.then( function( bot ) {
			bot.setHtmlWithSelection( '<div>F^oo</div><ul><li>one</li><li>two</li><li>three</li></ul>' );

			return assertCommandState( 1, 2, 2, 2, editor )();
		} );
	),
// ---- vs ----
	// #455
	'test alignment on disabled elements div mode': function() {
		bender.editorBot.create( {
			name: 'editor_div_1',
			creator: 'inline',
			config: {
				plugins: 'justify,toolbar,divarea',
				allowedContent: 'div ul{text-align};li;',
				enterMode: CKEDITOR.ENTER_DIV
			}
		}, function( bot ) {
			var editor = bot.editor;
			bot.setHtmlWithSelection( '<div>Foo</div><ul><li>on^e</li><li>two</li><li>three</li></ul>' );
			wrapPromises( assertCommandState( 0, 0, 0, 0, editor )()
				.then( function() {
					bot.setHtmlWithSelection( '<div>F^oo</div><ul><li>one</li><li>two</li><li>three</li></ul>' );
				} )
				.then( assertCommandState( 1, 2, 2, 2, editor ) ) );
		} );
	},

Summary

Given pros and cons outlined in this comment I don't see any solid reasons not to go with promise-oriented solution. I don't think it adds that much overhead (except mentioned bad Q API).

Also the code could be even further simplified as mentioned, which will make it even more pleasant to work with.

[msamsel]

I've rewrite test to be more user friendly, as it was proposed in above comment.
I combine asserts with function, which set up environment for given test. This nicely separates test into logical blocks of adjacent steps.

However it wasn't possible to fully apply proposed solution. Promises needs to be hidden inside function, to not be executed prior to test. Without it, bender start to lose track on available editors, what cause errors in tests used pre-build editor. That's why we need to guarantee that promise start to be executed only in its own test case.

[jacekbogdanski]

It would be great if we could keep this implementation as close as possible to the https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise API, so any change in 3rd party library for Promise polyfill would be much easier to integrate. I think that we could also make it much more maintainable if we avoid hard references inside test cases to Q library.

In that case, if we ever decide to provide promises into CKEditor4, tests refactoring would be as easy as:

bender.tools.promise = CKEDITOR.tools.promise;

I already prepared some POC for it, which is available at t/1479-b (bear in mind it still requires some refactoring and removing leftovers from the current implementation).

It would also be nice if we could get rid of bender-include: Q references and put this library inside bender in a single place, so it can be easily removed from the test environment if needed.

[jacekbogdanski]

It would be great if we could remove this reference and move library loading into a single place ( like test tools).

[jacekbogdanski]

I think that we should avoid hard references in tests to make this code more maintainable if we want to change promise library in the future.

[jacekbogdanski]

What do you think about renaming this member into createAsync?

[msamsel]

@jacekbogdanski, I've use your changes and I add slight modification to have working tests under IE8 (Function.prototype.bind is not supported under IE8).
Current solution seems to be pretty solid.

[f1ames]

Rebased on latest master and adjusted changelog entry.

[f1ames]

TBH it looks like quite solid solution already. Nothing to add from me apart from few very tiny details. Good job @msamsel and @jacekbogdanski.

Btw. I'm not sure if @msamsel will be able to proceed with these few changes so maybe @jacekbogdanski can take over?

[jacekbogdanski]

Btw. I'm not sure if @msamsel will be able to proceed with these few changes so maybe @jacekbogdanski can take over?

No problem, I already corrected issues pointed out in your R-.

[f1ames]

LGTM 👍