Fix for: Pasting inside editor which contains a table after selecting all content is not working

[Dumluregn]

What is the purpose of this pull request?

Bugfix

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 is the proposed changelog entry for this pull request?

[#875](https://github.com/ckeditor/ckeditor4/issues/875): Fix for: Pasting inside editor which contains a table with [Table Selection plugin](https://ckeditor.com/cke4/addon/tableselection) after selecting all content is not working.

What changes did you make?

The issue here was caused by the custom pasteListener() for table selection plugin. It overrides the default paste logic by moving selection during paste to the first table cell and ignoring the rest of editor:

// In case of mixed content or non table content just select first cell, and erase content of other selected cells.
// Selection is left in first cell, so that default CKEditor logic puts pasted content in the selection (#520).
if ( !pastedTable ) {
	selectCellContents( tableSel.cells.first );

	// Due to limitations of our undo manager, in case of mixed content
	// cells must be emptied after pasting (#520).
	editor.once( 'afterPaste', function() {
		editor.fire( 'lockSnapshot' );
		tableSel.emptyCells( tableSel.cells.all.slice( 1 ) );
		// Reselecting cells allows to create correct undo snapshot (#763).
		fakeSelectCells( editor, tableSel.cells.all );
		editor.fire( 'unlockSnapshot' );
	} );

	return;
}

To solve this, I added an extra condition for ignoring this custom pasting.

Closes #875.

[msamsel]

Your fix covers only the case where selection boundaries are located directly in the editable. There should be a more generic approach. I'd like to avoid the situation in the near future, where it is necessary to fix a very similar issue, but for content surrounded by div, blockquote, nested in another table, etc.
For example, you can add blockuote plugin to your manual test and check that fix stops working in such case:

You can also improve code style inside pasteListener function, as there is already a modification in this part. I would move all helper function to the beginning or to the end of a function scope, so those won't make extra noise during code reading.

I haven't check tests as there is some space for improvement in the proposed fix itself.

[Dumluregn]

Short summary:

• made fix more generic (including mentioned blockquote issue);
• there is known bug in Partial selection outside and inside the table cause cells to be deleted when pressing Del or Backspace #541 which limits the possibilities of this fix;
• when selecting cell with a nested table it will still act the same way as it seems impossibru to fix without major refactor or breaking something else;
• refactored and improved code style;
• rebased onto lastest master.

I had some major doubts about explicitly naming table elements in this line, but didn't manage to find any existing abstraction for them 🤔

[msamsel]

I noticed one bug, however, I see it was also present in 4.12.1.
When you use delete or backspace the content in table cell remains, however when you paste new content, then the entire table cell is cleared out. I couldn't find any similar issue reported.
Maybe you could make short research if this is somehow related to your fix and link it or report as a new issue. I don't want to spend too much time on it as it looks like an edge case.
Correct:

Wrong:

[Dumluregn]

Using rangeContainsTableElement() seems to fix also the bug you mentioned here:

[Dumluregn]

I had to add one positive early return in isCustomPaste() as tests pasting table into boundary selection had detected bugs.

[msamsel]

Overall we're closer to the final solution.
Can you also propose the unit tests for cases checked in point 2 and 5? I couldn't find any unit test utilize those and it could be nice to have proper unit tests for them as well.

[msamsel]

There are a few places like this one, where are:

• comment describing what is happening
• named variable

It's not always necessary, especially for simple conditions, as it duplicates the same information. I would choose one approach and use it here and in further cases.
Situations like this also tend to desynchronize the code, because the condition might be updated without comment modification or variable name remain the same when conditions are modified and new ones are added.

[Dumluregn]

Well, maybe I indeed created an information overflow here 🤔. As most of the conditions aren't that long, I'd give up creating variables and go with comments before ifs as they have some added value in helping to understand the conditions, while variable names are sometimes unclear.

[msamsel]

!boundarySelection it seems to be always true, as all the opposite cases when boundarySelection === true was already caught in line 884, point 2.1.

[msamsel]

However, if you consider my comment to point 2.1, it might be required to remain this check here. It might result that boundarySelection will be able to have different values here.

[Dumluregn]

Please check out this comment.

[msamsel]

!isTableElement will be always false, because of cases when !isTableElement === true was already catched in point 3, by earlier if statement.

[Dumluregn]

On one hand it is totally logical, but on the other hand here we explicitly added unrequired parameters to function just to avoid misleading in the future. I'm afraid that if we remove conditions that are unnecessary with current check order above and here, we will lose the information that they are still important and we don't place them just to keep code DRY. We could keep information about omitted conditions in comment, but it won't be save neither as code may desynchronize with comments as you mentioned here. WDYT?

[Dumluregn]

Added mentioned tests:

• test isBoundaryWithoutTable case covers point 2. (btw now it's point 3.)
• test isCollapsedPartial case covers point 5.

[msamsel]

Unfortunately unit test checking collapsed-partial fails. Can you take a look what 's happening?

[Dumluregn]

In the course of changing conditions for inCustomPaste() the last one called by me isCollapsedPartial became irrelevant as you noticed here, so I removed it - everything seems to be working fine.

[f1ames]

Rebased onto latest master.

[f1ames]

LGTM, good job @Dumluregn and @msamsel 👍 🎉