Fix disappearing undo steps after multiple selection changes #2792

msamsel · 2019-02-05T15:15:46Z

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?

Prevent of firing applyFormat command after mouse up event, when there is no stored formatting.

Closes #2780.
Closes #2655.
Closes #2470.

engineering-this

The fix looks good, just some minor things.

I've found one more case which is even worse than originally reported. It is covered by your fix, but could you add test for that case too?

plugins/copyformatting/plugin.js


 				editable.attachListener( mouseupHost, 'mouseup', function( evt ) {
-					if ( getMouseButton( evt ) === CKEDITOR.MOUSE_BUTTON_LEFT ) {
+					if ( getMouseButton( evt ) === CKEDITOR.MOUSE_BUTTON_LEFT && cmd.state === CKEDITOR.TRISTATE_ON ) {


engineering-this · 2019-02-06T12:26:43Z

tests/plugins/copyformatting/manual/undointegration.md

@@ -0,0 +1,13 @@
+@bender-ui: collapsed
+@bender-tags: bug, copyformatting, 4.11.3, trac16675


Wrong issue reference number.

Suggested change

@bender-tags: bug, copyformatting, 4.11.3, trac16675

@bender-tags: bug, copyformatting, 4.11.3, 2780

engineering-this · 2019-02-06T12:27:18Z

tests/plugins/copyformatting/manual/undointegration.md

+@bender-ckeditor-plugins: copyformatting, toolbar, wysiwygarea, undo, basicstyles
+
+## Test both editors
+1. Add some content to editor (e.g. new line), to "avctivate" undo step.


Typo.

Suggested change

1. Add some content to editor (e.g. new line), to "avctivate" undo step.

1. Add some content to editor (e.g. new line), to "activate" undo step.

engineering-this · 2019-02-06T13:16:26Z

tests/plugins/copyformatting/undointegration.js

+	};
+
+	bender.test( {
+		'test undo integration': function() {


Missing comment with ticket reference.

msamsel · 2019-02-11T11:41:36Z

I add test cases which covers "redo" issue.

engineering-this

Just few more small things.

engineering-this · 2019-02-11T14:08:21Z

tests/plugins/copyformatting/undointegration.js

@@ -11,6 +12,26 @@
 		}
 	};

+	function prepareEditor( bot ) {


Please move the function below bender.test, as we agreed to keep function declaration at the end.

I'm not sure if prepareEditor name works here. This function has plenty asserts and they aren't part of preparing editor.

engineering-this · 2019-02-11T14:12:03Z

tests/plugins/copyformatting/manual/undointegration.html

-	CKEDITOR.replace( 'editor1' );
+	CKEDITOR.replace( 'editor1', {
+		on: {
+			instanceReady: function( evt ) {


Listener function could be extracted to avoid duplicate code.
Additionally variable declarations could be grouped.

tests/plugins/copyformatting/manual/undointegration.md

engineering-this · 2019-02-11T14:25:02Z

tests/plugins/copyformatting/manual/undointegration.md

+
+## Test both editors
+1. Add some content to editor (e.g. new line), to "activate" undo step.
+2. Start to click around editor to change selection inside editor (20-25 times). Selection have to differ between adjacent steps.


I think that adding empty lines between each test step improves readability.

engineering-this · 2019-02-11T14:33:17Z

tests/plugins/copyformatting/manual/undointegration.html

+			instanceReady: function( evt ) {
+				var editor = evt.editor;
+				var currentIndicator = CKEDITOR.document.findOne( '#ed1-current' );
+				editor.editable().on( 'mouseup', function( evt ) {


evt param is unused, could be omitted.

engineering-this · 2019-02-11T14:38:25Z

tests/plugins/copyformatting/manual/undointegration.md

+1. Add some content to editor (e.g. new line), to "activate" undo step.
+2. Start to click around editor to change selection inside editor (20-25 times). Selection have to differ between adjacent steps.
+### Expected:
+Undo UI button is active. Current snapshot index does equals 1 and doesn't change with selection change.


doesn't change with selection change.

It updates only on mouse event so it's hard to tell if it's because of selection change or not. If we can't use other event like change maybe just go with interval. It will be much more convenient if index preview update weren't limited to user action.

engineering-this · 2019-02-11T14:49:22Z

tests/plugins/copyformatting/undointegration.js

+			}
+
+			assert.areSame( 1, editor.undoManager.index, 'There shouldn\'t be new undo steps and editor should remain on 1st step.' );
+			assert.isTrue( editor.undoManager.undoable(), 'Editor should has possibility to undo.' );


Suggested change

assert.isTrue( editor.undoManager.undoable(), 'Editor should has possibility to undo.' );

assert.isTrue( editor.undoManager.undoable(), 'Editor should have a possibility to undo.' );

engineering-this · 2019-02-11T14:50:15Z

tests/plugins/copyformatting/undointegration.js

+				} ) );
+			}
+
+			assert.areSame( 1, editor.undoManager.index, 'There shouldn\'t be new undo steps and editor should remain on 1st step.' );


Suggested change

assert.areSame( 1, editor.undoManager.index, 'There shouldn\'t be new undo steps and editor should remain on 1st step.' );

assert.areSame( 1, editor.undoManager.index, 'There shouldn\'t be new undo steps and editor should remain on the 1st step.' );

engineering-this

setInterval doesn't improve test, as second (after initial) snapshot is created when selection changes so it might be misleading for tester when he types something and index preview updates on click. Maybe we should just drop the preview of snapshot index and just repeat steps from originally reported issue.

msamsel · 2019-02-14T15:32:19Z

PR related to: #2811

Comandeer · 2019-02-14T16:24:23Z

I cherry-picked tests from #2811. I've also updated unit test for undo to behave correctly in IE 8 (as it uses non-standard code for left mouse button).

…st helpr to be more accurate.

… might confuse the user during testing phase.

Co-Authored-By: Comandeer <[email protected]>

… to make code simpler.

Comandeer

LGTM!

msamsel changed the base branch from major to master February 5, 2019 15:15

mlewand added the review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer. label Feb 6, 2019

mlewand requested a review from engineering-this February 6, 2019 09:27

engineering-this self-assigned this Feb 6, 2019

engineering-this suggested changes Feb 6, 2019

View reviewed changes

CKEditorBot unassigned engineering-this Feb 6, 2019

msamsel force-pushed the t/2780 branch from 66a6d07 to aadc343 Compare February 11, 2019 08:57

msamsel assigned msamsel and unassigned msamsel Feb 11, 2019

msamsel requested a review from engineering-this February 11, 2019 11:40

engineering-this self-assigned this Feb 11, 2019

engineering-this suggested changes Feb 11, 2019

View reviewed changes

CKEditorBot unassigned engineering-this Feb 11, 2019

msamsel assigned msamsel and unassigned msamsel Feb 12, 2019

msamsel requested a review from engineering-this February 12, 2019 15:30

engineering-this self-assigned this Feb 14, 2019

engineering-this suggested changes Feb 14, 2019

View reviewed changes

CKEditorBot unassigned engineering-this Feb 14, 2019

Comandeer force-pushed the t/2780 branch from a89abee to 5943004 Compare February 14, 2019 16:22

Comandeer mentioned this pull request Feb 14, 2019

Fix for focus issues in nested editables caused by copyformatting plugin #2811

Closed

2 tasks

msamsel assigned msamsel and unassigned msamsel Feb 15, 2019

msamsel requested a review from engineering-this February 15, 2019 10:29

msamsel assigned msamsel and unassigned msamsel Feb 15, 2019

engineering-this self-assigned this Feb 15, 2019

Mateusz Samsel and others added 24 commits February 19, 2019 13:25

Add fix to not refresh snapshot when there is no selected format.

d66b1ec

Correct manual test description, add unit test.

71bf1c6

Add changelog entry.

ede079b

Add issue reference, correct spelling mistake.

91c5095

Remove unreachable code.

bdccfcf

Extend manual test case with new scenario.

a4ea15a

Addunit test for redo case.

d20e91d

Add redo unit tests.

da96980

Add proper target to event.

e808244

Update test description, simplify manual test, change name of unit te…

0e03a7b

…st helpr to be more accurate.

Add unit test.

b96338f

Add manual test.

f5694fb

Include #2470 in tests.

1833554

Add changelog entries.

dcb4705

Add ticket reference to the code.

6edeceb

Fix unit test for IE8. Fix typo.

f2f0afd

Fix code style issue.

fa90cd1

Remove snapshot indicator in manual test, which works incorrectly and…

9d94ac4

… might confuse the user during testing phase.

Correct unit test, to not fail on build version.

0d72dea

Fix code style issues with CSS.

c05f7cd

Co-Authored-By: Comandeer <[email protected]>

Remove unnecessary assert, correct helper name.

050ad15

Update description of manual test.

6e76d77

Remove referrence to unused tools, remove variables and id attributes…

d63537f

… to make code simpler.

Update link in changelog entry.

a3361ff

Comandeer force-pushed the t/2780 branch from 30f6168 to a3361ff Compare February 19, 2019 12:28

Comandeer approved these changes Feb 19, 2019

View reviewed changes

Comandeer merged commit 7aee701 into master Feb 19, 2019

CKEditorBot deleted the t/2780 branch February 19, 2019 12:29

This was referenced Feb 19, 2019

Nested editable can't be focused under certain circumstances #2655

Closed

Widget nested editable triggers blur event on focus in Firefox #2470

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix disappearing undo steps after multiple selection changes #2792

Fix disappearing undo steps after multiple selection changes #2792

msamsel commented Feb 5, 2019 •

edited by Comandeer

Loading

engineering-this left a comment

This comment was marked as resolved.

engineering-this Feb 6, 2019

engineering-this Feb 6, 2019

engineering-this Feb 6, 2019

msamsel commented Feb 11, 2019

engineering-this left a comment

engineering-this Feb 11, 2019

engineering-this Feb 11, 2019

engineering-this Feb 11, 2019

engineering-this Feb 11, 2019

engineering-this Feb 11, 2019

engineering-this Feb 11, 2019

engineering-this Feb 11, 2019

engineering-this Feb 11, 2019

engineering-this left a comment •

edited

Loading

msamsel commented Feb 14, 2019

Comandeer commented Feb 14, 2019

Comandeer left a comment

		@@ -0,0 +1,13 @@
		@bender-ui: collapsed
		@bender-tags: bug, copyformatting, 4.11.3, trac16675

	1. Add some content to editor (e.g. new line), to "avctivate" undo step.
	1. Add some content to editor (e.g. new line), to "activate" undo step.

	assert.isTrue( editor.undoManager.undoable(), 'Editor should has possibility to undo.' );
	assert.isTrue( editor.undoManager.undoable(), 'Editor should have a possibility to undo.' );

	assert.areSame( 1, editor.undoManager.index, 'There shouldn\'t be new undo steps and editor should remain on 1st step.' );
	assert.areSame( 1, editor.undoManager.index, 'There shouldn\'t be new undo steps and editor should remain on the 1st step.' );

Fix disappearing undo steps after multiple selection changes #2792

Fix disappearing undo steps after multiple selection changes #2792

Conversation

msamsel commented Feb 5, 2019 • edited by Comandeer Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

engineering-this left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel commented Feb 11, 2019

engineering-this 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engineering-this left a comment • edited Loading

Choose a reason for hiding this comment

msamsel commented Feb 14, 2019

Comandeer commented Feb 14, 2019

Comandeer left a comment

Choose a reason for hiding this comment

msamsel commented Feb 5, 2019 •

edited by Comandeer

Loading

engineering-this left a comment •

edited

Loading