Opened 8 years ago
Closed 8 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
- Open:
samples/old/replacebycode.html
- Select from the begging to 'Americans' (
[<h1>Apollo 11<h1><p>Apollo 11 was the spaceflight that landed the first humans,]
) - Press Ctrl+X.
Result:
- the <p> is changed to <h1> (what is not that bad)
- 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 8 years ago by
Owner: | set to Szymon Cofalik |
---|---|
Status: | new → assigned |
comment:2 Changed 8 years ago by
comment:3 Changed 8 years ago by
Simpified case:
- Open editor with following content and selection:
<div>[Apollo 11</div> <p><strong>foo</strong> bar <strong>baz</strong> ]ban <strong>buz</strong></p>
- Press Ctrl + X
Result:
<p><strong>buz</strong>ban </p>
Expected result:
<p>ban <strong>buz</strong></p>
comment:4 Changed 8 years ago by
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 8 years ago by
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 8 years ago by
Status: | assigned → review |
---|
comment:7 Changed 8 years ago by
Milestone: | → CKEditor 4.5.0 |
---|---|
Version: | → 4.5.0 Beta |
comment:8 Changed 8 years ago by
Status: | review → review_passed |
---|
comment:9 Changed 8 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Merged into major in git:e480e6f.
After bisecting first bad commit is: git:039322aeab