Range#cloneContents changes DOM (and selection)

[Piotr Jasiun]

1. Open Replace by Class sample in Chrome.
2. Select part of the paragraph (e.g., "flight that landed the first hum" from the first paragraph).
3. Type in console:

   CKEDITOR.instances.editor1.getSelection().getRanges()[ 0 ].cloneContents();
   

Result: Range changes.

There is a note in the documentation that it could happen but it is not what I expected using method called cloneContents and it should be changed.

[Piotrek Koszuliński]

This method is useless if it destroys selection. Clone means clone, it should not cause any change to the DOM.

[Piotrek Koszuliński]

Actually the weirdest thing is that this is a range's method and range instance is just a virtual representation of a range. If that was selection's method it would be less odd, but in this case it's truly strange.

[Piotr Jasiun]

Related: #11636

[Piotr Jasiun]

This should be fixed so the getSelectedHtml method does not change DOM.

[Piotrek Koszuliński]

We checked that cloneContents() splits texts nodes and this breaks the selection. We also checked that native cloneContents() does not split texts nodes so we should fix our polyfill.

[Piotr Jasiun]

Also cloneContents should works as the native method. The native method is poorly described: ​http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html, but I create a simple test (attachment) which prove that Chrome, Firefox and IE 11 does not change DOM when native cloneContents is called.

[Piotrek Koszuliński]

Finally! That was tough. Changes in ​branch:t/11586b and followup ticket in #12964 (a bug in other method revealed by this change).

[Piotr Jasiun]

Great job, everything works good.

I have pushed some commits with changes in docs and some more tests. I have added to the docs some information, which was unclear for me, please check is it correct.

Also I notify that there are missing tests for some cases. Please check if these cases really does not need tests or should not be removed:

• ​https://github.com/cksource/ckeditor-dev/blob/t/11586b/core/dom/range.js#L220 missing else path,
• ​https://github.com/cksource/ckeditor-dev/blob/t/11586b/core/dom/range.js#L245 missing else path,
• ​https://github.com/cksource/ckeditor-dev/blob/t/11586b/core/dom/range.js#L340 last condition is never checked (maxLevelRight < maxLevelLeft),
• ​https://github.com/cksource/ckeditor-dev/blob/t/11586b/core/dom/range.js#L523 missing else path, so ​https://github.com/cksource/ckeditor-dev/blob/t/11586b/core/dom/range.js#L526 is never reached.

[Piotrek Koszuliński]

Back on review as thanks to the code coverage I found one broken case. I fixed it, but you can take a look at the fix. This change covered the first two missing else paths.

There are two uncovered expressions still, but I know that one of them was happening in one of the plugins tests and I can't think now about test for the second case, but I don't want to risk removing it :D.

[Piotr Jasiun]

Everything seems to be fine, so finally closed: ​git:11d9004