Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix overwriting text on paste #4356

Merged
merged 11 commits into from
Mar 17, 2021
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ CKEditor 4 Changelog
Fixed Issues:

* [#4433](https://github.com/ckeditor/ckeditor4/issues/4433): Fixed: Paragraph before or after a [widget](https://ckeditor.com/cke4/addon/widget) can not be removed. Thanks to [bunglegrind](https://github.com/bunglegrind)!
* [#4301](https://github.com/ckeditor/ckeditor4/issues/4301): Fixed: Pasted content is overwritten when pasted in initially empty editor with [`div` enter mode](https://ckeditor.com/docs/ckeditor4/latest/features/enterkey.html).

## CKEditor 4.16

Expand Down
16 changes: 13 additions & 3 deletions core/editable.js
Original file line number Diff line number Diff line change
Expand Up @@ -1647,7 +1647,9 @@
// guarantee it's result to be a valid DOM tree.
function insert( editable, type, data, range ) {
var editor = editable.editor,
dontFilter = false;
dontFilter = false,
html,
isEmptyEditable;

if ( type == 'unfiltered_html' ) {
type = 'html';
Expand Down Expand Up @@ -1685,9 +1687,17 @@

prepareRangeToDataInsertion( that );

html = editable.getHtml(),
// Instead of getData method, we directly check the HTML
// due to the fact that internal getData operates on latest snapshot,
// not the current content.
// Checking it after clearing the range's content will give the
// most correct results (#4301).
isEmptyEditable = html === '' || html.match( emptyParagraphRegexp );

// When enter mode is set to div and content wrapped with div is pasted,
// we must ensure that no additional divs are created (#2751, #3379).
if ( editor.enterMode === CKEDITOR.ENTER_DIV && editor.getData( true ) === '' ) {
jacekbogdanski marked this conversation as resolved.
Show resolved Hide resolved
// we must ensure that no additional divs are created (#2751).
if ( editor.enterMode === CKEDITOR.ENTER_DIV && isEmptyEditable ) {
clearEditable( editable, range );
}

Expand Down
107 changes: 83 additions & 24 deletions tests/core/editable/enterdiv.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,53 @@ var tests = {
'test insert two divs': testInsertHtml( '<div>foo</div><div>bar</div>' ),

// (#2751)
'test insert two divs wrapped in another div': testInsertHtml( '<div><div>foo</div><div>bar</div></div>' ),

// (#3379)
'test getData call (div enter mode)': testGetData()
'test insert two divs wrapped in another div': testInsertHtml( '<div><div>foo</div><div>bar</div></div>' )
};

tests = CKEDITOR.tools.object.merge( tests, generateEmptyParagraphsTests() );
tests = bender.tools.createTestsForEditors( CKEDITOR.tools.object.keys( bender.editors ), tests );

// (#3379)
tests[ 'test getData call (p enter mode)' ] = function() {
bender.editorBot.create( {}, function( bot ) {
testGetData()( bot.editor, bot );
// (#2751, #4301)
tests[ 'reinsert the current data' ] = function() {
var html = '<div>This is some sample text.</div><div>New line.</div>';

bender.editorBot.create( {
name: 'reinsert',
startupData: html,
config: {
plugins: 'selectall',
enterMode: CKEDITOR.ENTER_DIV
}
}, function( bot ) {
var editor = bot.editor,
range;

// Using Select All plugin ensures that the selection is emulating
// real user selection well.
editor.execCommand( 'selectAll' );
range = editor.getSelection().getRanges()[ 0 ];

editor.insertHtml( html, 'html', range );

assert.areSame( html, editor.getData() );
} );
};

// (#4301)
tests[ 'test multiple paste into empty editor' ] = function() {
bender.editorBot.create( {
name: 'multiple_paste',
config: {
enterMode: CKEDITOR.ENTER_DIV
}
}, function( bot ) {
var html = 'Lorem ipsum',
editor = bot.editor;

editor.insertHtml( html );
editor.insertHtml( html );

assert.areSame( '<div>' + html + html + '</div>', editor.getData() );
} );
};

Expand All @@ -51,23 +86,47 @@ function testInsertHtml( htmlString ) {
};
}

function testGetData() {
return function( editor, bot ) {
bot.setData( '', function() {
var i = 0,
listener = editor.on( 'beforeGetData', function() {
++i;
} ),
spy = sinon.spy( CKEDITOR.editor.prototype, 'getData' ),
expectedGetDataCount = Number( editor.config.enterMode === CKEDITOR.ENTER_DIV );
function generateEmptyParagraphsTests() {
var tags = [
'p',
'div',
'address',
'h1',
'h2',
'h3',
'h4',
'h5',
'h6',
'center',
'pre'
],
contents = [
'<br>',
'&nbsp;',
'\u00A0',
'&#160;'
];

editor.insertHtml( 'hublabubla' );
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 );
}, {} );
Comment on lines +110 to +127
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


function generateEmptyParagraph( tag, content ) {
return '<' + tag + '>' + content + '</' + tag + '>';
}
}
8 changes: 8 additions & 0 deletions tests/core/editable/manual/enterdivemptypaste.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<div>Lorem ipsum dolor sit amet</div>
<div id="editor"></div>

<script>
CKEDITOR.replace( 'editor', {
enterMode: CKEDITOR.ENTER_DIV
} );
</script>
17 changes: 17 additions & 0 deletions tests/core/editable/manual/enterdivemptypaste.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
@bender-tags: 4.16.1, bug, 4301
@bender-ui: collapsed
@bender-ckeditor-plugins: clipboard, toolbar, wysiwygarea, sourcearea

## For each editor

1. Copy anything (e.g. text above the editor).
2. Paste it in the editor.
3. Without moving selection, paste it once more.

## Expected

The text is pasted twice.

## Unexpected

The text is pasted once.