Opened 4 years ago

Closed 4 years ago

#9129 closed Bug (fixed)

Backspace at the start of first list item

Reported by: garry.yao Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.6.5
Component: Core : Lists Version:
Keywords: Cc:

Description (last modified by garry.yao)

The current behavior of backspace at the start of list is browser dependent:

<ol>
	<li>
		^foo</li>
	<li>
		bar</li>
</ol>

We should fix it to be not doing anything, if not inside of a sub list.

Attachments (5)

9129.patch (1.2 KB) - added by garry.yao 4 years ago.
9129_2.patch (3.5 KB) - added by garry.yao 4 years ago.
9129_3.patch (8.1 KB) - added by garry.yao 4 years ago.
9129_4.patch (7.8 KB) - added by garry.yao 4 years ago.
9129_5.patch (8.7 KB) - added by garry.yao 4 years ago.

Download all attachments as: .zip

Change History (29)

comment:1 Changed 4 years ago by garry.yao

  • Description modified (diff)
  • Owner set to garry.yao
  • Status changed from new to review

comment:2 Changed 4 years ago by garry.yao

  • Status changed from review to assigned

Changed 4 years ago by garry.yao

comment:3 follow-up: Changed 4 years ago by garry.yao

  • Status changed from assigned to review

The propose behavior introduce an exception that when list item is empty instead, it will perform an outdent, this matches actually the behavior of most browser impl, and our enterkey behavior as well.

comment:4 Changed 4 years ago by garry.yao

  • Milestone set to CKEditor 3.6.4

comment:5 Changed 4 years ago by garry.yao

comment:6 in reply to: ↑ 3 Changed 4 years ago by fredck

Replying to garry.yao:

The propose behavior introduce an exception that when list item is empty instead, it will perform an outdent, this matches actually the behavior of most browser impl, and our enterkey behavior as well.

Does that mean that, in this case:

 1. Item 1^ 
     * Sub 1
 2. Item 2

If I deleted the entire "Item 1" with BACKSPACE (leaving it empty), it will outdent if BACKSPACE again?

If yes, this is wrong. We can do so only if we're inside an empty "list", not an empty "list item".

comment:7 Changed 4 years ago by fredck

  • Status changed from review to review_failed

Other than the above:

  1. Load this HTML and selection:
<p>Line</p>
<ol>
	<li>^Item 1
		<ol>
			<li>Sub 1</li>
		</ol>
	</li>
	<li>Item 2</li>
</ol>
  1. BACKSPACE.

Nothing should happen.

comment:8 Changed 4 years ago by garry.yao

  • Keywords Discussion added

I can confirm that no browser could agree with the above TC in 7, Fx and Webkit both merges the list item with previous line.

As for 6, empty list item with nested list are not into this case, but I see browsers have problem removing this last char (empty the item) even, so it's blocking the test right now.

Besides, it makes sense to outdent for the following case, which invalidate the "outdent only empty list" argument:

1. ^
2. Item

The browsers behavior for the above case are:

  • Fx : Merge with the next list item
  • Webkit: Outdent the list item
  • IE: Outdent the list item (not case specific)

This would be in fact be the user's way to "exit from the start" of a list structure.

comment:9 Changed 4 years ago by garry.yao

  • Milestone CKEditor 3.6.4 deleted

In any case the impl would not be trivial anymore, so we'd avoid rushing with it at the moment.

comment:10 Changed 4 years ago by fredck

I don't really care about the default browser behavior. I think we can put our heads at work and come with a solution that is based on reasonable arguments instead.

The idea of making it outdent only if there is no sublist is acceptable as well. I'm still worried that users may few confused because sometimes it works, sometimes it doesn't (when having sublist). In any case, this is a risk we can take for now. I'm ok to proceed that way.

comment:11 Changed 4 years ago by wwalc

I've noticed that the behavior has changed in trunk with [7540] (at least in Firefox), when talking about the scenario when list is at the top of the content.

CKEditor 3.6.3:

<ol>
	<li>^
		foo</li>
	<li>
		bar</li>
</ol>

delete results in

<p>
	foo</p>
<ol>
	<li>
		bar</li>
</ol>

In 3.6.4 SVN: nothing happens when backspace is pressed.

The result from 3.6.3 is more intuitive imho.

comment:12 Changed 4 years ago by wwalc

As originally reported in this comment, the whole list is now deleted in Opera (I've filled a ticket for it: #9158). It makes this particular ticket even more important.

comment:13 Changed 4 years ago by garry.yao

  • Status changed from review_failed to review

New patch summarized the above discussion result, which outdent with the one of following cases:

  1. Empty list item - indicates a user wish to remove the list item
  2. No content before to merge with, e.g. at the start of doc - deconstruct list, try to avoid the "doing nothing" behavior.

comment:14 Changed 4 years ago by garry.yao

  • Keywords Discussion removed
  • Milestone set to CKEditor 3.6.5

Changed 4 years ago by garry.yao

comment:15 Changed 4 years ago by garry.yao

Updated ticket test: ​http://ckeditor.t/tt/8248/1.html

comment:16 Changed 4 years ago by fredck

  • Status changed from review to review_failed

I think there was a misinterpretation on our discussion. The BACKSPACE key must have no action if the list has a sublist. In all other cases, the list gets transformed into a paragraph.

I've updated the tests to reflect this.

Changed 4 years ago by garry.yao

comment:17 Changed 4 years ago by garry.yao

  • Status changed from review_failed to review

As #9152 is highly related to this ticket, I'd merge the review request here.

New patch is to adjust the behavior (to not outdent) when sub list is pertained, but instead of doing nothing in such case, we'd just move the cursor to the desired direction indicated by the keystroke (backward for BACKSPACE, and forward for DEL).

To clarify, this new "move cursor instead of deletion" is not our invention, but a standard behavior of browser impl, e.g. when del key is pressed at the end of a table, followed by a list:

// DEL pressed will move cursor forward
<table>
	<tr>
		<td>cell^</td>
	</tr>
</table>
<p>foo</p>


// BACKSPACE pressed will move cursor backward
<table>
	<tr>
		<td>cell</td>
	</tr>
</table>
<p>^foo</p>

This can be verified in many browsers, thus it makes sense for us to reuse it when we dont want sub list to be merged.

comment:18 Changed 4 years ago by fredck

  • Status changed from review to review_failed

I'm "ok" with the selection positioning on the "do nothing" case. Let's see how it'll be accepted by our users.

There are some bad decisions on the tests though.

Why "list2b_del_before" should behave differently than "list3_del_before"? We're just breaking an important current feature with that.

Additionally, there are two tests that have been changed but they're certainly correct.

I've updated t/9129 on tests to reflect this.

Changed 4 years ago by garry.yao

comment:19 Changed 4 years ago by garry.yao

  • Status changed from review_failed to review

Updated the patch to match the current TC set.

comment:20 follow-up: Changed 4 years ago by fredck

  • Status changed from review to review_failed

Do you confirm that the current two reds that we have are just a matter of updating the tests?
http://ckeditor.t/dt/plugins/keystrokes/list.html

If yes, please do so before putting it on review again.

comment:21 in reply to: ↑ 20 Changed 4 years ago by fredck

Replying to fredck:

Do you confirm that the current two reds that we have are just a matter of updating the tests?
http://ckeditor.t/dt/plugins/keystrokes/list.html

I have these reds with Chrome only. With FF it is ok.

Changed 4 years ago by garry.yao

comment:22 Changed 4 years ago by garry.yao

  • Status changed from review_failed to review

At first glance it's a test problem, but it's instead caused by missing filling char when the paragraph is empty, so I had to patched once more the filling char logic.

comment:23 Changed 4 years ago by fredck

  • Status changed from review to review_passed

comment:24 Changed 4 years ago by garry.yao

  • Resolution set to fixed
  • Status changed from review_passed to closed

Fixed with [7601].

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