Opened 5 years ago

Closed 5 years ago

#11413 closed Bug (fixed)

Wrong execCommand behavior

Reported by: Danto Owned by: Marek Lewandowski
Priority: Normal Milestone: CKEditor 4.3.3
Component: General Version: 4.0
Keywords: Cc:

Description

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.

Change History (10)

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

Milestone: CKEditor 4.3.3
Status: newconfirmed
Version: 4.3.14.0

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.

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

comment:2 Changed 5 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

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

Please remember about checking when this has to be introduced.

comment:4 Changed 5 years ago by Marek Lewandowski

Status: assignedreview

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;
} );

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

Status: reviewreview_failed

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.

comment:6 Changed 5 years ago by Marek Lewandowski

Status: review_failedreview

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.

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

Status: reviewreview_failed

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.

comment:8 Changed 5 years ago by Marek Lewandowski

Status: review_failedreview

Rebased and repushed both t/11413 branches.

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

Status: reviewreview_passed

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

comment:10 Changed 5 years ago by Marek Lewandowski

Resolution: fixed
Status: review_passedclosed

True, the shorter the assertion name the better.

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

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