Pasting image widget into table cell #694

Comandeer · 2017-07-28T13:12:51Z

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?

Now all non-table/mixed content is passed to other paste handlers and is no longer handled by table paste handler.
Selecting cells after paste was moved to afterPaste handler.
Logic of empting cells before real paste is also updated.
However there is still one issue, which I don't have a good idea how to solve: after selecting more than one cell, pasting into them and clicking "Undo" button, only the first cell is selected. It's caused by the change introduced while passing mixed content to other paste handlers.

closes #520, #460.

f1ames

The code looks good, however there are some failing tests (checked on Chrome and FF):

Comandeer · 2017-08-01T10:31:43Z

I've pushed fixes for failing tests (simple change from paste to afterPaste listener). I've also prepared additional test for testing paste flow when pasting non-tabular content.

mlewand

Here are few notes I made during the review. I went with adding my proposals in a PR based on, and targeted at this PR.

mlewand · 2017-08-04T09:33:00Z

plugins/tableselection/plugin.js

 		if ( tmpContainer.getChildCount() > 1 || !pastedTable ) {
-			selectedCells[ 0 ].setHtml( tmpContainer.getHtml() );
+			evt.data.dataValue = tmpContainer.getHtml();


I don't see a reason for using tmpContainer.getHtml() since evt.data.dataValue is already set, and actually we don't want to mess up evt.data.dataValue if possible, since it's about to be pasted into the table cell.

mlewand · 2017-08-04T09:35:08Z

plugins/tableselection/plugin.js


 		// Handle mixed content (if the table is not the only child in the tmpContainer, we
 		// are probably dealing with mixed content). We handle also non-table content here.
+		// We just collapse selection to the first selected cell and pass non-table/mixed
+		// content to other paste handlers (#520).


Seems to me that this comment is pretty much oudated. How about:

// In case of mixed content or non table content just insert it to a first cell, erasing the others.

?

Actually this paste handler does not perform paste in such case. It just selects the first cell and ends its job – so I don't see my comment as outdated. Maybe it could be a little bit more detailed.

mlewand · 2017-08-04T15:42:45Z

plugins/tableselection/plugin.js

+		// content (#520).
+		editor.once( 'afterPaste', function() {
+			var toSelect = cellToPaste ?
+				getCellsBetween( new CKEDITOR.dom.element( pastedTableMap[ 0 ][ 0 ] ), cellToPaste ) :


Not a critical thing but pastedTableMap is used before it's defined, we should avoid this kind of code.

Comandeer · 2017-08-10T09:58:22Z

Ok, I've merged t/520_fix branch and newest master. I've also fixed failing tests in IE11.

mlewand · 2017-08-10T10:52:20Z

@f1ames please check whether your issues have been addressed.

f1ames

As for my requests, I see all tests are green now.

f1ames · 2017-08-10T11:19:55Z

@mlewand should I merge it, or you will do the honors?

f1ames · 2017-08-10T12:00:10Z

Merging now :)

…o selection is given. This will make the API more consistent. Covered change with unit tests.

…sition.

This caused a bug that if pasted table was bigger than the current selection, the output would be missing some cells. Covered the change with unit test.

…tomized by tableselection. That caused user to be unable to paste a nested table into an existing table.

…rtColumn functions.

…n was present table would not be pasted and merged correctly.

…oArray.

mlewand · 2017-08-10T12:06:08Z

Sry I was out, please do the honors 🙂

mlewand · 2017-08-25T07:28:04Z

Looks like this PR also fixed #460 🎉.

f1ames self-requested a review July 31, 2017 10:44

f1ames suggested changes Aug 1, 2017

View reviewed changes

mlewand self-requested a review August 4, 2017 09:31

mlewand suggested changes Aug 9, 2017

View reviewed changes

Comandeer requested a review from mlewand August 10, 2017 09:58

mlewand approved these changes Aug 10, 2017

View reviewed changes

f1ames self-requested a review August 10, 2017 11:17

f1ames approved these changes Aug 10, 2017

View reviewed changes

wojtekw92 and others added 17 commits August 10, 2017 14:01

unit test for pasting image2 widget to table with tableselect plugin

2e16b06

whitespace fixes

6345724

Update unit test.

6cd7e32

Add manual test.

a1b9f09

Move evt.stop after all early returns.

2e914c3

Pass pasting to other paste handlers.

a9a03f0

Add undo integration tests.

5b046cd

Introduce emptyCells function.

84eaa5c

Update unit tests.

a6230f3

Select cells after paste.

636e816

Move assertion to afterPaste event listener.

797b10a

Add paste flow test case for non-tabular content.

aa6f3e0

Refactoring: tabletools.getSelectedCells() return an empty array if n…

61cf773

…o selection is given. This will make the API more consistent. Covered change with unit tests.

Refactoring: paste listener simplified.

90d437a

Refactoring: extracted some logic from boundary case handling.

723f0ad

Refactoring: extracted function responsible for parsing pasted content.

ffb1c13

Updated the comment according to my review suggestion.

b118d86

mlewand and others added 21 commits August 10, 2017 14:01

Refactoring: extracted pasteTable method.

f58d4d2

Refactoring: further simplifications.

d52f17e

Added a function that will reorder selected cells according to DOM po…

4be8747

…sition.

Added some API docs.

b23b4a8

Fixed outdated cells passed to insertColumn / insertRow.

825c6fa

This caused a bug that if pasted table was bigger than the current selection, the output would be missing some cells. Covered the change with unit test.

Fixed an edge case when inserting rows.

f127170

Fixed a case when pasting into a partially selected cell would be cus…

c6484e8

…tomized by tableselection. That caused user to be unable to paste a nested table into an existing table.

Refactoring: minor adjustments.

d2c0312

Refactoring: renamed paste listener.

037d80c

Make sure that cells in TableSelection are in DOM order.

7094ce2

Docs: corrected API docs.

5289cbf

Extracted a function for erasing selected cells content.

58fe2e7

Added unit tests for values returned by tabletools insertRow and inse…

77dd8af

…rtColumn functions.

Fixed a bug where merged table would have an invalid structure.

517d900

Added handling for an edge case where, in certain scenario, if rowspa…

76de5a7

…n was present table would not be pasted and merged correctly.

Refactoring: added code comment and a missing semicolon.

51757d5

Fixed test fixture id.

1048d62

Replace custom TableSelection._nodeListToArray with native NodeList.t…

fa6bf28

…oArray.

Update unit test.

770bf8f

Add missing ticket number.

f11e3af

Refactoring: renamed variable, joined var statements.

6a54e33

f1ames force-pushed the t/520 branch from 618c171 to 3a1c850 Compare August 10, 2017 12:03

Changelog entry.

95ea14e

f1ames force-pushed the t/520 branch from 3a1c850 to 95ea14e Compare August 10, 2017 12:07

f1ames merged commit deed636 into master Aug 10, 2017

f1ames deleted the t/520 branch August 10, 2017 12:08

mlewand mentioned this pull request Aug 25, 2017

Editor gone after pasting into an editor within a table #460

Closed

jacekbogdanski mentioned this pull request Jan 29, 2018

Copy & paste in a table #1178

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pasting image widget into table cell #694

Pasting image widget into table cell #694

Comandeer commented Jul 28, 2017 •

edited by mlewand

Loading

f1ames left a comment

Comandeer commented Aug 1, 2017

mlewand left a comment

mlewand Aug 4, 2017

mlewand Aug 4, 2017

Comandeer Aug 9, 2017

mlewand Aug 4, 2017

Comandeer commented Aug 10, 2017

mlewand commented Aug 10, 2017

f1ames left a comment

f1ames commented Aug 10, 2017

f1ames commented Aug 10, 2017

mlewand commented Aug 10, 2017

mlewand commented Aug 25, 2017

Pasting image widget into table cell #694

Pasting image widget into table cell #694

Conversation

Comandeer commented Jul 28, 2017 • edited by mlewand Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

f1ames left a comment

Choose a reason for hiding this comment

Comandeer commented Aug 1, 2017

mlewand left a comment

Choose a reason for hiding this comment

mlewand Aug 4, 2017

Choose a reason for hiding this comment

mlewand Aug 4, 2017

Choose a reason for hiding this comment

Comandeer Aug 9, 2017

Choose a reason for hiding this comment

mlewand Aug 4, 2017

Choose a reason for hiding this comment

Comandeer commented Aug 10, 2017

mlewand commented Aug 10, 2017

f1ames left a comment

Choose a reason for hiding this comment

f1ames commented Aug 10, 2017

f1ames commented Aug 10, 2017

mlewand commented Aug 10, 2017

mlewand commented Aug 25, 2017

Comandeer commented Jul 28, 2017 •

edited by mlewand

Loading