Opened 4 years ago

Closed 4 years ago

#13449 closed Bug (fixed)

Cut reverse notes

Reported by: Piotr Jasiun Owned by: Szymon Cofalik
Priority: Normal Milestone: CKEditor 4.5.0
Component: General Version: 4.5.0 Beta
Keywords: Cc:

Description

  1. Open: samples/old/replacebycode.html
  2. Select from the begging to 'Americans' ([<h1>Apollo 11<h1><p>Apollo 11 was the spaceflight that landed the first humans,])
  3. Press Ctrl+X.

Result:

  1. the <p> is changed to <h1> (what is not that bad)
  2. all nodes in the paragraph are in the REVERS order (what is the biggest WTF of the week)

So we have:

, on the Moon on July 20, 1969, at 20:18 UTC. Armstrong became the first to step onto the lunar surface 6 hours later on July 21 at 02:56 UTC.Buzz Aldrin and Neil Armstrong Americans

instead of:

, Americans Neil Armstrong and Buzz Aldrin, on the Moon on July 20, 1969, at 20:18 UTC. Armstrong became the first to step onto the lunar surface 6 hours later on July 21 at 02:56 UTC.

Reproduced on Opera, Chrome and Firefox, so wherever we have custom "Cut" handling.

Change History (9)

comment:1 Changed 4 years ago by Szymon Cofalik

Owner: set to Szymon Cofalik
Status: newassigned

comment:2 Changed 4 years ago by Szymon Cofalik

After bisecting first bad commit is: git:039322aeab

comment:3 Changed 4 years ago by Artur Delura

Simpified case:

  1. Open editor with following content and selection:
    <div>[Apollo 11</div>
    
    <p><strong>foo</strong> bar <strong>baz</strong> ]ban <strong>buz</strong></p>
    
  2. Press Ctrl + X

Result:

<p><strong>buz</strong>ban </p>

Expected result:

<p>ban <strong>buz</strong></p>
Last edited 4 years ago by Artur Delura (previous) (diff)

comment:4 Changed 4 years ago by Szymon Cofalik

Fix available on branch:t/13449 however I don't think this is the end of the work with this part of the code.

What comes to my mind is providing better test cases and maybe moving some of internal functions to public scope to test them (mergeElements() I am looking at you). I would also look if during whole cutting and merging we really need so many bookmarks (at one moment I had <h1><bookmark><bookmark><bookmark></h1>).

comment:5 Changed 4 years ago by Szymon Cofalik

Also, I would like to know why mergeElements() has to use loop to "throw" nodes from one node to another one. Shouldn't be easier just to move all childNodes of one node? I guess there might be some edge cases where this won't result in proper merge, but I can't think of any now.

comment:6 Changed 4 years ago by Szymon Cofalik

Status: assignedreview

comment:7 Changed 4 years ago by Olek Nowodziński

Milestone: CKEditor 4.5.0
Version: 4.5.0 Beta

comment:8 Changed 4 years ago by Olek Nowodziński

Status: reviewreview_passed

comment:9 Changed 4 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

Merged into major in git:e480e6f.

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