Fix overwriting text on paste #4356

Comandeer · 2020-11-03T15:55:31Z

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches that 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

Did you follow the CKEditor 4 code style guide?

Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.

PR is consistent with the code style guide

What is the proposed changelog entry for this pull request?

* [#4301](https://github.com/ckeditor/ckeditor4/issues/4301): Fixed: pasted content overwritten when pasted in initially empty editor with [`div` enter mode](https://ckeditor.com/docs/ckeditor4/latest/features/enterkey.html).

What changes did you make?

I've removed the invocation of getData() and replaced it with direct checking of editable's HTML.

Which issues does your PR resolve?

Closes #4301.

github-actions · 2020-11-11T09:09:08Z

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

jacekbogdanski · 2020-11-13T07:26:27Z

Rebased onto the latest master.

jacekbogdanski

Looks good, just some minor changes.

jacekbogdanski · 2020-11-12T13:21:43Z

core/editable.js

+				// Instead of getData method, we directly check the HTML
+				// due to the fact that interal getData operates on latest snapshot,
+				// not the current content (#4301).
+				isEmptyEditable = html === '' || html.match( emptyParagraphRegexp );


Is the emptyParagraphRegexp check here because it's still possible to omit upcasting p to div with unfiltered_html option? If so, we need unit tests covering this case. Not sure if it's possible to do it manually. Did you manage to reproduce with normal editor usage?

Actually emptyParagraphRegexp searches for empty div, p and several other elements. It's not a paragraph as an element, but just as a block of text.

Thanks for explaining that, but still we are missing unit and manual tests for that case. I just wanted to add them quickly during this review, but find out that I'm actually not able to create any easily reproducible test case for that scenario. Could you update your PR with these tests for empty container elements? If it's just defensive programming, it would be still good to have some spying tests instead.

core/editable.js

jacekbogdanski

I see that test /tests/core/editable/manual/enterdiv is failing on Chrome - but it seems like it's also failing on master branch. Not sure if the test started to failing or it has been poorly written. Maybe do you remember @Comandeer how this test should behave?

jacekbogdanski · 2020-11-20T14:10:43Z

core/editable.js

+				// Instead of getData method, we directly check the HTML
+				// due to the fact that interal getData operates on latest snapshot,
+				// not the current content (#4301).
+				isEmptyEditable = html === '' || html.match( emptyParagraphRegexp );


Thanks for explaining that, but still we are missing unit and manual tests for that case. I just wanted to add them quickly during this review, but find out that I'm actually not able to create any easily reproducible test case for that scenario. Could you update your PR with these tests for empty container elements? If it's just defensive programming, it would be still good to have some spying tests instead.

github-actions · 2020-11-28T09:11:01Z

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

Comandeer · 2020-12-01T14:22:17Z

I see that test /tests/core/editable/manual/enterdiv is failing on Chrome - but it seems like it's also failing on master branch. Not sure if the test started to failing or it has been poorly written.

In fact it was a regression introduced in #3380. Unit tests didn't check for it. I've added the missing test and fixed the current solution to include the case.

I've also added tests for empty paragraph regex.

jacekbogdanski

The test /tests/core/editable/manual/enterdiv is still failing on Firefox, IE (not all editors).

If we've introduced regression recently and it has been already published, we should create a separate ticket for that. Could you do it with some explanation of why the regression happened, as you already checked it out and have more knowledge on the topic?

jacekbogdanski · 2020-12-07T12:05:24Z

tests/core/editable/enterdiv.js

+	return CKEDITOR.tools.array.reduce( tags, function( tests, tag ) {
+		var tagTests = CKEDITOR.tools.array.reduce( contents, function( tests, content ) {
+			var test = {};

-			listener.removeListener();
-			spy.restore();
+			test[ 'empty paragraph test: ' + tag + ' with ' + CKEDITOR.tools.htmlEncode( content ) ] = function( editor ) {
+				var html = '<div>foo</div><div>bar</div>';

-			assert.areSame( expectedGetDataCount, spy.callCount, 'getData count' );
-			assert.areSame( 0, i, 'beforeGetData count' );
-		} );
-	};
+				editor.editable().setHtml( generateEmptyParagraph( tag, content ) );
+				editor.insertHtml( html );
+
+				assert.areSame( html, editor.getData() );
+			};
+
+			return CKEDITOR.tools.object.merge( tests, test );
+		}, {} );
+
+		return CKEDITOR.tools.object.merge( tests, tagTests );
+	}, {} );


Maybe this test generation code would be a bit more readable if we would extract the cartesian product as a separate helper function? So we could just write cartesianProduct( arr1, arr2 ) producing e.g. array-based tuple?

After a second thought, it probably doesn't make much sense as the code is readable enough. Otherwise, we would have to avoid changing tests state directly which may complicate this test further.

f1ames · 2020-12-10T12:46:44Z

@Comandeer, @jacekbogdanski when you will be ready with this PR let's ask @ckeditor/qa-team (@LukaszGudel) to check it too 👍

github-actions · 2020-12-18T09:16:07Z

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

jacekbogdanski · 2021-02-25T10:27:53Z

I did some wrap up about the whole issue with the current implementation, so we can finally proceed with this PR:

The first fix was for Editor with enterMode set to ENTER_DIV alters content on paste #2751, but
1. We introduced a regression for Incorrect getData event during dropping content into editable #3379
2. It didn't totally fix the issue. Some cases like selecting all content with the mouse (not ctrl+A) don't work correctly e.g. on IE11
We did fix for Incorrect getData event during dropping content into editable #3379, but
1. We introduced a regression for 4.15.0 - Paste overwrites existing text #4301
We are here now, fixing 4.15.0 - Paste overwrites existing text #4301, but
1. We experienced all additional edge cases not fixed by partial Editor with enterMode set to ENTER_DIV alters content on paste #2751 fix.

The best approach would be revisiting #2751 and going with the proper fix in the first place. However, since the proper fix may not be trivial and the current issue looks like something which may occur much more often when using the editor, I would be for keeping the current fix and extracting the proper solution as a separate ticket. To compare issues:

Fix in Editor with enterMode set to ENTER_DIV alters content on paste #2751 - is partial and is not covering cases when whole editor selection is made by a mouse due to different selection anchors. Results in incorrectly wrapped HTML with redundant elements.
Current fix - fixes the case when pasting the same text again into the empty editor results in losing pasted data.

So, it makes more sense to go with the current workaround (since it's almost finished) and start thinking about a more complicated, general fix for selection (replacing #2751 entirely), then postponing the fix for the current issue and working on a new fix #2751 right now.

I will go with another review round and if there are only minor issues left (like improving tests), we can go with this PR as the current solution.

github-actions · 2021-03-05T09:13:32Z

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

github-actions · 2021-03-13T09:13:56Z

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

jacekbogdanski · 2021-03-15T15:22:32Z

Looks like we are ready for some QA, @ckeditor/qa-team team could you help with testing this fix? We are focused on fixing #4301 but as explained in #4356 (comment) we still may have some issues with invalid div wrapping, but it should not be a part of this PR. I will extract a follow-up in a moment (#4572).

LukaszGudel · 2021-03-16T13:06:19Z

Other than the scenario from this issue (paste some text from outside) I've found three other ways of inserting content to the editor that are also affected by this bug. Additionally, it is possible to:

drag and drop some text from outside the editor multiple times
insert text using editor.insertHtml('content') multiple times
type some text and then copy it and paste at the end multiple times

In all of the above cases the result is the same, only the last one copied/inserted text is showing.

On the t/4301 branch none of those cases produces this issue. I did not find any other issues with empty editor and div_enter.

jacekbogdanski

Thanks, @LukaszGudel! I see that failing CI is already reported:

#3424
#3868

Good job @Comandeer!

github-actions bot added the stale The issue / PR will be closed within the next 7 days of inactivity. label Nov 11, 2020

jacekbogdanski self-requested a review November 12, 2020 12:35

jacekbogdanski self-assigned this Nov 12, 2020

jacekbogdanski force-pushed the t/4301 branch from 4c15eb8 to 9df8b50 Compare November 13, 2020 07:26

jacekbogdanski requested changes Nov 13, 2020

View reviewed changes

CKEditorBot unassigned jacekbogdanski Nov 13, 2020

github-actions bot removed the stale The issue / PR will be closed within the next 7 days of inactivity. label Nov 13, 2020

Comandeer self-assigned this Nov 17, 2020

Comandeer force-pushed the t/4301 branch from 9df8b50 to e16f526 Compare November 17, 2020 11:51

Comandeer requested a review from jacekbogdanski November 17, 2020 11:51

CKEditorBot unassigned Comandeer Nov 17, 2020

jacekbogdanski self-assigned this Nov 20, 2020

jacekbogdanski requested changes Nov 20, 2020

View reviewed changes

CKEditorBot unassigned jacekbogdanski Nov 20, 2020

github-actions bot added the stale The issue / PR will be closed within the next 7 days of inactivity. label Nov 28, 2020

Comandeer self-assigned this Nov 30, 2020

Comandeer force-pushed the t/4301 branch from e16f526 to b4d9be7 Compare December 1, 2020 14:10

Comandeer requested a review from jacekbogdanski December 1, 2020 14:22

CKEditorBot unassigned Comandeer Dec 1, 2020

github-actions bot removed the stale The issue / PR will be closed within the next 7 days of inactivity. label Dec 2, 2020

jacekbogdanski self-assigned this Dec 7, 2020

jacekbogdanski requested changes Dec 7, 2020

View reviewed changes

CKEditorBot unassigned jacekbogdanski Dec 7, 2020

Comandeer self-assigned this Dec 7, 2020

jacekbogdanski assigned jacekbogdanski and unassigned Comandeer Feb 25, 2021

github-actions bot added the stale The issue / PR will be closed within the next 7 days of inactivity. label Mar 5, 2021

Dumluregn removed the stale The issue / PR will be closed within the next 7 days of inactivity. label Mar 5, 2021

github-actions bot added the stale The issue / PR will be closed within the next 7 days of inactivity. label Mar 13, 2021

Dumluregn removed the stale The issue / PR will be closed within the next 7 days of inactivity. label Mar 15, 2021

jacekbogdanski self-requested a review March 15, 2021 14:33

Comandeer added 9 commits March 15, 2021 15:36

Add manual test.

2136d77

Add unit tests.

8f2eeef

Check directly editable's HTML, not editor's data.

8bac732

Update unit tests.

bd6adf3

Update comments.

d3bde9e

Update version tag in the manual test.

4cd9b43

Add unit test for original issue.

ec62a74

Check editable's content after clearing the range.

1a23f7d

Add tests for empty paragraphs.

c5ab30b

jacekbogdanski force-pushed the t/4301 branch from b4d9be7 to c5ab30b Compare March 15, 2021 14:37

Updated version tag.

33361c3

jacekbogdanski mentioned this pull request Mar 15, 2021

Incorrectly wrapped content when pasting into editor with enter div mode #4572

Open

Added changelog entry. [skip ci]

76bd964

jacekbogdanski approved these changes Mar 17, 2021

View reviewed changes

CKEditorBot unassigned jacekbogdanski Mar 17, 2021

jacekbogdanski merged commit 36f69e5 into master Mar 17, 2021

jacekbogdanski deleted the t/4301 branch March 17, 2021 10:50

CKEditorBot mentioned this pull request Mar 17, 2021

4.15.0 - Paste overwrites existing text #4301

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix overwriting text on paste #4356

Fix overwriting text on paste #4356

Comandeer commented Nov 3, 2020

github-actions bot commented Nov 11, 2020

jacekbogdanski commented Nov 13, 2020

jacekbogdanski left a comment

jacekbogdanski Nov 12, 2020

Comandeer Nov 17, 2020

jacekbogdanski Nov 20, 2020

jacekbogdanski left a comment

jacekbogdanski Nov 20, 2020

github-actions bot commented Nov 28, 2020

Comandeer commented Dec 1, 2020

jacekbogdanski left a comment

jacekbogdanski Dec 7, 2020

jacekbogdanski Mar 15, 2021

f1ames commented Dec 10, 2020

github-actions bot commented Dec 18, 2020

jacekbogdanski commented Feb 25, 2021

github-actions bot commented Mar 5, 2021

github-actions bot commented Mar 13, 2021

jacekbogdanski commented Mar 15, 2021 •

edited

Loading

LukaszGudel commented Mar 16, 2021 •

edited

Loading

jacekbogdanski left a comment

Fix overwriting text on paste #4356

Fix overwriting text on paste #4356

Conversation

Comandeer commented Nov 3, 2020

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

Did you follow the CKEditor 4 code style guide?

What is the proposed changelog entry for this pull request?

What changes did you make?

Which issues does your PR resolve?

github-actions bot commented Nov 11, 2020

jacekbogdanski commented Nov 13, 2020

jacekbogdanski left a comment

Choose a reason for hiding this comment

jacekbogdanski Nov 12, 2020

Choose a reason for hiding this comment

Comandeer Nov 17, 2020

Choose a reason for hiding this comment

jacekbogdanski Nov 20, 2020

Choose a reason for hiding this comment

jacekbogdanski left a comment

Choose a reason for hiding this comment

jacekbogdanski Nov 20, 2020

Choose a reason for hiding this comment

github-actions bot commented Nov 28, 2020

Comandeer commented Dec 1, 2020

jacekbogdanski left a comment

Choose a reason for hiding this comment

jacekbogdanski Dec 7, 2020

Choose a reason for hiding this comment

jacekbogdanski Mar 15, 2021

Choose a reason for hiding this comment

f1ames commented Dec 10, 2020

github-actions bot commented Dec 18, 2020

jacekbogdanski commented Feb 25, 2021

github-actions bot commented Mar 5, 2021

github-actions bot commented Mar 13, 2021

jacekbogdanski commented Mar 15, 2021 • edited Loading

LukaszGudel commented Mar 16, 2021 • edited Loading

jacekbogdanski left a comment

Choose a reason for hiding this comment

jacekbogdanski commented Mar 15, 2021 •

edited

Loading

LukaszGudel commented Mar 16, 2021 •

edited

Loading