Opened 5 years ago

Closed 4 years ago

#11586 closed Bug (fixed)

Range#cloneContents changes DOM (and selection)

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

Description

  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.

Attachments (1)

native-cloneContents.html (948 bytes) - added by Piotr Jasiun 4 years ago.

Download all attachments as: .zip

Change History (13)

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

Status: newconfirmed

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

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

Summary: cloneContents changes rangeRange#cloneContents changes DOM (and selection)

comment:3 Changed 5 years ago by 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.

comment:4 Changed 5 years ago by Piotr Jasiun

Related: #11636

comment:5 Changed 4 years ago by Piotr Jasiun

Milestone: CKEditor 4.5.0

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

comment:6 Changed 4 years ago by 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.

Changed 4 years ago by Piotr Jasiun

Attachment: native-cloneContents.html added

comment:7 Changed 4 years ago by 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.

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

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

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

Status: assignedreview

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

comment:10 Changed 4 years ago by Piotr Jasiun

Status: reviewreview_passed

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:

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

Status: review_passedreview

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.

comment:12 Changed 4 years ago by Piotr Jasiun

Resolution: fixed
Status: reviewclosed

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

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy