Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#13409 closed Bug (fixed)

List elements incorrectly merged

Reported by: Szymon Cofalik Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.5.2
Component: General Version: 4.0
Keywords: Cc:

Description (last modified by Piotrek Koszuliński)

Followup for #12729.

Steps to reproduce:

  1. Set this as editor contents:
    <ul>
    	<li><a href="#foo">foo</a>
    
    	<ol>
    		<li><em>bar</em></li>
    	</ol>
    	</li>
    </ul>
    
  2. Set caret after foo (at the end of the list item).
  3. Press delete key.

Result: <em> is inside <a>
Expected result: <em> should be next to <a>

You can test this bug in this manual test (test available git:7456617935): http://tests.ckeditor.dev:1030/tests/plugins/list/manual/mergelistitems

Change History (14)

comment:1 Changed 3 years ago by Szymon Cofalik

Description: modified (diff)

comment:2 Changed 3 years ago by Szymon Cofalik

Description: modified (diff)

comment:3 Changed 3 years ago by Piotr Jasiun

Status: newconfirmed

comment:4 Changed 3 years ago by Piotr Jasiun

It was tested and does not work on Chrome, Firefox and IE8-11.

comment:5 Changed 3 years ago by Piotr Jasiun

It should be fixed by #12729, but it looks that it fixed only Backspace case.

Last edited 3 years ago by Piotr Jasiun (previous) (diff)

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

Description: modified (diff)
Milestone: CKEditor 4.5.1

Very strange that it does not work, because we have tests for delete too. To be investigated in 4.5.1.

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

Ok, I get now why we haven't noticed this. This bug happens only when merging with a nested list. Does not happen when merging with a list item on the same level.

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

There's a http://tests.ckeditor.dev:1030/tests/plugins/list/manual/mergelistitems test, which description I find so obscure that it took me a long, long while to get what's going on there. We should focus on writing MTs that easy, they require no knowledge about the ticket whatsoever.

IMHO, this test should be split into several steps that reflect the problem, like

  1. Set caret here.
  2. Press X.
  3. See if Y is Z.
  1. Set caret there.
  2. Press A.
  3. See if B is C.

...

and then, eventually, at the very end, having ensured testers know what is expected of them, there could be an invitation to play a little bit like

Test merging lists items (and blocks around) with backspace/delete in both editors.

+ general principles.

KISS!

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

Owner: set to Olek Nowodziński
Status: confirmedassigned

comment:10 Changed 3 years ago by Olek Nowodziński

Status: assignedreview

Changes in branch:t/13409, but without comment:8 (will be a separate ticket).

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

Status: reviewreview_failed

I rebased and pushed one additional commit to branch:t/13409. The code looks good, but some of the new tests fail on IE8 and IE11.

comment:12 Changed 3 years ago by Olek Nowodziński

Status: review_failedassigned

comment:13 Changed 3 years ago by Olek Nowodziński

Status: assignedreview

Pushed updates to branch:t/13409.

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

Resolution: fixed
Status: reviewclosed

Fixed on master with git:03c3329.

Last edited 3 years ago by Piotrek Koszuliński (previous) (diff)
Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy