Wrong execCommand behavior

[Danto]

See: ​https://github.com/ckeditor/ckeditor-dev/blob/master/core/editor.js#L821

 // This condition is never false
 if ( this.fire( 'beforeCommandExec', eventData ) !== true ) {
 ...
 }

If beforeCommandExec returns false, execCommand should stop the execution. But : if(false !== true) is true. Also afterCommandExec:

  if ( !command.async && this.fire( 'afterCommandExec', eventData ) !== true )

Fix it by just remove the " !== true " check.

[Piotrek Koszuliński]

I'm not sure right now but it seems that in CKE 4.0 we changed value returned by fire() when event is cancelled. So to check whether event has not been cancelled we need to compare returned value with !== false.

A part of the ticket is to find when and what has been changed. And also we should check whether we don't have other fire() !== true leftovers.

Additionally, documentation for beforeCommandExec and fire should be clarified.

[Piotrek Koszuliński]

Please remember about checking when this has to be introduced.

[Marek Lewandowski]

Solution pushed to ​t/11413 at dev and t/11413 at tests.

Value returned by fire() when event was canceled was modified in ​git:55e02f7 - which was first introduced with 4.0 version.

beforeCommandExec and afterCommandExec events were mainly used in undo and scayt plugins, but they don't return false value.

Checked chrome, ie8, ie11 and all tests are green.

Note that with current solution one still may cancel event with following construction, if he assigns null to evt.data:

evt.editor.on( 'beforeCommandExec', function( evt, stopEvent, cancelEvent ) {
	evt.data = null;
	// or evt.data = 0;
} );

[Piotrek Koszuliński]

Result of firing beforeCommandExec should be compared to !== false. It will avoid the situation when setting data to other falsy value will cancel execution. See:

grep -R "!== false" core/* plugins/* | grep fire

---

What's more, there is one more fire() call which may need to be updated as well:

grep -R "== true" core/* plugins/* | grep fire

If it needs to be fixed, let's do it now.

---

And the last thing - tests are overcomplicated and unnecessarily check CKEDITOR.event behaviour. We should only add test whether command execution is correctly stopped if events are cancelled and tests should be easy to read. Let's have these tests in dt/command/events.html.

[Marek Lewandowski]

If we care about these falsy values there will be still possibility to stop execution with following assignment:

event.data = false;

But as far as i see it is an alternative way to cancel event, so in that case it'll be fine.

---

In this case we should also take care of results of this query

grep -r '![a-zA-Z0-9\.]\{1,\}.fire' core/* plugins/*

I've fixed them with ​git:9c8881af73

---

Are you sure that you want to reduce these tests? Currently we test all three aspects of editor#execCommand(), which is:

• beforeCommandExec
• command execution itself
• afterCommandExec

Skipping these tests will leave both beforeCommandExec and afterCommandExec firing untested.

[Piotrek Koszuliński]

The first three tests cases are ok. Checking valid return values and returning false is too much. Of course tests are correct, but there's no need for them. Tests have to be simple to read and verify. If there's too much of them or too much APIs/mocukping/settingup etc. involved then you need to read a lot to understand a simple thing. And those tests which we need could be implemented in 3x less LOC, thus will need 3x less time to be understood.

So:

1. Less tests cases - test what matters.
2. KISS - who needs values of assertEventsCount to default to 0?! This method should has 3 lines.
3. Helpers should be defined before tcs.
4. Remove allowedContent flags - not needed in these tests.
5. Move tests to dt/command/events.html
6. Remove empty lines after {\n.

PS. Dev changes are ok.

[Marek Lewandowski]

Rebased and repushed both t/11413 branches.

[Piotrek Koszuliński]

R+, but please change areEqual( true/false, sth ) to isFalse/isTrue.

[Marek Lewandowski]

True, the shorter the assertion name the better.

Fixed with ​git:d571f704529 in dev and 2e421c2d6a in tests.