Opened 10 years ago

Closed 10 years ago

Last modified 9 years ago

#13059 closed Bug (fixed)

Warnings in clipboard/drop tests

Reported by: Piotrek Koszuliński Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.5.0
Component: General Version: 4.5.0 Beta
Keywords: Cc:

Description

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.$

Change History (9)

comment:1 Changed 10 years ago by Piotrek Koszuliński

Status: newconfirmed

comment:2 Changed 10 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

comment:3 Changed 10 years ago by Piotrek Koszuliński

Status: assignedreview

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.

comment:4 Changed 10 years ago by Piotrek Koszuliński

comment:5 Changed 10 years ago by Piotr Jasiun

Status: reviewreview_failed

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.

comment:6 Changed 10 years ago by Piotrek Koszuliński

Status: review_failedreview

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.

comment:7 Changed 10 years ago by Piotrek Koszuliński

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

comment:8 Changed 10 years ago by Piotr Jasiun

Resolution: fixed
Status: reviewclosed

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

comment:9 Changed 9 years ago by Piotr Jasiun

Closed with git:fe4de43.

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy