Warnings in clipboard/drop tests

[Piotrek Koszuliński]

​http://tests.ckeditor.dev:1030/tests/plugins/clipboard/drop

Logs dozens of:

[CKEDITOR.dom.range.setEnd] Element CKEDITOR.dom.element is not a descendant of root CKEDITOR.tools.createClass.$
range.js:2701 [CKEDITOR.dom.range.setStart] Element CKEDITOR.dom.element is not a descendant of root CKEDITOR.tools.createClass.$

[Piotrek Koszuliński]

It happened to be an interesting ticket, because there were 3 separate issues - all proving that the validation inside range which we added recently is really useful.

1. First commit solves a bug when incorrect editable was used to extract content from the source editor.
2. Second commit fixes a bug in tests which took me a good time to find. Since there is an inline and divarea and the same ids were used in both editors editor.document.getById started returning a wrong elements.
3. The last piece is for IE8 on which some errors were still logged, however, I think it also may fix some issues. Basically, there should be some selection inside the source editor, so when you return to it everything works (undo, and the locked selection). I was worried that this will break the focus which on cross-editor DnD should land in the target editor, but on normal browsers everything is fine. One IEs this bit was broken even before this patch, so no regression as well.

[Piotrek Koszuliński]

Pushed ​branch:t/13059.

[Piotr Jasiun]

The solution is good, all of these changes should be done in the first place. However if such issues occurred we need tests, which will be red, not only show some messages in the console, to be sure that they will not come back during future code changes. So there are missing tests for:

• check if editor.toolbox.focus() is not called if there is no toolbox,
• extractHtmlFromRange( dragRange ) is called on the source editor editable in the cross editor drag and drop,
• range is properly set on the source editor after the cross editor drag and drop.

[Piotrek Koszuliński]

The solution is good, all of these changes should be done in the first place. However if such issues occurred we need tests, which will be red, not only show some messages in the console, to be sure that they will not come back during future code changes.

I agree and disagree at the same time :D. I would also like that every little broken bit bleed, but at the same time too many things bleed randomly in IEs so we cannot stop tests with window.onerror. Therefore, we need to react whenever we see warnings/errors in the console which do not fail tests. Just like this time.

Of course, we can also write tests for these specific small bits like you asked me. But the question is whether it is worth it. We are not any more testing the core of the feature, but some details that were broken but most likely will never be broken again (because thanks to errors it is pretty obvious that something is wrong).

So it's a matter of time and added value. We should not test everything, because we will be testing forever. We should test things that matter.

Pushed ​branch:t/13059.

[Piotrek Koszuliński]

PS. The change with the toolbox was not part of this branch.

[Piotr Jasiun]

Tests green. When I removed code - red. Works. Closed.

[Piotr Jasiun]

Closed with ​git:fe4de43.