Opened 4 years ago

Closed 4 years ago

#12141 closed Bug (fixed)

List is lost when indenting in <li><p>

Reported by: Piotrek Koszuliński Owned by: Artur Delura
Priority: Normal Milestone: CKEditor 4.4.6
Component: General Version: 4.2
Keywords: Cc:

Description (last modified by Piotrek Koszuliński)

  1. Set source:
    <ul>
    <li><p>First</p></li>
    <li><p>Second</p></li>
    </ul>
    
  2. Place caret in the first item.
  3. Press indent button.

Second item is lost.

Change History (13)

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

Status: newconfirmed

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

Description: modified (diff)

comment:3 Changed 4 years ago by Jakub Ś

Version: 4.2

#12467 was marked as duplicate.

Same problem can be reproduced with below code:

<ol>
	<li>
	<pre>
test</pre>
	</li>
	<li>
	<p>paragraph 1</p>
	</li>
	<li>item 3</li>
	<li>item 4</li>
	<li>item 5</li>
</ol>

Problem can be reproduced from CKEditor 4.2.

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

Milestone: CKEditor 4.4.6

comment:5 Changed 4 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:6 Changed 4 years ago by Artur Delura

Status: assignedreview

Pull request from lykahb was almost fine. His commit fixed issue described in this test (to write this one I had to make firstItemInPath public). The remaining problem was following: When indentblock plugin was enabled, caret was at the first list item and list item include paragraph, then indent button was disabled. I fixed this issue and unified behaviour with one when list item didn't include paragraph. So now when user click indent button, then whole list is indented. Fix is here - instead of disabling button I switch context to closest li. Changes in branch:t/12141.

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

Status: reviewreview_failed
  1. Docs are written incorrectly. First of all, make sure to generate it, because your additions throw errors. Second, read about the order of @tags - http://docs.ckeditor.com/#!/guide/dev_code_documentation. Third, document what the query argument is (you can simply link to the elementPath.contains doc).
  2. The name of the new test file is incorrect.
  3. Add more tests for the changes in the indent* plugins. You should check what indentblock does, what indentlist does, what both do together. And also you should check cases like selection in second paragraph in list item, in the second list item, etc - so cases which should not be touched by the change you did.

comment:8 in reply to:  7 Changed 4 years ago by Artur Delura

Replying to Reinmar:

  1. Docs are written incorrectly. First of all, make sure to generate it, because your additions throw errors. Second, read about the order of @tags - http://docs.ckeditor.com/#!/guide/dev_code_documentation. Third, document what the query argument is (you can simply link to the elementPath.contains doc).

Done

  1. The name of the new test file is incorrect.

Done

  1. Add more tests for the changes in the indent* plugins. You should check what indentblock does, what indentlist does, what both do together. And also you should check cases like selection in second paragraph in list item, in the second list item, etc - so cases which should not be touched by the change you did.

I have written 8 tests (2 * 2 * 2) for various cases:

  • whether indentBlock is enbled.
  • whether list items are wrapped in p element.
  • whether caret is at the first item.

Chages and tests in branch:t/12141.

comment:9 Changed 4 years ago by Artur Delura

Status: review_failedreview

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

Summary: [PR#108] List is lost when indenting in <li><p>List is lost when indenting in <li><p>

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

I rebased the branch:t/12141 and added one more commit there (cleanup - make sure to check what I changed).

On the last meetup we talked about adding manual tests with basic samples which will let us to quickly check the TC without the need to build your own samples. This ticket requires testing 3 configurations and only one is available in our samples.

Could you create 3 manual tests (in Bender) with short description what should be checked?

comment:12 Changed 4 years ago by Artur Delura

I added manual tests changes in the same branch.

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

Resolution: fixed
Status: reviewclosed

Good work! Fixed on master with git:a112253.

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