Opened 10 years ago

Closed 10 years ago

#3312 closed Bug (fixed)

It's not possible to remove list on empty <li> ENTER

Reported by: Frederico Caldeira Knabben Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.0
Component: General Version:
Keywords: Confirmed Review+ Cc:

Description

When hitting ENTER in in the last <li> of a list, if empty, the <li> must be removed, and the a new paragraph with the selection must be placed after the list.

Attachments (4)

3312.patch (10.1 KB) - added by Garry Yao 10 years ago.
3312_2.patch (2.5 KB) - added by Garry Yao 10 years ago.
3312_3.patch (3.1 KB) - added by Garry Yao 10 years ago.
3312_4.patch (636 bytes) - added by Garry Yao 10 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 10 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

Changed 10 years ago by Garry Yao

Attachment: 3312.patch added

comment:2 Changed 10 years ago by Garry Yao

Keywords: Review? added

Though this feature depends on #3189 to work,it's still possible to have a code complete without the other.

comment:3 Changed 10 years ago by Josh Nisly

One (very) minor comment: in the patch, the line that starts with

// 2. Exist the whole list

should be

// 2. Exit the whole list

comment:4 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
  • The range class already offers the "moveToElementEditStart" function. Part of the code in the enterkey plugin is a duplicate of it.
  • Please do not include stuff that's not related to the fix, otherwise it's get hard to understand the reason why things got changed, making the review process more difficult.
  • Just o make it easier to understand, are the changes on scrollIntoView an attempt to simplify it, or it's really fixing something?

Changed 10 years ago by Garry Yao

Attachment: 3312_2.patch added

comment:5 in reply to:  4 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed

Replying to fredck:

  • The range class already offers the "moveToElementEditStart" function. Part of the code in the enterkey plugin is a duplicate of it.

'moveToElementEditStart' was used in 'splitBlock' function will guarantee there's always been a following editable block element, it's different with the logic in this function.

Just to make it easier to understand, are the changes on scrollIntoView an attempt to simplify it, or it's really fixing something?

Sorry,I've been include the wrong patch trunk here, it's not related to the ticket at all.

comment:6 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

Ok you are correct of course. But, the private getNextEditableElement is not needed at all. It's called only once, to set the editableElement var value. There is no need for all that complications: editableElement = walker.next();

In any case, this doesn't look like the correct approach. The correct behavior is having an empty paragraph being created after the list. Actually, we should "outdent" the list. In V2, we do that by using the list functions, but in V3 those are into separated plugins. So, we must create a plugin dependence here, and make the enterkey plugin use the relative code from indent plugin (we must probably expose the functions to public there). the V2 implementation should be checked.

Changed 10 years ago by Garry Yao

Attachment: 3312_3.patch added

comment:7 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:8 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
  • The enterkey plugin must now have a dependent (requires) for the list plugin.
  • The range bookmark should be used instead of a custom "marker" system.
  • There is no need to save a range clone (orginalRange) for every call just for this case. It's enough to create a new range before the outdentList call (reusing the "range" variable). Also, that same range can be used to create the above bookmark and selected it later.
  • Minor coding style issues in the indent/plugin.js changes.

comment:9 in reply to:  8 Changed 10 years ago by Garry Yao

Replying to fredck:

  • The range bookmark should be used instead of a custom "marker" system.

After agreed with you on this, I found the tiny difference between this and the bookmark nodes:

  1. Bookmark nodes are invisible, which will be ingored by domiterator, thus the outdentList will not creating a new paragraph after outdent, which make IE has nowhere to place the carot after the list;
  2. The marker node not only record the editing position, it also helps to establish a paragraph after the list;
  • There is no need to save a range clone (orginalRange) for every call just for this case. It's enough to create a new range before the outdentList call (reusing the "range" variable). Also, that same range can be used to create the above bookmark and selected it later.

The reason for save the range is avoid it been changed when calling 'splitBlock' later, which I comment out, save before 'outdentList' call is too later for us to plant the marker node.

Please comment on those two points before my attaching a new patch.

Changed 10 years ago by Garry Yao

Attachment: 3312_4.patch added

comment:10 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed

A more simplified way could be found.

comment:11 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

That's a wonderful solution Garry ;)

comment:12 Changed 10 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [3449].

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